Hi Mark, Any reason not to do this check in the Attribute superclass?
Gaty On Mon, Nov 21, 2022, 05:50 <ma...@apache.org> wrote: > This is an automated email from the ASF dual-hosted git repository. > > markt pushed a commit to branch master > in repository https://gitbox.apache.org/repos/asf/commons-bcel.git > > > The following commit(s) were added to refs/heads/master by this push: > new 89015357 Unknown attributes with invalid length now trigger > ClassFormatException > 89015357 is described below > > commit 89015357effff5559d0ae3e73b25801db08d7812 > Author: Mark Thomas <ma...@apache.org> > AuthorDate: Mon Nov 21 10:50:23 2022 +0000 > > Unknown attributes with invalid length now trigger ClassFormatException > --- > src/changes/changes.xml | 1 + > src/main/java/org/apache/bcel/classfile/Unknown.java | 8 ++++++++ > src/test/java/org/apache/bcel/OssFuzzTestCase.java | 9 +++++++++ > src/test/resources/ossfuzz/issue53544a/Test.class | Bin 0 -> 49 bytes > 4 files changed, 18 insertions(+) > > diff --git a/src/changes/changes.xml b/src/changes/changes.xml > index ab28355f..502b818f 100644 > --- a/src/changes/changes.xml > +++ b/src/changes/changes.xml > @@ -98,6 +98,7 @@ The <action> type attribute can be add,update,fix,remove. > <action type="fix" dev="ggregory" due-to="Gary > Gregory">org.apache.bcel.util.ClassPath hashCode() and equals() don't > match.</action> > <action type="fix" dev="markt" > due-to="OSS-Fuzz">org.apache.bcel.classfile.StackMapType constructors now > throw ClassFormatException on invalid input.</action> > <action type="add" dev="ggregory" > due-to="nbauma109, Gary Gregory">Code coverage and bug fixes for bcelifier > #171.</action> > + <action type="fix" dev="markt" due-to="Mark > Thomas">org.apache.bcel.classfile.Unknown constructors now throw > ClassFormatException on invalid length input.</action> > <!-- UPDATE --> > <action type="update" dev="ggregory" due-to="Gary > Gregory">Bump spotbugs-maven-plugin from 4.7.2.2 to 4.7.3.0 #167.</action> > <action type="update" dev="ggregory" > due-to="Dependabot">Bump jmh.version from 1.35 to 1.36 #170.</action> > diff --git a/src/main/java/org/apache/bcel/classfile/Unknown.java > b/src/main/java/org/apache/bcel/classfile/Unknown.java > index 23cd38eb..df088430 100644 > --- a/src/main/java/org/apache/bcel/classfile/Unknown.java > +++ b/src/main/java/org/apache/bcel/classfile/Unknown.java > @@ -66,6 +66,14 @@ public final class Unknown extends Attribute { > if (length > 0) { > bytes = new byte[length]; > input.readFully(bytes); > + } else if (length < 0) { > + // Length is defined in the JVM specification as an unsigned > 4 byte > + // integer but is read as a signed integer. This is not an > issue as > + // the JRE API consistently uses byte[] or ByteBuffer for > classes. > + // Therefore treat any negative number here as a format error. > + throw new ClassFormatException(String.format( > + "Invalid length %,d for Unknown attribute. Must be > greater than or equal to zero and less than %,d", > + length & 0xFFFFFFFFL, Integer.MAX_VALUE)); > } > } > > diff --git a/src/test/java/org/apache/bcel/OssFuzzTestCase.java > b/src/test/java/org/apache/bcel/OssFuzzTestCase.java > index 9f4e63bb..8944ca4b 100644 > --- a/src/test/java/org/apache/bcel/OssFuzzTestCase.java > +++ b/src/test/java/org/apache/bcel/OssFuzzTestCase.java > @@ -47,6 +47,15 @@ public class OssFuzzTestCase { > testOssFuzzReproducer("53543"); > } > > + /* > + * The original issue 53544 was a false positive but reviewing that > issue > + * did find a valid issue nearby. > + */ > + @Test > + public void testIssue53544a() throws Exception { > + testOssFuzzReproducer("53544a"); > + } > + > private void testOssFuzzReproducer(final String issue) throws > Exception { > final File reproducerFile = new > File("target/test-classes/ossfuzz/issue" + issue + "/Test.class"); > try (final FileInputStream reproducerInputStream = new > FileInputStream(reproducerFile)) { > diff --git a/src/test/resources/ossfuzz/issue53544a/Test.class > b/src/test/resources/ossfuzz/issue53544a/Test.class > new file mode 100644 > index 00000000..5fbdd67f > Binary files /dev/null and > b/src/test/resources/ossfuzz/issue53544a/Test.class differ > >
