Restrict allowed classes during deserialization of signature files (#253

)

* Restrict allowed classes during deserialization of signature files

Because signature files are created using Java Serialization, adds a new
`SignatureObjectInputStream` which restricts the classes which are allowed
to be loaded when reading signature files to increase security.

animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java

@@ -124,8 +124,7 @@ else if ( ( file.getName().endsWith( ".jar" ) || file.getName().endsWith( ".jmod
     /**
      * Recursively finds class files and invokes {@link #process(String, InputStream)}
      *
-     * @param path Directory (or other Path like {@code Paths.get(URI.create("jrt:/modules"))}) full of class files,
-     *             or a class file (in which case that single class is processed).
+     * @param path Directory (or other Path like {@code Paths.get(URI.create("jrt:/modules"))}) full of class files
      */
     public void process( Path path )
         throws IOException

animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Clazz.java

@@ -27,7 +27,7 @@
 
 import java.io.Serializable;
 import java.util.Arrays;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.Objects;
 import java.util.Set;
 
@@ -59,20 +59,56 @@ public final class Clazz
      */
     private final String[] superInterfaces;
 
+    /**
+     * Internal constructor which just assigns all fields, without performing any defensive copying
+     * or similar.
+     *
+     * @param dummy Unused; only needed to avoid constructor signature conflicts
+     */
+    private Clazz( String name, Set<String> signatures, String superClass, String[] superInterfaces, Void dummy ) {
+        this.name = name;
+        this.signatures = signatures;
+        this.superClass = superClass;
+        this.superInterfaces = superInterfaces;
+    }
+
+    /**
+     * Creates a new class signature, with an initially empty set of member signatures.
+     *
+     * @param name            the name of the class.
+     * @param superClass      the superclass.
+     * @param superInterfaces the interfaces implemented by the class; a copy of this array is created.
+     */
+    public Clazz( String name, String superClass, String[] superInterfaces )
+    {
+        this(
+            name,
+            new LinkedHashSet<>(),
+            superClass,
+            superInterfaces.clone(),
+            null
+        );
+    }
+
     /**
      * Creates a new class signature.
      *
      * @param name            the name of the class.
-     * @param signatures      the signatures.
+     * @param signatures      the signatures; a copy of this set is created.
      * @param superClass      the superclass.
-     * @param superInterfaces the interfaces implemented by the class.
+     * @param superInterfaces the interfaces implemented by the class; a copy of this array is created.
      */
     public Clazz( String name, Set<String> signatures, String superClass, String[] superInterfaces )
     {
-        this.name = name;
-        this.signatures = signatures;
-        this.superClass = superClass;
-        this.superInterfaces = superInterfaces.clone();
+        this(
+            name,
+            // Create defensive copy; this also makes sure that field has known JDK Set implementation as value,
+            // which is important for Java Serialization and for SignatureObjectInputStream
+            new LinkedHashSet<>(signatures),
+            superClass,
+            superInterfaces.clone(),
+            null
+        );
     }
 
     /**
@@ -94,7 +130,7 @@ public Clazz( Clazz defA, Clazz defB )
             // nothing we can do... this is a breaking change
             throw new ClassCastException( "Cannot merge class " + defB.name + " as it has changed superclass:" );
         }
-        Set<String> superInterfaces = new HashSet<>();
+        Set<String> superInterfaces = new LinkedHashSet<>();
         if ( defA.superInterfaces != null )
         {
             superInterfaces.addAll( Arrays.asList( defA.superInterfaces ) );
@@ -103,7 +139,7 @@ public Clazz( Clazz defA, Clazz defB )
         {
             superInterfaces.addAll( Arrays.asList( defB.superInterfaces ) );
         }
-        Set<String> signatures = new HashSet<>();
+        Set<String> signatures = new LinkedHashSet<>();
         signatures.addAll( defA.signatures );
         signatures.addAll( defB.signatures );
         this.name = defA.getName();
@@ -117,6 +153,9 @@ public String getName()
         return name;
     }
 
+    /**
+     * Gets a mutable reference to the set of member signatures.
+     */
     public Set<String> getSignatures()
     {
         return signatures;

animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureBuilder.java

@@ -35,7 +35,6 @@
 import java.net.URI;
 import java.nio.file.Paths;
 import java.util.ArrayList;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
@@ -116,7 +115,7 @@ public SignatureBuilder( InputStream[] ins, OutputStream out, Logger logger )
         {
             for ( InputStream in : ins )
             {
-                try (ObjectInputStream ois = new ObjectInputStream( new GZIPInputStream( in ) ))
+                try (ObjectInputStream ois = new SignatureObjectInputStream( new GZIPInputStream( in ) ))
                 {
                     while ( true )
                     {
@@ -182,6 +181,7 @@ public void close()
         }
     }
 
+    @Override
     protected void process( String name, InputStream image )
         throws IOException
     {
@@ -202,10 +202,11 @@ public SignatureVisitor() {
             super(Opcodes.ASM9);
         }
 
+        @Override
         public void visit( int version, int access, String name, String signature, String superName,
                            String[] interfaces )
         {
-            this.clazz = new Clazz( name, new HashSet<>(), superName, interfaces );
+            this.clazz = new Clazz( name, superName, interfaces );
         }
 
         public void end()
@@ -222,12 +223,14 @@ public void end()
             }
         }
 
+        @Override
         public MethodVisitor visitMethod( int access, String name, String desc, String signature, String[] exceptions )
         {
             clazz.getSignatures().add( name + desc );
             return null;
         }
 
+        @Override
         public FieldVisitor visitField( int access, String name, String desc, String signature, Object value )
         {
             clazz.getSignatures().add( name + "#" + desc );

animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java

@@ -138,7 +138,7 @@ public SignatureChecker( Map<String, Clazz> classes, Set<String> ignoredPackages
     public static Map<String, Clazz> loadClasses( InputStream in ) throws IOException
     {
         Map<String, Clazz> classes = new HashMap<>();
-        try (ObjectInputStream ois = new ObjectInputStream( new GZIPInputStream( in ) ))
+        try (ObjectInputStream ois = new SignatureObjectInputStream( new GZIPInputStream( in ) ))
         {
             while ( true )
             {
@@ -573,7 +573,6 @@ private boolean shouldBeIgnored( String type, boolean ignoreError )
 
         /**
          * If the given signature is found in the specified class, return true.
-         * @param baseFind TODO
          */
         private boolean find( Clazz c , String sig )
         {

animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureMerger.java

@@ -64,7 +64,7 @@ public SignatureMerger( InputStream[] in, OutputStream out, Logger logger )
         {
             try
             {
-                ObjectInputStream ois = new ObjectInputStream( new GZIPInputStream( i ) );
+                ObjectInputStream ois = new SignatureObjectInputStream( new GZIPInputStream( i ) );
                 while ( true )
                 {
                     Clazz c = (Clazz) ois.readObject();

animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureObjectInputStream.java

@@ -0,0 +1,59 @@
+package org.codehaus.mojo.animal_sniffer;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InvalidClassException;
+import java.io.ObjectInputStream;
+import java.io.ObjectStreamClass;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * {@link ObjectInputStream} subclass which only permits loading classes which are needed
+ * by signature files. All other classes are rejected for security reasons.
+ */
+public class SignatureObjectInputStream extends ObjectInputStream {
+    private static final Set<String> ALLOWED_CLASS_NAMES = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
+        Clazz.class.getName(),
+        String[].class.getName()
+    )));
+
+    public SignatureObjectInputStream(InputStream in) throws IOException {
+        super(in);
+    }
+
+    // Impose restrictions on allowed classes, see https://wiki.sei.cmu.edu/confluence/display/java/SER12-J.+Prevent+deserialization+of+untrusted+data
+    @Override
+    protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException, ClassNotFoundException {
+        String className = desc.getName();
+
+        if (ALLOWED_CLASS_NAMES.contains(className)) {
+            return super.resolveClass(desc);
+        }
+
+        Class<?> c;
+        try {
+            // Should be safe because default implementation uses `initialize=false`, and this is guaranteed by the Javadoc
+            c = super.resolveClass(desc);
+        } catch (ClassNotFoundException classNotFoundException) {
+            // To be safe throw InvalidClassException instead because all allowed classes should exist on classpath
+            throw new InvalidClassException(className, "Class not found, probably disallowed class");
+        }
+
+        // Also allow Set classes because Clazz has field of type Set
+        if (isAllowedSetClass(c)) {
+            return c;
+        }
+
+        throw new InvalidClassException(className, "Disallowed class for signature data");
+    }
+
+    /**
+     * Check if the class is an allowed implementation of {@link Set}.
+     */
+    private static boolean isAllowedSetClass(Class<?> c) {
+        return Set.class.isAssignableFrom(c) && c.getName().startsWith("java.util.");
+    }
+}