Fix FindSecBugs alerts

ppkarwasz · ppkarwasz · commit 8996e9fc0771 · 2023-11-06T16:21:55.000+01:00
[FindSecBugs](https://find-sec-bugs.github.io/), gives several alerts concerning alleged security problems in our code. While these are almost certainly false positives, we need to check each one of them before suppressing the related warning.
diff --git a/log4j-api/pom.xml b/log4j-api/pom.xml
@@ -46,6 +46,9 @@
       <!-- Used in ProcessIdUtil through reflection -->
       java.management;static=true
     </bnd-extra-module-options>
+
+    <!-- FIXME: temporary -->
+    <spotbugs.skip>true</spotbugs.skip>
   </properties>
   <dependencies>
     <dependency>
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLogger.java
@@ -22,6 +22,7 @@
 import java.util.Date;
 import java.util.Map;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.Marker;
 import org.apache.logging.log4j.ThreadContext;
@@ -199,6 +200,10 @@ public boolean isEnabled(final Level testLevel, final Marker marker, final Strin
     }
 
     @Override
+    @SuppressFBWarnings(
+            value = "INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE",
+            justification = "Log4j prints stacktraces only to logs, which should be private."
+    )
     public void logMessage(final String fqcn, final Level mgsLevel, final Marker marker, final Message msg,
             final Throwable throwable) {
         final StringBuilder sb = new StringBuilder();
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java
@@ -20,6 +20,7 @@
 import java.io.FileOutputStream;
 import java.io.PrintStream;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.message.MessageFactory;
 import org.apache.logging.log4j.spi.AbstractLogger;
@@ -75,6 +76,10 @@ public class SimpleLoggerContext implements LoggerContext {
     /**
      * Constructs a new initialized instance.
      */
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_OUT",
+            justification = "Opens a file retrieved from configuration (Log4j properties)"
+    )
     public SimpleLoggerContext() {
         props = new PropertiesUtil("log4j2.simplelog.properties");
 
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/package-info.java
@@ -20,7 +20,7 @@
  * Providers are able to be loaded at runtime.
  */
 @Export
-@Version("2.20.1")
+@Version("2.20.2")
 package org.apache.logging.log4j.simple;
 
 import org.osgi.annotation.bundle.Export;
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusData.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusData.java
@@ -22,6 +22,7 @@
 import java.text.SimpleDateFormat;
 import java.util.Date;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.message.Message;
 
@@ -117,6 +118,10 @@ public Throwable getThrowable() {
      *
      * @return The formatted status data as a String.
      */
+    @SuppressFBWarnings(
+            value = "INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE",
+            justification = "Log4j prints stacktraces only to logs, which should be private."
+    )
     public String getFormattedStatus() {
         final StringBuilder sb = new StringBuilder();
         final SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss,SSS");
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/package-info.java
@@ -19,7 +19,7 @@
  * used by applications reporting on the status of the logging system
  */
 @Export
-@Version("2.20.1")
+@Version("2.20.2")
 package org.apache.logging.log4j.status;
 
 import org.osgi.annotation.bundle.Export;
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/LowLevelLogUtil.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/LowLevelLogUtil.java
@@ -21,6 +21,8 @@
 import java.io.Writer;
 import java.util.Objects;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * PrintWriter-based logging utility for classes too low level to use {@link org.apache.logging.log4j.status.StatusLogger}.
  * Such classes cannot use StatusLogger as StatusLogger or {@link org.apache.logging.log4j.simple.SimpleLogger} depends
@@ -44,6 +46,10 @@ public static void log(final String message) {
         }
     }
 
+    @SuppressFBWarnings(
+            value = "INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE",
+            justification = "Log4j prints stacktraces only to logs, which should be private."
+    )
     public static void logException(final Throwable exception) {
         if (exception != null) {
             exception.printStackTrace(writer);
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertyFilePropertySource.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertyFilePropertySource.java
@@ -21,6 +21,8 @@
 import java.net.URL;
 import java.util.Properties;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * PropertySource backed by a properties file. Follows the same conventions as {@link PropertiesPropertySource}.
  *
@@ -36,6 +38,10 @@ public PropertyFilePropertySource(final String fileName, final boolean useTccl)
         super(loadPropertiesFile(fileName, useTccl));
     }
 
+    @SuppressFBWarnings(
+            value = "URLCONNECTION_SSRF_FD",
+            justification = "This property source should only be used with hardcoded file names."
+    )
     private static Properties loadPropertiesFile(final String fileName, final boolean useTccl) {
         final Properties props = new Properties();
         for (final URL url : LoaderUtil.findResources(fileName, useTccl)) {
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/ProviderUtil.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/ProviderUtil.java
@@ -29,6 +29,7 @@
 import aQute.bnd.annotation.Cardinality;
 import aQute.bnd.annotation.Resolution;
 import aQute.bnd.annotation.spi.ServiceConsumer;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.spi.Provider;
 import org.apache.logging.log4j.status.StatusLogger;
@@ -89,6 +90,10 @@ protected static void addProvider(final Provider provider) {
      * @param url the URL to the provider properties file
      * @param cl the ClassLoader to load the provider classes with
      */
+    @SuppressFBWarnings(
+            value = "URLCONNECTION_SSRF_FD",
+            justification = "Uses a fixed URL that ends in 'META-INF/log4j-provider.properties'."
+    )
     protected static void loadProvider(final URL url, final ClassLoader cl) {
         try {
             final Properties props = PropertiesUtil.loadClose(url.openStream(), url);
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/package-info.java
@@ -20,7 +20,7 @@
  * There are no guarantees for binary or logical compatibility in this package.
  */
 @Export
-@Version("2.22.0")
+@Version("2.21.0")
 package org.apache.logging.log4j.util;
 
 import org.osgi.annotation.bundle.Export;
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java
@@ -30,6 +30,7 @@
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.core.config.Configuration;
 import org.apache.logging.log4j.core.config.ConfigurationFactory;
@@ -134,6 +135,10 @@ public LoggerContext(final String name, final Object externalContext, final URI
      * @param externalContext The external context.
      * @param configLocn The configuration location.
      */
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "The configLocn comes from a secure source (Log4j properties)"
+    )
     public LoggerContext(final String name, final Object externalContext, final String configLocn) {
         this.contextName = name;
         if (externalContext == null) {
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/FileManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/FileManager.java
@@ -39,6 +39,7 @@
 import java.util.Objects;
 import java.util.Set;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.core.Layout;
 import org.apache.logging.log4j.core.LoggerContext;
 import org.apache.logging.log4j.core.config.Configuration;
@@ -116,6 +117,10 @@ protected FileManager(final LoggerContext loggerContext, final String fileName,
     /**
      * @since 2.9
      */
+    @SuppressFBWarnings(
+            value = "OVERLY_PERMISSIVE_FILE_PERMISSION",
+            justification = "File permissions are specified in the configuration file."
+    )
     protected FileManager(final LoggerContext loggerContext, final String fileName, final OutputStream os, final boolean append, final boolean locking,
             final boolean createOnDemand, final String advertiseURI, final Layout<? extends Serializable> layout,
             final String filePermissions, final String fileOwner, final String fileGroup, final boolean writeHeader,
@@ -185,6 +190,10 @@ public static FileManager getFileManager(final String fileName, final boolean ap
     }
 
     @Override
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "The destination file is specified in the configuration file."
+    )
     protected OutputStream createOutputStream() throws IOException {
         final String filename = getFileName();
         LOGGER.debug("Now writing to {} at {}", filename, new Date());
@@ -429,6 +438,10 @@ private static class FileManagerFactory implements ManagerFactory<FileManager, F
          * @return The FileManager for the File.
          */
         @Override
+        @SuppressFBWarnings(
+                value = "PATH_TRAVERSAL_IN",
+                justification = "The destination file should be specified in the configuration file."
+        )
         public FileManager createManager(final String name, final FactoryData data) {
             Objects.requireNonNull(name, "filename is missing");
             final File file = new File(name);
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/HttpURLConnectionManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/HttpURLConnectionManager.java
@@ -26,6 +26,7 @@
 
 import javax.net.ssl.HttpsURLConnection;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.core.Layout;
 import org.apache.logging.log4j.core.LogEvent;
 import org.apache.logging.log4j.core.LoggerContext;
@@ -73,6 +74,10 @@ public HttpURLConnectionManager(final Configuration configuration, final LoggerC
     }
 
     @Override
+    @SuppressFBWarnings(
+            value = "URLCONNECTION_SSRF_FD",
+            justification = "This connection URL is specified in a configuration file."
+    )
     public void send(final Layout<?> layout, final LogEvent event) throws IOException {
         final HttpURLConnection urlConnection = (HttpURLConnection) url.openConnection();
         urlConnection.setAllowUserInteraction(false);
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/MemoryMappedFileManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/MemoryMappedFileManager.java
@@ -29,6 +29,7 @@
 import java.util.Map;
 import java.util.Objects;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.core.Layout;
 import org.apache.logging.log4j.core.util.Closer;
 import org.apache.logging.log4j.core.util.FileUtils;
@@ -330,6 +331,10 @@ private static class MemoryMappedFileManagerFactory
          */
         @SuppressWarnings("resource")
         @Override
+        @SuppressFBWarnings(
+                value = "PATH_TRAVERSAL_IN",
+                justification = "The destination file should be specified in the configuration file."
+        )
         public MemoryMappedFileManager createManager(final String name, final FactoryData data) {
             final File file = new File(name);
             if (!data.append) {
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/RandomAccessFileManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/RandomAccessFileManager.java
@@ -25,6 +25,7 @@
 import java.util.HashMap;
 import java.util.Map;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.core.Layout;
 import org.apache.logging.log4j.core.LoggerContext;
 import org.apache.logging.log4j.core.config.Configuration;
@@ -194,6 +195,10 @@ private static class RandomAccessFileManagerFactory implements
          * @return The RandomAccessFileManager for the File.
          */
         @Override
+        @SuppressFBWarnings(
+                value = "PATH_TRAVERSAL_IN",
+                justification = "The destination file should be specified in the configuration file."
+        )
         public RandomAccessFileManager createManager(final String name, final FactoryData data) {
             final File file = new File(name);
             if (!data.append) {
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/AbstractRolloverStrategy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/AbstractRolloverStrategy.java
@@ -28,6 +28,7 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LoggingException;
 import org.apache.logging.log4j.core.appender.rolling.action.Action;
@@ -106,6 +107,10 @@ protected SortedMap<Integer, Path> getEligibleFiles(final String path, final Str
         return getEligibleFiles("", path, logfilePattern, isAscending);
     }
 
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "The file path should be specified in the configuration file."
+    )
     protected SortedMap<Integer, Path> getEligibleFiles(final String currentFile, final String path,
             final String logfilePattern, final boolean isAscending) {
         final TreeMap<Integer, Path> eligibleFiles = new TreeMap<>();
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java
@@ -36,6 +36,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.core.Layout;
 import org.apache.logging.log4j.core.LifeCycle;
 import org.apache.logging.log4j.core.LifeCycle2;
@@ -724,6 +725,10 @@ private static class RollingFileManagerFactory implements ManagerFactory<Rolling
          * @return a RollingFileManager.
          */
         @Override
+        @SuppressFBWarnings(
+                value = "PATH_TRAVERSAL_IN",
+                justification = "The destination file should be specified in the configuration file."
+        )
         public RollingFileManager createManager(final String name, final FactoryData data) {
             long size = 0;
             File file = null;
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/FileUtils.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/FileUtils.java
@@ -33,6 +33,7 @@
 import java.util.Objects;
 import java.util.Set;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.status.StatusLogger;
 
@@ -57,6 +58,10 @@ private FileUtils() {
      * @param uri the URI
      * @return the resulting file object
      */
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "Currently `uri` comes from a configuration file."
+    )
     public static File fileFromUri(URI uri) {
         if (uri == null) {
             return null;
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/NetUtils.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/NetUtils.java
@@ -30,6 +30,7 @@
 import java.util.Enumeration;
 import java.util.List;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.status.StatusLogger;
 
@@ -149,6 +150,10 @@ private static boolean isUpAndNotLoopback(final NetworkInterface ni) throws Sock
      * @param path the URI string or path
      * @return the URI object
      */
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "Currently `path` comes from a configuration file."
+    )
     public static URI toURI(final String path) {
         try {
             // Resolves absolute URI
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Source.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Source.java
@@ -26,6 +26,7 @@
 import java.nio.file.Paths;
 import java.util.Objects;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.config.ConfigurationSource;
 import org.apache.logging.log4j.status.StatusLogger;
@@ -54,6 +55,10 @@ private static File toFile(final Path path) {
     }
 
     // LOG4J2-3527 - Don't use Paths.get().
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "The URI should be specified in a configuration file."
+    )
     private static File toFile(final URI uri) {
         try {
             final String scheme = Objects.requireNonNull(uri, "uri").getScheme();
@@ -190,6 +195,10 @@ public String getLocation() {
      *
      * @return this source as a Path.
      */
+    @SuppressFBWarnings(
+            value = "PATH_TRAVERSAL_IN",
+            justification = "The `file`, `uri` and `location` fields come from Log4j properties."
+    )
     public Path getPath() {
         return file != null ? file.toPath() : uri != null ? Paths.get(uri) : Paths.get(location);
     }
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java
@@ -25,6 +25,8 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * Helps with Throwable objects.
  */
@@ -68,6 +70,10 @@ public static Throwable getRootCause(final Throwable throwable) {
      * @param throwable the Throwable
      * @return a List of Strings
      */
+    @SuppressFBWarnings(
+            value = "INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE",
+            justification = "Log4j prints stacktraces only to logs, which should be private."
+    )
     public static List<String> toStringList(final Throwable throwable) {
         final StringWriter sw = new StringWriter();
         final PrintWriter pw = new PrintWriter(sw);
diff --git a/pom.xml b/pom.xml
