Fix string substitution recursion

apache · Dec 17, 2021 · 8060232 · 8060232
1 parent 4a4b753
commit 8060232
Show file tree

Hide file tree

Showing 35 changed files with 710 additions and 86 deletions.
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/AbstractBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/AbstractBuilder.java
@@ -24,6 +24,7 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.filter.CompositeFilter;
 import org.apache.logging.log4j.core.filter.ThresholdFilter;
+import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor;
 import org.apache.logging.log4j.core.lookup.StrSubstitutor;
 import org.apache.logging.log4j.status.StatusLogger;
 
@@ -54,7 +55,7 @@ public abstract class AbstractBuilder {
     public AbstractBuilder() {
         this.prefix = null;
         this.props = new Properties();
-        strSubstitutor = new StrSubstitutor(System.getProperties());
+        strSubstitutor = new ConfigurationStrSubstitutor(System.getProperties());
     }
 
     public AbstractBuilder(String prefix, Properties props) {
@@ -63,7 +64,7 @@ public AbstractBuilder(String prefix, Properties props) {
         Map<String, String> map = new HashMap<>();
         System.getProperties().forEach((k, v) -> map.put(k.toString(), v.toString()));
         props.forEach((k, v) -> map.put(k.toString(), v.toString()));
-        strSubstitutor = new StrSubstitutor(map);
+        strSubstitutor = new ConfigurationStrSubstitutor(map);
     }
 
     public String getProperty(String key) {

diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1Configuration.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1Configuration.java
@@ -55,6 +55,7 @@ public BuilderManager getBuilderManager() {
     @Override
     public void initialize() {
         getStrSubstitutor().setConfiguration(this);
+        getConfigurationStrSubstitutor().setConfiguration(this);
         super.getScheduler().start();
         doConfigure();
         setState(State.INITIALIZED);

diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java
@@ -39,6 +39,7 @@
 import org.apache.logging.log4j.core.config.builder.api.LoggerComponentBuilder;
 import org.apache.logging.log4j.core.config.builder.api.RootLoggerComponentBuilder;
 import org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration;
+import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor;
 import org.apache.logging.log4j.core.lookup.StrSubstitutor;
 import org.apache.logging.log4j.status.StatusLogger;
 import org.apache.logging.log4j.util.Strings;
@@ -89,8 +90,8 @@ public ConfigurationBuilder<BuiltConfiguration> buildConfigurationBuilder(final
             throws IOException {
         try {
             properties.load(input);
-            strSubstitutorProperties = new StrSubstitutor(properties);
-            strSubstitutorSystem = new StrSubstitutor(System.getProperties());
+            strSubstitutorProperties = new ConfigurationStrSubstitutor(properties);
+            strSubstitutorSystem = new ConfigurationStrSubstitutor(System.getProperties());
             final String rootCategoryValue = getLog4jValue(ROOTCATEGORY);
             final String rootLoggerValue = getLog4jValue(ROOTLOGGER);
             if (rootCategoryValue == null && rootLoggerValue == null) {

diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java
@@ -2104,7 +2104,7 @@ public void logMessage(final Level level, final Marker marker, final String fqcn
         try {
             incrementRecursionDepth();
             log(level, marker, fqcn, location, message, throwable);
-        } catch (Exception ex) {
+        } catch (Throwable ex) {
             handleLogMessageException(ex, fqcn, message);
         } finally {
             decrementRecursionDepth();
@@ -2203,9 +2203,9 @@ private void tryLogMessage(final String fqcn,
                                final Throwable throwable) {
         try {
             log(level, marker, fqcn, location, message, throwable);
-        } catch (final Exception e) {
+        } catch (final Throwable t) {
             // LOG4J2-1990 Log4j2 suppresses all exceptions that occur once application called the logger
-            handleLogMessageException(e, fqcn, message);
+            handleLogMessageException(t, fqcn, message);
         }
     }
 
@@ -2218,16 +2218,16 @@ private StackTraceElement getLocation(String fqcn) {
 
     // LOG4J2-1990 Log4j2 suppresses all exceptions that occur once application called the logger
     // TODO Configuration setting to propagate exceptions back to the caller *if requested*
-    private void handleLogMessageException(final Exception exception, final String fqcn, final Message message) {
-        if (exception instanceof LoggingException) {
-            throw (LoggingException) exception;
+    private void handleLogMessageException(final Throwable throwable, final String fqcn, final Message message) {
+        if (throwable instanceof LoggingException) {
+            throw (LoggingException) throwable;
         }
         StatusLogger.getLogger().warn("{} caught {} logging {}: {}",
                 fqcn,
-                exception.getClass().getName(),
+                throwable.getClass().getName(),
                 message.getClass().getSimpleName(),
                 message.getFormat(),
-                exception);
+                throwable);
     }
 
     @Override

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java
@@ -56,8 +56,10 @@
 import org.apache.logging.log4j.core.config.plugins.util.PluginType;
 import org.apache.logging.log4j.core.filter.AbstractFilterable;
 import org.apache.logging.log4j.core.layout.PatternLayout;
+import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor;
 import org.apache.logging.log4j.core.lookup.Interpolator;
 import org.apache.logging.log4j.core.lookup.MapLookup;
+import org.apache.logging.log4j.core.lookup.RuntimeStrSubstitutor;
 import org.apache.logging.log4j.core.lookup.StrLookup;
 import org.apache.logging.log4j.core.lookup.StrSubstitutor;
 import org.apache.logging.log4j.core.net.Advertiser;
@@ -129,7 +131,8 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement
     private List<CustomLevelConfig> customLevels = Collections.emptyList();
     private final ConcurrentMap<String, String> propertyMap = new ConcurrentHashMap<>();
     private final StrLookup tempLookup = new Interpolator(propertyMap);
-    private final StrSubstitutor subst = new StrSubstitutor(tempLookup);
+    private final StrSubstitutor subst = new RuntimeStrSubstitutor(tempLookup);
+    private final StrSubstitutor configurationStrSubstitutor = new ConfigurationStrSubstitutor(subst);
     private LoggerConfig root = new LoggerConfig();
     private final ConcurrentMap<String, Object> componentMap = new ConcurrentHashMap<>();
     private final ConfigurationSource configurationSource;
@@ -217,6 +220,7 @@ public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() {
     public void initialize() {
         LOGGER.debug(Version.getProductString() + " initializing configuration {}", this);
         subst.setConfiguration(this);
+        configurationStrSubstitutor.setConfiguration(this);
         try {
             scriptManager = new ScriptManager(this, watchManager);
         } catch (final LinkageError | Exception e) {
@@ -623,12 +627,16 @@ protected void doConfigure() {
             final Node first = rootNode.getChildren().get(0);
             createConfiguration(first, null);
             if (first.getObject() != null) {
-                subst.setVariableResolver((StrLookup) first.getObject());
+                StrLookup lookup = (StrLookup) first.getObject();
+                subst.setVariableResolver(lookup);
+                configurationStrSubstitutor.setVariableResolver(lookup);
             }
         } else {
             final Map<String, String> map = this.getComponent(CONTEXT_PROPERTIES);
             final StrLookup lookup = map == null ? null : new MapLookup(map);
-            subst.setVariableResolver(new Interpolator(lookup, pluginPackages));
+            Interpolator interpolator = new Interpolator(lookup, pluginPackages);
+            subst.setVariableResolver(interpolator);
+            configurationStrSubstitutor.setVariableResolver(interpolator);
         }
 
         boolean setLoggers = false;
@@ -804,6 +812,11 @@ public StrSubstitutor getStrSubstitutor() {
         return subst;
     }
 
+    @Override
+    public StrSubstitutor getConfigurationStrSubstitutor() {
+        return configurationStrSubstitutor;
+    }
+
     @Override
     public void setAdvertiser(final Advertiser advertiser) {
         this.advertiser = advertiser;

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java
@@ -161,8 +161,8 @@ private void tryCallAppender(final LogEvent event) {
             appender.append(event);
         } catch (final RuntimeException error) {
             handleAppenderError(event, error);
-        } catch (final Exception error) {
-            handleAppenderError(event, new AppenderLoggingException(error));
+        } catch (final Throwable throwable) {
+            handleAppenderError(event, new AppenderLoggingException(throwable));
         }
     }
 

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java
@@ -27,6 +27,7 @@
 import org.apache.logging.log4j.core.LoggerContext;
 import org.apache.logging.log4j.core.async.AsyncLoggerConfigDelegate;
 import org.apache.logging.log4j.core.filter.Filterable;
+import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor;
 import org.apache.logging.log4j.core.lookup.StrSubstitutor;
 import org.apache.logging.log4j.core.net.Advertiser;
 import org.apache.logging.log4j.core.script.ScriptManager;
@@ -116,6 +117,14 @@ public interface Configuration extends Filterable {
 
     StrSubstitutor getStrSubstitutor();
 
+    default StrSubstitutor getConfigurationStrSubstitutor() {
+        StrSubstitutor defaultSubstitutor = getStrSubstitutor();
+        if (defaultSubstitutor == null) {
+            return new ConfigurationStrSubstitutor();
+        }
+        return new ConfigurationStrSubstitutor(defaultSubstitutor);
+    }
+
     void createConfiguration(Node node, LogEvent event);
 
     <T> T getComponent(String name);

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
@@ -23,6 +23,7 @@
 import org.apache.logging.log4j.core.config.composite.CompositeConfiguration;
 import org.apache.logging.log4j.core.config.plugins.util.PluginManager;
 import org.apache.logging.log4j.core.config.plugins.util.PluginType;
+import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor;
 import org.apache.logging.log4j.core.lookup.Interpolator;
 import org.apache.logging.log4j.core.lookup.StrSubstitutor;
 import org.apache.logging.log4j.core.net.UrlConnectionFactory;
@@ -141,7 +142,7 @@ public ConfigurationFactory() {
 
     private static ConfigurationFactory configFactory = new Factory();
 
-    protected final StrSubstitutor substitutor = new StrSubstitutor(new Interpolator());
+    protected final StrSubstitutor substitutor = new ConfigurationStrSubstitutor(new Interpolator());
 
     private static final Lock LOCK = new ReentrantLock();
 

diff --git a/.../src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java b/.../src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java
@@ -79,7 +79,7 @@ public CompositeConfiguration(final List<? extends AbstractConfiguration> config
                 .withStatus(getDefaultStatus());
         for (final Map.Entry<String, String> entry : rootNode.getAttributes().entrySet()) {
             final String key = entry.getKey();
-            final String value = getStrSubstitutor().replace(entry.getValue());
+            final String value = getConfigurationStrSubstitutor().replace(entry.getValue());
             if ("status".equalsIgnoreCase(key)) {
                 statusConfig.withStatus(value.toUpperCase());
             } else if ("dest".equalsIgnoreCase(key)) {

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java
@@ -71,7 +71,7 @@ public JsonConfiguration(final LoggerContext loggerContext, final ConfigurationS
             int monitorIntervalSeconds = 0;
             for (final Map.Entry<String, String> entry : rootNode.getAttributes().entrySet()) {
                 final String key = entry.getKey();
-                final String value = getStrSubstitutor().replace(entry.getValue());
+                final String value = getConfigurationStrSubstitutor().replace(entry.getValue());
                 // TODO: this duplicates a lot of the XmlConfiguration constructor
                 if ("status".equalsIgnoreCase(key)) {
                     statusConfig.withStatus(value);

diff --git a/...j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java b/...j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java
@@ -124,20 +124,20 @@ public Object build() {
         } catch (final ConfigurationException e) { // LOG4J2-1908
             LOGGER.error("Could not create plugin of type {} for element {}", this.clazz, node.getName(), e);
             return null; // no point in trying the factory method
-        } catch (final Exception e) {
+        } catch (final Throwable t) {
             LOGGER.error("Could not create plugin of type {} for element {}: {}",
                     this.clazz, node.getName(),
-                    (e instanceof InvocationTargetException ? ((InvocationTargetException) e).getCause() : e).toString(), e);
+                    (t instanceof InvocationTargetException ? ((InvocationTargetException) t).getCause() : t).toString(), t);
         }
         // or fall back to factory method if no builder class is available
         try {
             final Method factory = findFactoryMethod(this.clazz);
             final Object[] params = generateParameters(factory);
             return factory.invoke(null, params);
-        } catch (final Exception e) {
+        } catch (final Throwable t) {
             LOGGER.error("Unable to invoke factory method in {} for element {}: {}",
                     this.clazz, this.node.getName(),
-                    (e instanceof InvocationTargetException ? ((InvocationTargetException) e).getCause() : e).toString(), e);
+                    (t instanceof InvocationTargetException ? ((InvocationTargetException) t).getCause() : t).toString(), t);
             return null;
         }
     }
@@ -180,7 +180,9 @@ private void injectFields(final Builder<?> builder) throws IllegalAccessExceptio
                     final Object value = visitor.setAliases(aliases)
                         .setAnnotation(a)
                         .setConversionType(field.getType())
-                        .setStrSubstitutor(configuration.getStrSubstitutor())
+                        .setStrSubstitutor(event == null
+                                ? configuration.getConfigurationStrSubstitutor()
+                                : configuration.getStrSubstitutor())
                         .setMember(field)
                         .visit(configuration, node, event, log);
                     // don't overwrite default values if the visitor gives us no value to inject
@@ -253,7 +255,9 @@ private Object[] generateParameters(final Method factory) {
                     final Object value = visitor.setAliases(aliases)
                         .setAnnotation(a)
                         .setConversionType(types[i])
-                        .setStrSubstitutor(configuration.getStrSubstitutor())
+                        .setStrSubstitutor(event == null
+                                ? configuration.getConfigurationStrSubstitutor()
+                                : configuration.getStrSubstitutor())
                         .setMember(factory)
                         .visit(configuration, node, event, log);
                     // don't overwrite existing values if the visitor gives us no value to inject

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java
@@ -110,7 +110,7 @@ public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSo
             int monitorIntervalSeconds = 0;
             for (final Map.Entry<String, String> entry : attrs.entrySet()) {
                 final String key = entry.getKey();
-                final String value = getStrSubstitutor().replace(entry.getValue());
+                final String value = getConfigurationStrSubstitutor().replace(entry.getValue());
                 if ("status".equalsIgnoreCase(key)) {
                     statusConfig.withStatus(value);
                 } else if ("dest".equalsIgnoreCase(key)) {

diff --git a/...-core/src/main/java/org/apache/logging/log4j/core/lookup/ConfigurationStrSubstitutor.java b/...-core/src/main/java/org/apache/logging/log4j/core/lookup/ConfigurationStrSubstitutor.java
@@ -0,0 +1,47 @@
+package org.apache.logging.log4j.core.lookup;
+
+import java.util.Map;
+import java.util.Properties;
+
+/**
+ * {@link RuntimeStrSubstitutor} is a {@link StrSubstitutor} which only supports recursive evaluation of lookups.
+ * This can be dangerous when combined with user-provided inputs, and should only be used on data directly from
+ * a configuration.
+ */
+public final class ConfigurationStrSubstitutor extends StrSubstitutor {
+
+    public ConfigurationStrSubstitutor() {
+    }
+
+    public ConfigurationStrSubstitutor(final Map<String, String> valueMap) {
+        super(valueMap);
+    }
+
+    public ConfigurationStrSubstitutor(final Properties properties) {
+        super(properties);
+    }
+
+    public ConfigurationStrSubstitutor(final StrLookup lookup) {
+        super(lookup);
+    }
+
+    public ConfigurationStrSubstitutor(final StrSubstitutor other) {
+        super(other);
+    }
+
+    @Override
+    boolean isRecursiveEvaluationAllowed() {
+        return true;
+    }
+
+    @Override
+    void setRecursiveEvaluationAllowed(final boolean recursiveEvaluationAllowed) {
+        throw new UnsupportedOperationException(
+                "recursiveEvaluationAllowed cannot be modified within ConfigurationStrSubstitutor");
+    }
+
+    @Override
+    public String toString() {
+        return "ConfigurationStrSubstitutor{" + super.toString() + "}";
+    }
+}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ContextMapLookup.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ContextMapLookup.java
@@ -55,6 +55,6 @@ private ReadOnlyStringMap currentContextData() {
      */
     @Override
     public String lookup(final LogEvent event, final String key) {
-        return event.getContextData().getValue(key);
+        return event == null ? null : event.getContextData().getValue(key);
     }
 }
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/DateLookup.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/DateLookup.java
@@ -54,7 +54,7 @@ public String lookup(final String key) {
      */
     @Override
     public String lookup(final LogEvent event, final String key) {
-        return formatDate(event.getTimeMillis(), key);
+        return event == null ? lookup(key) : formatDate(event.getTimeMillis(), key);
     }
 
     private String formatDate(final long date, final String format) {

diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/EventLookup.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/EventLookup.java
@@ -33,6 +33,9 @@ public class EventLookup extends AbstractLookup {
      */
     @Override
     public String lookup(final LogEvent event, final String key) {
+        if (event == null) {
+            return null;
+        }
         switch (key) {
             case "Marker": {
                 return event.getMarker() != null ? event.getMarker().getName() : null;