Merge pull request #361 from jeffret-b/findsecbugs

Add findsecbugs plugin to spotbugs.

pom.xml

@@ -414,6 +414,19 @@ THE SOFTWARE.
           <rerunFailingTestsCount>4</rerunFailingTestsCount>
         </configuration>
       </plugin>
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <configuration>
+          <plugins>
+            <plugin>
+              <groupId>com.h3xstream.findsecbugs</groupId>
+              <artifactId>findsecbugs-plugin</artifactId>
+              <version>1.10.1</version>
+            </plugin>
+          </plugins>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
 

src/main/java/hudson/remoting/Capability.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.remoting.Channel.Mode;
 import java.io.IOException;
 import java.io.InputStream;
@@ -145,6 +146,7 @@ protected void annotateClass(Class<?> c) throws IOException {
     /**
      * The opposite operation of {@link #writePreamble(OutputStream)}.
      */
+    @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Capability is used for negotiating channel between authorized agent and server. Whitelisting and proper deserialization hygiene are used.")
     public static Capability read(InputStream is) throws IOException {
         try (ObjectInputStream ois = new ObjectInputStream(Mode.TEXT.wrap(is)) {
                 // during deserialization, only accept Capability to protect ourselves

src/main/java/hudson/remoting/Checksum.java

@@ -1,5 +1,7 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.ByteArrayInputStream;
 import java.io.DataInputStream;
 import java.io.File;
@@ -72,6 +74,7 @@ static Checksum forFile(File file) throws IOException {
     /**
      * Returns the checksum for the given URL.
      */
+    @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "This is only used for managing the jar cache as files, not URLs.")
     static Checksum forURL(URL url) throws IOException {
         try {
             MessageDigest md = MessageDigest.getInstance(JarLoaderImpl.DIGEST_ALGORITHM);

src/main/java/hudson/remoting/ClassFilter.java

@@ -16,6 +16,8 @@
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.jenkinsci.remoting.util.AnonymousClassWarnings;
 
 /**
@@ -240,6 +242,7 @@ private static List<String> loadPatternOverride() {
     /**
      * A class that uses a given set of regular expression patterns to determine if the class is blacklisted.
      */
+    @SuppressFBWarnings(value = "REDOS", justification = "In an odd usage, this pattern is used to determine if another pattern matches it and not to match a string to it. REDOS doesn't apply.")
     private static final class RegExpClassFilter extends ClassFilter {
 
         /**

src/main/java/hudson/remoting/Command.java

@@ -23,6 +23,8 @@
  */
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -146,6 +148,7 @@ public static Command readFrom(@Nonnull Channel channel, @Nonnull byte[] payload
 
 
     /** Consider calling {@link Channel#notifyRead} afterwards. */
+    @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Used for sending commands between authorized agent and server. Class filtering is done through JEP-200.")
     static Command readFromObjectStream(Channel channel, ObjectInputStream ois) throws IOException, ClassNotFoundException {
         Channel old = Channel.setCurrent(channel);
         try {

src/main/java/hudson/remoting/DiagnosedStreamCorruptionException.java

@@ -1,5 +1,7 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.PrintWriter;
 import java.io.StreamCorruptedException;
 import java.io.StringWriter;
@@ -42,6 +44,7 @@ public byte[] getReadAhead() {
     }
 
     @Override
+    @SuppressFBWarnings(value = "INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE", justification = "Used for diagnosing stream corruption between agent and server.")
     public String toString() {
         StringBuilder buf = new StringBuilder();
         buf.append(super.toString()).append("\n");

src/main/java/hudson/remoting/Engine.java

@@ -23,6 +23,7 @@
  */
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.remoting.Channel.Mode;
 import java.io.File;
 import java.io.FileInputStream;
@@ -438,6 +439,7 @@ public void removeListener(EngineListener el) {
     }
 
     @Override
+    @SuppressFBWarnings(value = "HARD_CODE_PASSWORD", justification = "Password doesn't need to be protected.")
     public void run() {
         // Create the engine
         try {
@@ -702,6 +704,7 @@ public static Engine current() {
 
     private static final Logger LOGGER = Logger.getLogger(Engine.class.getName());
 
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "File path is loaded from system properties.")
     static KeyStore getCacertsKeyStore()
             throws PrivilegedActionException, KeyStoreException, NoSuchProviderException, CertificateException,
             NoSuchAlgorithmException, IOException {

src/main/java/hudson/remoting/FileSystemJarCache.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.jenkinsci.remoting.util.PathUtils;
 
 import javax.annotation.Nonnull;
@@ -85,6 +86,7 @@ protected URL lookInCache(Channel channel, long sum1, long sum2) throws IOExcept
     }
 
     @Override
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "The file path is a generated value based on server supplied data.")
     protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException, InterruptedException {
         Checksum expected = new Checksum(sum1, sum2);
         File target = map(sum1, sum2);
@@ -170,6 +172,7 @@ private Checksum fileChecksum(File file) throws IOException {
         }
     }
 
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "This path exists within a temp directory so the potential traversal is limited.")
     /*package for testing*/ File createTempJar(@Nonnull File target) throws IOException {
         File parent = target.getParentFile();
         Util.mkdirs(parent);

src/main/java/hudson/remoting/InitializeJarCacheMain.java

@@ -1,5 +1,7 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
@@ -31,6 +33,7 @@ public boolean accept(File dir, String name) {
      * <li>The jar cache directory.
      * </ol>
      */
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "These file values are provided by users with sufficient adminstrative permissions to run this utility program.")
     public static void main(String[] argv) throws Exception {
         if (argv.length != 2) {
             throw new IllegalArgumentException(

src/main/java/hudson/remoting/JarLoaderImpl.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.jenkinsci.remoting.SerializableOnlyOverRemoting;
 
 import java.io.File;
@@ -33,6 +34,7 @@ class JarLoaderImpl implements JarLoader, SerializableOnlyOverRemoting {
 
     private final Set<Checksum> presentOnRemote = Collections.synchronizedSet(new HashSet<Checksum>());
 
+    @SuppressFBWarnings(value = {"URLCONNECTION_SSRF_FD", "PATH_TRAVERSAL_IN"}, justification = "This is only used for managing the jar cache as files, not URLs.")
     public void writeJarTo(long sum1, long sum2, OutputStream sink) throws IOException, InterruptedException {
         Checksum k = new Checksum(sum1, sum2);
         URL url = knownJars.get(k);

src/main/java/hudson/remoting/Launcher.java

@@ -53,6 +53,7 @@
 import org.kohsuke.args4j.CmdLineException;
 
 import javax.crypto.spec.IvParameterSpec;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -146,6 +147,7 @@ public void setTextMode(boolean b) {
 
     @Option(name="-cp",aliases="-classpath",metaVar="PATH",
             usage="add the given classpath elements to the system classloader.")
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Parameter supplied by user / administrator.")
     public void addClasspath(String pathList) throws Exception {
         Method $addURL = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);
         $addURL.setAccessible(true);
@@ -401,6 +403,7 @@ public void run() throws Exception {
     }
 
     @CheckForNull
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Parameter supplied by user / administrator.")
     private SSLSocketFactory getSSLSocketFactory()
             throws PrivilegedActionException, KeyStoreException, NoSuchProviderException, CertificateException,
             NoSuchAlgorithmException, IOException, KeyManagementException {
@@ -489,6 +492,7 @@ private SSLSocketFactory getSSLSocketFactory()
     /**
      * Parses the connection arguments from JNLP file given in the URL.
      */
+    @SuppressFBWarnings(value = {"CIPHER_INTEGRITY", "STATIC_IV"}, justification = "Integrity not needed here. IV used for decryption only, loaded from encryptor.")
     public List<String> parseJnlpArguments() throws ParserConfigurationException, SAXException, IOException, InterruptedException {
         if (secret != null) {
             slaveJnlpURL = new URL(slaveJnlpURL + "?encrypt=true");
@@ -611,15 +615,18 @@ private static byte[] fromHexString(String data) {
     }
 
     private static Document loadDom(URL agentJnlpURL, InputStream is) throws ParserConfigurationException, SAXException, IOException {
-        DocumentBuilder db = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+        dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        DocumentBuilder db = dbf.newDocumentBuilder();
         return db.parse(is, agentJnlpURL.toExternalForm());
     }
 
     /**
      * Listens on an ephemeral port, record that port number in a port file,
      * then accepts one TCP connection.
      */
-    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings("DM_DEFAULT_ENCODING")    // port number file should be in platform default encoding
+    @Deprecated
+    @SuppressFBWarnings(value = {"UNENCRYPTED_SERVER_SOCKET", "DM_DEFAULT_ENCODING"}, justification = "This is an old, insecure mechanism that should be removed. port number file should be in platform default encoding.")
     private void runAsTcpServer() throws IOException, InterruptedException {
         // if no one connects for too long, assume something went wrong
         // and avoid hanging forever
@@ -665,6 +672,8 @@ private void runOnSocket(Socket s) throws IOException, InterruptedException {
     /**
      * Connects to the given TCP port and then start running
      */
+    @SuppressFBWarnings(value = "UNENCRYPTED_SOCKET", justification = "This implements an old, insecure connection mechanism.")
+    @Deprecated
     private void runAsTcpClient() throws IOException, InterruptedException {
         // if no one connects for too long, assume something went wrong
         // and avoid hanging forever
@@ -783,6 +792,7 @@ protected void onDead() {
                     System.exit(-1);
                 }
                 @Override
+                @SuppressFBWarnings(value = "INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE", justification = "Prints the agent-side message to the agent log before exiting.")
                 protected void onDead(Throwable cause) {
                     System.err.println("Ping failed. Terminating");
                     cause.printStackTrace();

src/main/java/hudson/remoting/Pipe.java

@@ -23,6 +23,7 @@
  */
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.jenkinsci.remoting.SerializableOnlyOverRemoting;
 
 import java.io.IOException;
@@ -173,6 +174,7 @@ private void writeObject(ObjectOutputStream oos) throws IOException {
         }
     }
 
+    @SuppressFBWarnings(value = "DESERIALIZATION_GADGET", justification = "Serializable only over remoting. Class filtering is done through JEP-200.")
     private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
         final Channel channel = getChannelForSerialization();
 

src/main/java/hudson/remoting/RemoteClassLoader.java

@@ -794,6 +794,7 @@ public ClassLoaderProxy(@Nonnull ClassLoader cl, Channel channel) {
             this.channel = channel;
         }
 
+        @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "This is only used for managing the jar cache as files.")
         public byte[] fetchJar(URL url) throws IOException {
             return readFully(url.openStream());
         }
@@ -885,6 +886,7 @@ public ClassFile2 fetch4(String className, @CheckForNull ClassFile2 referer) thr
             }
         }
 
+        @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "This is only used for managing the jar cache as files.")
         public Map<String,ClassFile2> fetch3(String className) throws ClassNotFoundException {
             ClassFile2 cf = fetch4(className,null);
             Map<String,ClassFile2> all = new HashMap<String,ClassFile2>();

src/main/java/hudson/remoting/RemoteInputStream.java

@@ -23,6 +23,7 @@
  */
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.jenkinsci.remoting.SerializableOnlyOverRemoting;
 
 import java.io.BufferedInputStream;
@@ -175,6 +176,7 @@ public void run() {
         oos.writeInt(id);
     }
 
+    @SuppressFBWarnings(value = "DESERIALIZATION_GADGET", justification = "Serializable only over remoting. Class filtering is done through JEP-200.")
     private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
         final Channel channel = getChannelForSerialization();
         if (channel.remoteCapability.supportsGreedyRemoteInputStream()) {

src/main/java/hudson/remoting/RemoteInvocationHandler.java

@@ -62,6 +62,7 @@
  */
 //TODO: Likely should be serializable over Remoting logic, but this class has protection logic
 // Use-cases need to be investigated
+@SuppressFBWarnings(value = "DESERIALIZATION_GADGET", justification = "This class has protection logic.")
 final class RemoteInvocationHandler implements InvocationHandler, Serializable {
     /**
      * Our logger.

src/main/java/hudson/remoting/ResourceImageDirect.java

@@ -1,5 +1,7 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.IOException;
 import java.net.URL;
 import java.util.logging.Level;
@@ -15,6 +17,7 @@
  *
  * @author Kohsuke Kawaguchi
  */
+@SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Used by the agent as part of jar cache management.")
 class ResourceImageDirect extends ResourceImageRef {
     /**
      * The actual resource.

src/main/java/hudson/remoting/ResourceImageInJar.java

@@ -1,5 +1,7 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.IOException;
 import java.net.URL;
 import java.net.URLClassLoader;
@@ -42,6 +44,7 @@ class ResourceImageInJar extends ResourceImageRef {
     Future<byte[]> resolve(Channel channel, final String resourcePath) throws IOException, InterruptedException {
         return new FutureAdapter<byte[],URL>(_resolveJarURL(channel)) {
             @Override
+            @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "This is only used for managing the jar cache as files.")
             protected byte[] adapt(URL jar) throws ExecutionException {
                 try {
                     return Util.readFully(toResourceURL(jar,resourcePath).openStream());

src/main/java/hudson/remoting/URLDeserializationHelper.java

@@ -69,12 +69,14 @@ public class URLDeserializationHelper {
 
     private static class SafeURLStreamHandler extends URLStreamHandler {
         @Override
+        @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Used for safely handling URLs, not for opening a connection.")
         protected URLConnection openConnection(URL u, Proxy p) throws IOException
         {
             return new URL(u.toString()).openConnection(p);
         }
 
         @Override
+        @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Used for safely handling URLs, not for opening a connection.")
         protected URLConnection openConnection(URL u) throws IOException {
             return new URL(u.toString()).openConnection();
         }

src/main/java/hudson/remoting/UserRequest.java

@@ -277,6 +277,7 @@ private byte[] serialize(Object o, Channel localChannel) throws IOException {
         }
     }
 
+    @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Used for sending user requests between authorized agent and server.")
     /*package*/ static Object deserialize(final Channel channel, byte[] data, ClassLoader defaultClassLoader) throws IOException, ClassNotFoundException {
         ByteArrayInputStream in = new ByteArrayInputStream(data);
 

src/main/java/hudson/remoting/Util.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.jenkinsci.remoting.util.PathUtils;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -60,6 +61,7 @@ public static void copy(InputStream in, OutputStream out) throws IOException {
     }
 
     @Nonnull
+    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "This path exists within a temp directory so the potential traversal is limited.")
     static File makeResource(String name, byte[] image) throws IOException {
         Path tmpDir = Files.createTempDirectory("resource-");
         File resource = new File(tmpDir.toFile(), name);
@@ -103,6 +105,7 @@ static String indent(String s) {
      * If http_proxy environment variable exists,  the connection uses the proxy.
      * Credentials can be passed e.g. to support running Jenkins behind a (reverse) proxy requiring authorization
      */
+    @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Used for retrieving the connection info from the server. We should cleanup the other, unused references.")
     static URLConnection openURLConnection(URL url, String credentials, String proxyCredentials, SSLSocketFactory sslSocketFactory) throws IOException {
         String httpProxy = null;
         // If http.proxyHost property exists, openConnection() uses it.