diff --git a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java index ba83856c..2a50e51f 100644 --- a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java +++ b/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 diff --git a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Clazz.java b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Clazz.java index 6216f3fb..d879b94b 100644 --- a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Clazz.java +++ b/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 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 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 superInterfaces = new HashSet<>(); + Set 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 signatures = new HashSet<>(); + Set 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 getSignatures() { return signatures; diff --git a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureBuilder.java b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureBuilder.java index 5518c801..998ffdb7 100644 --- a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureBuilder.java +++ b/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 ); diff --git a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java index e9d8468e..2a6b674d 100644 --- a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java +++ b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java @@ -138,7 +138,7 @@ public SignatureChecker( Map classes, Set ignoredPackages public static Map loadClasses( InputStream in ) throws IOException { Map 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 ) { diff --git a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureMerger.java b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureMerger.java index 69602cf9..dd6ae082 100644 --- a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureMerger.java +++ b/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(); diff --git a/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureObjectInputStream.java b/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureObjectInputStream.java new file mode 100644 index 00000000..3cc87eab --- /dev/null +++ b/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 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."); + } +} diff --git a/animal-sniffer/src/test/java/org/codehaus/mojo/animal_sniffer/SignatureCheckerTest.java b/animal-sniffer/src/test/java/org/codehaus/mojo/animal_sniffer/SignatureCheckerTest.java index 0b914ae0..8673e308 100644 --- a/animal-sniffer/src/test/java/org/codehaus/mojo/animal_sniffer/SignatureCheckerTest.java +++ b/animal-sniffer/src/test/java/org/codehaus/mojo/animal_sniffer/SignatureCheckerTest.java @@ -24,6 +24,21 @@ package org.codehaus.mojo.animal_sniffer; +import static org.junit.Assert.assertArrayEquals; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.InvalidClassException; +import java.io.ObjectOutputStream; +import java.io.Serializable; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Collections; +import java.util.Map; +import java.util.zip.GZIPOutputStream; + +import org.codehaus.mojo.animal_sniffer.logging.Logger; + import junit.framework.TestCase; public class SignatureCheckerTest extends TestCase @@ -75,4 +90,178 @@ private static void assertAnnotationDescriptor( String expected, String fqn ) assertEquals( expected, SignatureChecker.toAnnotationDescriptor( fqn ) ); } + public void testLoadClasses() throws Exception + { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + try (ObjectOutputStream objOut = new ObjectOutputStream(new GZIPOutputStream(out))) { + objOut.writeObject(new Clazz("my/Class1", Collections.singleton("field1#Ljava/lang/String;"), "java/lang/Object", new String[] {"my/SuperInterface"})); + objOut.writeObject(new Clazz("my/Class2", Collections.emptySet(), "my/SuperClass", new String[0])); + objOut.writeObject(null); + } + + Map classes = SignatureChecker.loadClasses(new ByteArrayInputStream(out.toByteArray())); + assertEquals(2, classes.size()); + + Clazz class1 = classes.get("my/Class1"); + assertEquals("my/Class1", class1.getName()); + assertEquals(Collections.singleton("field1#Ljava/lang/String;"), class1.getSignatures()); + assertEquals("java/lang/Object", class1.getSuperClass()); + assertArrayEquals(new String[] {"my/SuperInterface"}, class1.getSuperInterfaces()); + + Clazz class2 = classes.get("my/Class2"); + assertEquals("my/Class2", class2.getName()); + assertEquals(Collections.emptySet(), class2.getSignatures()); + assertEquals("my/SuperClass", class2.getSuperClass()); + assertArrayEquals(new String[0], class2.getSuperInterfaces()); + } + + /** + * Verifies that only certain allowed classes may be deserialized. + */ + public void testLoadClasses_DisallowedClass() throws Exception + { + // Java Serialization data for an instance of DisallowedDummyClass followed by `null` + byte[] serializationData = { + 31, -117, 8, 0, 0, 0, 0, 0, 0, -1, 37, -63, 65, 14, 64, 48, 16, 0, -64, 37, + -15, 19, -25, 126, -126, -109, 43, 119, -39, -80, -91, -76, -35, 102, 87, -125, 63, -7, -102, 63, + 56, -104, 121, 94, -88, 84, -96, 99, 89, -52, -60, 51, -83, -104, -43, 4, -34, -40, 96, 116, + 1, -3, -88, -47, 89, 75, 98, 122, -73, 68, 60, -78, 80, -77, -46, -76, -109, 12, -92, 71, + -35, 58, 69, -17, -7, -92, -71, -51, 33, -36, -115, 71, 85, -8, 21, 37, -64, -107, -46, 7, + -126, -50, 87, -21, 96, 0, 0, 0, + }; + + try + { + SignatureChecker.loadClasses(new ByteArrayInputStream(serializationData)); + fail(); + } + catch (InvalidClassException e) { + assertEquals(DisallowedDummyClass.class.getName(), e.classname); + assertEquals(DisallowedDummyClass.class.getName() + "; Disallowed class for signature data", e.getMessage()); + } + + + // Java Serialization data for an instance of a non-existent class followed by `null` + byte[] missingClassSerializationData = { + 31, -117, 8, 0, 0, 0, 0, 0, 0, -1, 37, -63, -53, 17, 64, 64, 12, 0, -48, 48, + -93, 19, -25, 20, 97, 75, -64, -39, 100, 8, -69, 62, -119, 73, 24, -118, -46, -102, 30, 28, + -68, -9, -68, 80, -72, 65, -91, 54, 97, -81, 3, 71, 58, 29, 55, -99, 21, 73, -46, 70, + 107, -25, -110, -58, -111, 13, -21, 52, 9, 29, -89, 113, -120, -36, 47, 108, 13, -5, 81, -74, + -78, -120, 94, 18, 86, 114, -121, 95, -106, 3, -36, -5, -2, 1, -60, 22, -5, 99, 88, 0, + 0, 0, + }; + + try + { + SignatureChecker.loadClasses(new ByteArrayInputStream(missingClassSerializationData)); + fail(); + } + catch (InvalidClassException e) { + String className = "org.codehaus.mojo.animal_sniffer.SignatureCheckerTest$UnknownClass"; + assertEquals(className, e.classname); + assertEquals(className + "; Class not found, probably disallowed class", e.getMessage()); + } + } + + private static class DisallowedDummyClass implements Serializable + { + private static final long serialVersionUID = 1L; + + static { + // Verify that class is not initialized by test during deserialization + fail("Must not be initialized"); + } + } + + /** + * Uses first {@link SignatureBuilder} to build signature data and then {@link SignatureChecker} + * to load it. + */ + public void testLoadClasses_Roundtrip() throws Exception + { + /* + * Bytes for this class: + * ``` + * package mypackage; + * + * interface MyClass { + * int i = 1; + * } + * ``` + */ + byte[] classBytes = { + -54, -2, -70, -66, 0, 0, 0, 61, 0, 11, 7, 0, 2, 1, 0, 17, 109, 121, 112, 97, + 99, 107, 97, 103, 101, 47, 77, 121, 67, 108, 97, 115, 115, 7, 0, 4, 1, 0, 16, 106, + 97, 118, 97, 47, 108, 97, 110, 103, 47, 79, 98, 106, 101, 99, 116, 1, 0, 1, 105, 1, + 0, 1, 73, 1, 0, 13, 67, 111, 110, 115, 116, 97, 110, 116, 86, 97, 108, 117, 101, 3, + 0, 0, 0, 1, 1, 0, 10, 83, 111, 117, 114, 99, 101, 70, 105, 108, 101, 1, 0, 12, + 77, 121, 67, 108, 97, 115, 115, 46, 106, 97, 118, 97, 6, 0, 0, 1, 0, 3, 0, 0, + 0, 1, 0, 25, 0, 5, 0, 6, 0, 1, 0, 7, 0, 0, 0, 2, 0, 8, 0, 0, + 0, 1, 0, 9, 0, 0, 0, 2, 0, 10 + }; + Path tempFile = Files.createTempFile("animal-sniffer-test-class", ".class"); + Files.write(tempFile, classBytes); + + ByteArrayOutputStream out = new ByteArrayOutputStream(); + SignatureBuilder signatureBuilder = new SignatureBuilder(out, new TestLogger()); + // Use process(File) here because process(Path) does not support single class file at the moment + signatureBuilder.process(tempFile.toFile()); + signatureBuilder.close(); + + Files.delete(tempFile); + + Map classes = SignatureChecker.loadClasses(new ByteArrayInputStream(out.toByteArray())); + assertEquals(1, classes.size()); + + Clazz class1 = classes.get("mypackage/MyClass"); + assertEquals("mypackage/MyClass", class1.getName()); + assertEquals(Collections.singleton("i#I"), class1.getSignatures()); + assertEquals("java/lang/Object", class1.getSuperClass()); + assertArrayEquals(new String[0], class1.getSuperInterfaces()); + } + + static class TestLogger implements Logger + { + @Override + public void debug(String message) { + } + + @Override + public void debug(String message, Throwable t) { + } + + @Override + public void info(String message) { + } + + @Override + public void info(String message, Throwable t) { + } + + @Override + public void warn(String message) { + fail("Unexpected warning: " + message); + } + + @Override + public void warn(String message, Throwable t) { + if (t != null) { + t.printStackTrace(); + } + fail("Unexpected warning: " + message); + } + + @Override + public void error(String message) { + fail("Unexpected error: " + message); + } + + @Override + public void error(String message, Throwable t) { + if (t != null) { + t.printStackTrace(); + } + fail("Unexpected error: " + message); + } + } }