You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by jk...@apache.org on 2021/03/23 10:12:12 UTC
[unomi] branch unomi-1.4.x updated: Secure the unomi logs (#266)
This is an automated email from the ASF dual-hosted git repository.
jkevan pushed a commit to branch unomi-1.4.x
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/unomi-1.4.x by this push:
new 4494f92 Secure the unomi logs (#266)
4494f92 is described below
commit 4494f921c174d286a54bb18ea142b5d6595c4107
Author: kevan Jahanshahi <ke...@jahia.com>
AuthorDate: Tue Mar 23 10:55:18 2021 +0100
Secure the unomi logs (#266)
* Secure the unomi logs
* improve logs to use formating instead of string concatenation
---
.../ConditionESQueryBuilderDispatcher.java | 11 ++++-
.../baseplugin/actions/ModifyConsentAction.java | 5 +-
.../conditions/BooleanConditionESQueryBuilder.java | 10 +++-
.../HardcodedPropertyAccessorRegistry.java | 5 +-
.../conditions/PropertyConditionEvaluator.java | 55 +++++++++++++++-------
.../request/actions/SetRemoteHostInfoAction.java | 8 ++--
.../apache/unomi/rest/ClusterServiceEndPoint.java | 5 +-
.../apache/unomi/scripting/ExpressionFilter.java | 12 ++++-
.../java/org/apache/unomi/web/ContextServlet.java | 20 ++++++--
.../apache/unomi/web/EventsCollectorServlet.java | 13 ++++-
10 files changed, 109 insertions(+), 35 deletions(-)
diff --git a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionESQueryBuilderDispatcher.java b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionESQueryBuilderDispatcher.java
index be2535c..97fb810 100644
--- a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionESQueryBuilderDispatcher.java
+++ b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionESQueryBuilderDispatcher.java
@@ -86,7 +86,11 @@ public class ConditionESQueryBuilderDispatcher {
}
// if no matching
- logger.warn("No matching query builder for condition {} and context {}", condition, context);
+ logger.warn("No matching query builder. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("No matching query builder for condition {} and context {}", condition, context);
+ }
+
return QueryBuilders.matchAllQuery();
}
@@ -118,7 +122,10 @@ public class ConditionESQueryBuilderDispatcher {
}
// if no matching
- logger.warn("No matching query builder for condition {} and context {}", condition, context);
+ logger.warn("No matching query builder. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("No matching query builder for condition {} and context {}", condition, context);
+ }
throw new UnsupportedOperationException();
}
}
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/ModifyConsentAction.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/ModifyConsentAction.java
index 7facccf..3ebb034 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/ModifyConsentAction.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/ModifyConsentAction.java
@@ -52,7 +52,10 @@ public class ModifyConsentAction implements ActionExecutor {
consent = new Consent(consentMap, dateFormat);
isProfileUpdated = profile.setConsent(consent);
} catch (ParseException e) {
- logger.error("Error parsing date format", e);
+ logger.error("Error parsing consent dates (statusDate or revokeDate). See debug log level to have more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Error parsing consent dates (statusDate or revokeDate).", e);
+ }
}
} else {
logger.warn("Event properties for modifyConsent is missing typeIdentifier and grant properties. We will ignore this event.");
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/BooleanConditionESQueryBuilder.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/BooleanConditionESQueryBuilder.java
index 8a7a381..5547301 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/BooleanConditionESQueryBuilder.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/BooleanConditionESQueryBuilder.java
@@ -60,7 +60,10 @@ public class BooleanConditionESQueryBuilder implements ConditionESQueryBuilder {
boolQueryBuilder.must(andFilter);
}
} else {
- logger.warn("Null filter for boolean AND sub condition " + conditions.get(i));
+ logger.warn("Null filter for boolean AND sub condition. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Null filter for boolean AND sub condition {}", conditions.get(i));
+ }
}
} else {
QueryBuilder orFilter = dispatcher.buildFilter(conditions.get(i), context);
@@ -71,7 +74,10 @@ public class BooleanConditionESQueryBuilder implements ConditionESQueryBuilder {
boolQueryBuilder.should(orFilter);
}
} else {
- logger.warn("Null filter for boolean OR sub condition " + conditions.get(i));
+ logger.warn("Null filter for boolean OR sub condition. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Null filter for boolean OR sub condition {}", conditions.get(i));
+ }
}
}
}
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/HardcodedPropertyAccessorRegistry.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/HardcodedPropertyAccessorRegistry.java
index 56f8cd5..ed0d5c1 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/HardcodedPropertyAccessorRegistry.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/HardcodedPropertyAccessorRegistry.java
@@ -134,7 +134,10 @@ public class HardcodedPropertyAccessorRegistry {
}
}
}
- logger.warn("Couldn't find any property access for class {} and expression {}", object.getClass().getName(), expression);
+ logger.warn("Couldn't find any property access for class {}. See debug log level for more information", object.getClass().getName());
+ if (logger.isDebugEnabled()) {
+ logger.debug("Couldn't find any property access for class {} and expression {}", object.getClass().getName(), expression);
+ }
return HardcodedPropertyAccessor.PROPERTY_NOT_FOUND_MARKER;
}
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java
index d2e4185..9329f3d 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java
@@ -182,7 +182,10 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
if (!(e instanceof OgnlException)
|| (!StringUtils.startsWith(e.getMessage(),
"source is null for getProperty(null"))) {
- logger.warn("Error evaluating value for " + item.getClass().getName() + " " + name, e);
+ logger.warn("Error evaluating value for {} {}. See debug level for more information", item.getClass().getName(), name);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Error evaluating value for {} {}", item.getClass().getName(), name, e);
+ }
}
actualValue = null;
}
@@ -269,7 +272,10 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
if (useOGNLScripting) {
return getOGNLPropertyValue(item, expression);
} else {
- logger.warn("OGNL Off. Expression not evaluated on item {} : {}", item.getClass().getName(), expression);
+ logger.warn("OGNL Off. Expression not evaluated on item {}. See debug log level for more information", item.getClass().getName());
+ if (logger.isDebugEnabled()) {
+ logger.debug("OGNL Off. Expression not evaluated on item {}: {}", item.getClass().getName(), expression);
+ }
return null;
}
}
@@ -282,7 +288,10 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
protected Object getOGNLPropertyValue(Item item, String expression) throws Exception {
if (expressionFilterFactory.getExpressionFilter("ognl").filter(expression) == null) {
- logger.warn("Expression {} is not allowed !", expression);
+ logger.warn("OGNL expression filtered because not allowed on item: {}. See debug log level for more information", item.getClass().getName());
+ if (logger.isDebugEnabled()) {
+ logger.debug("OGNL expression filtered because not allowed: {}", expression);
+ }
return null;
}
OgnlContext ognlContext = getOgnlContext(secureFilteringClassLoader);
@@ -291,7 +300,11 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
try {
return accessor.get(ognlContext, item);
} catch (Throwable t) {
- logger.error("Error evaluating expression {} on item {} : {}", expression, item.getClass().getName(), t);
+ logger.error("Error evaluating expression on item {}. See debug level for more information", item.getClass().getName());
+ if (logger.isDebugEnabled()) {
+ logger.debug("Error evaluating expression {} on item {}.", expression, item.getClass().getName(), t);
+ }
+
return null;
}
}
@@ -325,15 +338,17 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
@Override
public boolean isAccessible(Map context, Object target, Member member, String propertyName) {
int modifiers = member.getModifiers();
- if (target instanceof Item) {
- if ("getClass".equals(member.getName())) {
- logger.warn("Target {} and member {} for property {} are not allowed by OGNL security filter", target, member, propertyName);
- return false;
+ boolean accessible = false;
+ if (target instanceof Item && !"getClass".equals(member.getName())) {
+ accessible = Modifier.isPublic(modifiers);
+ }
+ if (!accessible) {
+ logger.warn("OGNL security filtered target, member for property. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("OGNL security filtered: Target {} and member {} for property {}. Not allowed", target, member, propertyName);
}
- return Modifier.isPublic(modifiers);
}
- logger.warn("Target {} and member {} for property {} are not allowed by OGNL security filter", target, member, propertyName);
- return false;
+ return accessible;
}
}, new ClassLoaderClassResolver(classLoader),
null);
@@ -363,11 +378,16 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
if (accessor != null) {
expressions.put(expression, accessor);
} else {
- logger.warn("Unable to compile expression for {} and {}", clazz, expression);
+ logger.warn("Unable to compile expression for {}. See debug log level for more information", clazz);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Unable to compile expression: {} for: {}", expression, clazz);
+ }
}
- if (logger.isInfoEnabled()) {
- time = System.nanoTime() - time;
- logger.info("Expression compilation for item={} expression={} took {}", item.getClass().getName(), expression, time / 1000000L);
+ time = System.nanoTime() - time;
+ if (logger.isDebugEnabled()) {
+ logger.debug("Expression compilation for item={} expression={} took {}", item.getClass().getName(), expression, time / 1000000L);
+ } else if (logger.isInfoEnabled()) {
+ logger.info("Expression compilation for item={} took {}. See debug log level for more information", item.getClass().getName(), time / 1000000L);
}
}
@@ -389,7 +409,10 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
}
}));
} catch (ElasticsearchParseException e) {
- logger.warn("unable to parse date " + value.toString(), e);
+ logger.warn("unable to parse date. See debug log level for full stacktrace");
+ if (logger.isDebugEnabled()) {
+ logger.debug("unable to parse date {}", value.toString(), e);
+ }
}
}
return null;
diff --git a/plugins/request/src/main/java/org/apache/unomi/plugins/request/actions/SetRemoteHostInfoAction.java b/plugins/request/src/main/java/org/apache/unomi/plugins/request/actions/SetRemoteHostInfoAction.java
index 11c9f74..bfb2919 100644
--- a/plugins/request/src/main/java/org/apache/unomi/plugins/request/actions/SetRemoteHostInfoAction.java
+++ b/plugins/request/src/main/java/org/apache/unomi/plugins/request/actions/SetRemoteHostInfoAction.java
@@ -229,9 +229,9 @@ public class SetRemoteHostInfoAction implements ActionExecutor {
}
return true;
} catch (IOException | GeoIp2Exception e) {
- logger.warn("Cannot resolve IP {}, enable debug log level for complete stacktrace", remoteAddr);
+ logger.warn("Cannot resolve IP, enable debug log level for complete stacktrace");
if (logger.isDebugEnabled()) {
- logger.debug("Cannot resolve IP", e);
+ logger.debug("Cannot resolve IP: {}", remoteAddr, e);
}
}
return false;
@@ -243,9 +243,9 @@ public class SetRemoteHostInfoAction implements ActionExecutor {
try {
addr = InetAddress.getByName(remoteAddr);
} catch (UnknownHostException e) {
- logger.warn("Cannot resolve IP {}, enable debug log level for complete stacktrace", remoteAddr);
+ logger.warn("Cannot resolve IP, enable debug log level for complete stacktrace");
if (logger.isDebugEnabled()) {
- logger.debug("Cannot resolve IP", e);
+ logger.debug("Cannot resolve IP: {}", remoteAddr, e);
}
return false;
}
diff --git a/rest/src/main/java/org/apache/unomi/rest/ClusterServiceEndPoint.java b/rest/src/main/java/org/apache/unomi/rest/ClusterServiceEndPoint.java
index 21134e2..1bb01e3 100644
--- a/rest/src/main/java/org/apache/unomi/rest/ClusterServiceEndPoint.java
+++ b/rest/src/main/java/org/apache/unomi/rest/ClusterServiceEndPoint.java
@@ -86,7 +86,10 @@ public class ClusterServiceEndPoint {
try {
clusterService.purge(new SimpleDateFormat("yyyy-MM-dd").parse(date));
} catch (ParseException e) {
- logger.error("Cannot purge",e);
+ logger.error("Cannot parse date, expected format is: yyyy-MM-dd. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Cannot parse date: {}", date, e);
+ }
}
}
diff --git a/scripting/src/main/java/org/apache/unomi/scripting/ExpressionFilter.java b/scripting/src/main/java/org/apache/unomi/scripting/ExpressionFilter.java
index 74d4974..87669aa 100644
--- a/scripting/src/main/java/org/apache/unomi/scripting/ExpressionFilter.java
+++ b/scripting/src/main/java/org/apache/unomi/scripting/ExpressionFilter.java
@@ -38,11 +38,19 @@ public class ExpressionFilter {
public String filter(String expression) {
if (forbiddenExpressionPatterns != null && expressionMatches(expression, forbiddenExpressionPatterns)) {
- logger.warn("Expression {} is forbidden by expression filter", expression);
+ logger.warn("Expression filtered because forbidden. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Expression {} is forbidden by expression filter", expression);
+ }
+
return null;
}
if (allowedExpressionPatterns != null && !expressionMatches(expression, allowedExpressionPatterns)) {
- logger.warn("Expression {} is not allowed by expression filter", expression);
+ logger.warn("Expression filtered because not allowed. See debug log level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Expression {} is not allowed by expression filter", expression);
+ }
+
return null;
}
return expression;
diff --git a/wab/src/main/java/org/apache/unomi/web/ContextServlet.java b/wab/src/main/java/org/apache/unomi/web/ContextServlet.java
index ed5d06b..9e48a8d 100644
--- a/wab/src/main/java/org/apache/unomi/web/ContextServlet.java
+++ b/wab/src/main/java/org/apache/unomi/web/ContextServlet.java
@@ -74,7 +74,13 @@ public class ContextServlet extends HttpServlet {
public void service(ServletRequest request, ServletResponse response) throws IOException {
final Date timestamp = new Date();
if (request.getParameter("timestamp") != null) {
- timestamp.setTime(Long.parseLong(request.getParameter("timestamp")));
+ try {
+ timestamp.setTime(Long.parseLong(request.getParameter("timestamp")));
+ } catch (NumberFormatException e) {
+ // catch to avoid logging the error with the timestamp value to avoid potential log injection
+ logger.error("Invalid timestamp parameter");
+ return;
+ }
}
HttpServletRequest httpServletRequest = (HttpServletRequest) request;
@@ -99,7 +105,7 @@ public class ContextServlet extends HttpServlet {
if (personaId != null) {
PersonaWithSessions personaWithSessions = profileService.loadPersonaWithSessions(personaId);
if (personaWithSessions == null) {
- logger.error("Couldn't find persona with id=" + personaId);
+ logger.error("Couldn't find persona, please check your personaId parameter");
profile = null;
} else {
profile = personaWithSessions.getPersona();
@@ -119,7 +125,10 @@ public class ContextServlet extends HttpServlet {
contextRequest = mapper.readValue(factory.createParser(stringPayload), ContextRequest.class);
} catch (Exception e) {
((HttpServletResponse)response).sendError(HttpServletResponse.SC_BAD_REQUEST, "Check logs for more details");
- logger.error("Cannot read payload " + stringPayload, e);
+ logger.error("Cannot deserialize the context request payload. See debug level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Cannot deserialize the context request payload: {}", stringPayload, e);
+ }
return;
}
if (contextRequest.getSource() != null) {
@@ -512,7 +521,10 @@ public class ContextServlet extends HttpServlet {
if (value instanceof String) {
String stringValue = (String) value;
if (stringValue.startsWith("script::") || stringValue.startsWith("parameter::")) {
- logger.warn("Scripting detected in context request with value {}, filtering out...", value);
+ logger.warn("Scripting detected in context request, filtering out. See debug level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Scripting detected in context request with value {}, filtering out...", value);
+ }
return null;
} else {
return stringValue;
diff --git a/wab/src/main/java/org/apache/unomi/web/EventsCollectorServlet.java b/wab/src/main/java/org/apache/unomi/web/EventsCollectorServlet.java
index a55adf8..22f1b92 100644
--- a/wab/src/main/java/org/apache/unomi/web/EventsCollectorServlet.java
+++ b/wab/src/main/java/org/apache/unomi/web/EventsCollectorServlet.java
@@ -80,7 +80,13 @@ public class EventsCollectorServlet extends HttpServlet {
private void doEvent(HttpServletRequest request, HttpServletResponse response) throws IOException {
Date timestamp = new Date();
if (request.getParameter("timestamp") != null) {
- timestamp.setTime(Long.parseLong(request.getParameter("timestamp")));
+ try {
+ timestamp.setTime(Long.parseLong(request.getParameter("timestamp")));
+ } catch (NumberFormatException e) {
+ // catch to avoid logging the error with the timestamp value to avoid potential log injection
+ logger.error("Invalid timestamp parameter");
+ return;
+ }
}
HttpUtils.setupCORSHeaders(request, response);
@@ -99,7 +105,10 @@ public class EventsCollectorServlet extends HttpServlet {
eventsCollectorRequest = mapper.readValue(factory.createParser(payload), EventsCollectorRequest.class);
} catch (Exception e) {
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Check logs for more details");
- logger.error("Cannot read payload " + payload, e);
+ logger.error("Cannot read payload. See debug level for more information");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Cannot read payload: {}", payload, e);
+ }
return;
}
if (eventsCollectorRequest == null || eventsCollectorRequest.getEvents() == null) {