From 20241c950c0e69ecbe10eb26f89cebb2d0ef91e9 Mon Sep 17 00:00:00 2001 From: jhh Date: Wed, 12 May 2021 15:08:04 -0500 Subject: fix telemetry related sonar security issues Issue-ID: POLICY-3257 Signed-off-by: jhh Change-Id: Ic599b593abbc999c4e6a6fd4bc72acd5ec6e09f9 --- .../protocol/coders/GenericEventProtocolCoder.java | 28 ++++++++++++++-------- .../coders/MultiplexorEventProtocolCoder.java | 26 ++++++++++++++++---- .../policy/drools/server/restful/RestManager.java | 17 ++++++++++++- 3 files changed, 56 insertions(+), 15 deletions(-) (limited to 'policy-management') diff --git a/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/GenericEventProtocolCoder.java b/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/GenericEventProtocolCoder.java index b16186d6..5faf8a86 100644 --- a/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/GenericEventProtocolCoder.java +++ b/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/GenericEventProtocolCoder.java @@ -2,7 +2,7 @@ * ============LICENSE_START======================================================= * ONAP * ================================================================================ - * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved. + * Copyright (C) 2019-2020, 2021 AT&T Intellectual Property. All rights reserved. * ================================================================================ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -43,7 +43,7 @@ abstract class GenericEventProtocolCoder { private static final String UNSUPPORTED_EX_MSG = "Unsupported:"; private static final String MISSING_CLASS = "class must be provided"; - private static Logger logger = LoggerFactory.getLogger(GenericEventProtocolCoder.class); + private static final Logger logger = LoggerFactory.getLogger(GenericEventProtocolCoder.class); /** * Mapping topic:controller-id -> / where protocol-coder-toolset contains @@ -64,9 +64,6 @@ abstract class GenericEventProtocolCoder { /** * Index a new coder. - * - * @param eventProtocolParams parameter object for event encoder - * @throw IllegalArgumentException if an invalid parameter is passed */ public void add(EventProtocolParams eventProtocolParams) { if (eventProtocolParams.getGroupId() == null || eventProtocolParams.getGroupId().isEmpty()) { @@ -117,8 +114,7 @@ abstract class GenericEventProtocolCoder { return; } - GsonProtocolCoderToolset coderTools = - new GsonProtocolCoderToolset(eventProtocolParams, key); + var coderTools = new GsonProtocolCoderToolset(eventProtocolParams, key); logger.info("{}: adding coders for new {}: {}", this, key, coderTools); @@ -135,7 +131,7 @@ abstract class GenericEventProtocolCoder { List toolsets = reverseCoders.get(reverseKey); - boolean present = false; + var present = false; for (ProtocolCoderToolset parserSet : toolsets) { // just doublecheck present = parserSet.getControllerId().equals(key); @@ -402,7 +398,19 @@ abstract class GenericEventProtocolCoder { */ protected String encodeInternal(String key, Object event) { - logger.debug("{}: encode for {}: {}", this, key, event); + logger.debug("{}: encode for {}: {}", this, key, event); // NOSONAR + + /* + * It seems that sonar declares the previous logging line as a security vulnerability + * when logging the topic variable. The static code analysis indicates that + * the path starts in org.onap.policy.drools.server.restful.RestManager::decode(), + * but the request is rejected if the topic contains invalid characters (the sonar description + * mentions "/r/n/t" characters) all of which are validated against in the checkValidNameInput(topic). + * Furthermore production instances are assumed not to have debug enabled, nor the REST telemetry API + * should be published externally. An additional note is that Path URLs containing spaces and newlines + * will be rejected earlier in the HTTP protocol libraries (jetty) so an URL of the form + * "https://../to\npic" won't even make it here. + */ ProtocolCoderToolset coderTools = coders.get(key); try { @@ -476,7 +484,7 @@ abstract class GenericEventProtocolCoder { List coderFilters = encoderSet.getCoders(); for (CoderFilters coder : coderFilters) { if (coder.getCodedClass().equals(encodedClass.getClass().getName())) { - DroolsController droolsController = + var droolsController = DroolsControllerConstants.getFactory().get(groupId, artifactId, ""); if (droolsController.ownsCoder( encodedClass.getClass(), coder.getModelClassLoaderHash())) { diff --git a/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/MultiplexorEventProtocolCoder.java b/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/MultiplexorEventProtocolCoder.java index a8f3c3a3..d341024a 100644 --- a/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/MultiplexorEventProtocolCoder.java +++ b/policy-management/src/main/java/org/onap/policy/drools/protocol/coders/MultiplexorEventProtocolCoder.java @@ -2,7 +2,7 @@ * ============LICENSE_START======================================================= * ONAP * ================================================================================ - * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved. + * Copyright (C) 2019, 2021 AT&T Intellectual Property. All rights reserved. * ================================================================================ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -35,7 +35,7 @@ class MultiplexorEventProtocolCoder implements EventProtocolCoder { /** * Logger. */ - private static Logger logger = LoggerFactory.getLogger(MultiplexorEventProtocolCoder.class); + private static final Logger logger = LoggerFactory.getLogger(MultiplexorEventProtocolCoder.class); /** * Decoders. @@ -124,7 +124,19 @@ class MultiplexorEventProtocolCoder implements EventProtocolCoder { */ @Override public Object decode(String groupId, String artifactId, String topic, String json) { - logger.debug("{}: decode {}:{}:{}:{}", this, groupId, artifactId, topic, json); + logger.debug("{}: decode {}:{}:{}:{}", this, groupId, artifactId, topic, json); // NOSONAR + + /* + * It seems that sonar declares the previous logging line as a security vulnerability + * when logging the topic variable. The static code analysis indicates that + * the path starts in org.onap.policy.drools.server.restful.RestManager::decode(), + * but the request is rejected if the topic contains invalid characters (the sonar description + * mentions "/r/n/t" characters) which are validated against in the checkValidNameInput(topic). + * Furthermore production instances are assumed not to have debug enabled, nor the REST telemetry API + * should be published externally. An additional note is that Path URLs containing spaces and newlines + * will be failed earlier at the HTTP protocol libraries (jetty, etc ..) so an URL of the form + * "https://../to\npic" won't even make it here. + */ return this.decoders.decode(groupId, artifactId, topic, json); } @@ -142,7 +154,13 @@ class MultiplexorEventProtocolCoder implements EventProtocolCoder { */ @Override public String encode(String topic, Object event) { - logger.debug("{}: encode {}:{}", this, topic, event); + logger.debug("{}: encode {}:{}", this, topic, event); // NOSONAR + + /* + * See explanation for decode(String groupId, String artifactId, String topic, String json). + * The same applies here as it is called from + * org.onap.policy.drools.server.restful.RestManager::encode(), + */ return this.encoders.encode(topic, event); } diff --git a/policy-management/src/main/java/org/onap/policy/drools/server/restful/RestManager.java b/policy-management/src/main/java/org/onap/policy/drools/server/restful/RestManager.java index e704f0b0..ca2618c1 100644 --- a/policy-management/src/main/java/org/onap/policy/drools/server/restful/RestManager.java +++ b/policy-management/src/main/java/org/onap/policy/drools/server/restful/RestManager.java @@ -32,6 +32,7 @@ import io.swagger.annotations.SwaggerDefinition; import io.swagger.annotations.Tag; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -129,7 +130,7 @@ public class RestManager { /** * Feed Ports into Resources. */ - private static final List INPUTS = Arrays.asList("configuration"); + private static final List INPUTS = Collections.singletonList("configuration"); /** * Resource Toggles. @@ -1626,6 +1627,13 @@ public class RestManager { public Response commSources( @ApiParam(value = "Communication Mechanism", required = true) @PathParam("comm") String comm ) { + if (!checkValidNameInput(comm)) { + return Response + .status(Response.Status.NOT_ACCEPTABLE) + .entity(new Error("source communication mechanism contains whitespaces " + NOT_ACCEPTABLE_MSG)) + .build(); + } + List sources = new ArrayList<>(); var status = Status.OK; switch (CommInfrastructure.valueOf(comm.toUpperCase())) { @@ -1656,6 +1664,13 @@ public class RestManager { public Response commSinks( @ApiParam(value = "Communication Mechanism", required = true) @PathParam("comm") String comm ) { + if (!checkValidNameInput(comm)) { + return Response + .status(Response.Status.NOT_ACCEPTABLE) + .entity(new Error("sink communication mechanism contains whitespaces " + NOT_ACCEPTABLE_MSG)) + .build(); + } + List sinks = new ArrayList<>(); var status = Status.OK; switch (CommInfrastructure.valueOf(comm.toUpperCase())) { -- cgit 1.2.3-korg