Re: [commons-bcel] branch master updated: Unknown attributes with invalid length now trigger ClassFormatException
Hi Mark,
Any reason not to do this check in the Attribute superclass?
Gaty
On Mon, Nov 21, 2022, 05:50 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 89015357e5559d0ae3e73b25801db08d7812
> Author: Mark Thomas
> AuthorDate: Mon Nov 21 10:50:23 2022 +
>
> 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 type attribute can be add,update,fix,remove.
>org.apache.bcel.util.ClassPath hashCode() and equals() don't
> match.
> due-to="OSS-Fuzz">org.apache.bcel.classfile.StackMapType constructors now
> throw ClassFormatException on invalid input.
> due-to="nbauma109, Gary Gregory">Code coverage and bug fixes for bcelifier
> #171.
> + org.apache.bcel.classfile.Unknown constructors now throw
> ClassFormatException on invalid length input.
>
>Bump spotbugs-maven-plugin from 4.7.2.2 to 4.7.3.0 #167.
> due-to="Dependabot">Bump jmh.version from 1.35 to 1.36 #170.
> 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 & 0xL, 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 ..5fbdd67f
> Binary files /dev/null and
> b/src/test/resources/ossfuzz/issue53544a/Test.class differ
>
>
Re: [commons-bcel] branch master updated: Unknown attributes with invalid length now trigger ClassFormatException
Hm, after reading
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7 I will
pull up the validation.
TY for the commit :-)
Gary
On 2022/11/21 12:00:20 Gary Gregory wrote:
> Hi Mark,
>
> Any reason not to do this check in the Attribute superclass?
>
> Gaty
>
> On Mon, Nov 21, 2022, 05:50 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 89015357e5559d0ae3e73b25801db08d7812
> > Author: Mark Thomas
> > AuthorDate: Mon Nov 21 10:50:23 2022 +
> >
> > 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 type attribute can be add,update,fix,remove.
> >org.apache.bcel.util.ClassPath hashCode() and equals() don't
> > match.
> > > due-to="OSS-Fuzz">org.apache.bcel.classfile.StackMapType constructors now
> > throw ClassFormatException on invalid input.
> > > due-to="nbauma109, Gary Gregory">Code coverage and bug fixes for bcelifier
> > #171.
> > + org.apache.bcel.classfile.Unknown constructors now throw
> > ClassFormatException on invalid length input.
> >
> >Bump spotbugs-maven-plugin from 4.7.2.2 to 4.7.3.0 #167.
> > > due-to="Dependabot">Bump jmh.version from 1.35 to 1.36 #170.
> > 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 & 0xL, 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 ..5fbdd67f
> > Binary files /dev/null and
> > b/src/test/resources/ossfuzz/issue53544a/Test.class differ
> >
> >
>
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-bcel] branch master updated: Validate the u4 length of all attributes
Gotcha, will fix.
Gary
On Mon, Nov 21, 2022, 08:23 Mark Thomas wrote:
> On 21/11/2022 13:03, ggreg...@apache.org wrote:
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > ggregory 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 428b65e0 Validate the u4 length of all attributes
> > 428b65e0 is described below
> >
> > commit 428b65e0d6cc0f094b428f8ab92810ad7221afe1
> > Author: Gary David Gregory (Code signing key)
> > AuthorDate: Mon Nov 21 08:03:04 2022 -0500
> >
> > Validate the u4 length of all attributes
>
> This breaks the format of the error message for four byte unsigned int
> values bigger than Integer.MAX_VALUE
>
> The "length & 0xL" snippet was there to ensure that these were
> correctly displayed as a positive (long) integer.
>
> Mark
>
>
> > ---
> > src/changes/changes.xml| 2 +-
> > .../java/org/apache/bcel/classfile/Attribute.java | 37
> ++
> > .../java/org/apache/bcel/classfile/Unknown.java| 10 +-
> > src/main/java/org/apache/bcel/util/Args.java | 28
>
> > 4 files changed, 61 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> > index 502b818f..30a26c71 100644
> > --- a/src/changes/changes.xml
> > +++ b/src/changes/changes.xml
> > @@ -98,7 +98,7 @@ The type attribute can be
> add,update,fix,remove.
> > org.apache.bcel.util.ClassPath hashCode() and equals() don't
> match.
> > due-to="OSS-Fuzz">org.apache.bcel.classfile.StackMapType constructors now
> throw ClassFormatException on invalid input.
> > due-to="nbauma109, Gary Gregory">Code coverage and bug fixes for bcelifier
> #171.
> > - org.apache.bcel.classfile.Unknown constructors now throw
> ClassFormatException on invalid length input.
> > + org.apache.bcel.classfile.Attribute constructors now
> throw ClassFormatException on invalid length input.
> >
> > due-to="Gary Gregory">Bump spotbugs-maven-plugin from 4.7.2.2 to 4.7.3.0
> #167.
> > due-to="Dependabot">Bump jmh.version from 1.35 to 1.36 #170.
> > diff --git a/src/main/java/org/apache/bcel/classfile/Attribute.java
> b/src/main/java/org/apache/bcel/classfile/Attribute.java
> > index d1d38f40..14a76cc2 100644
> > --- a/src/main/java/org/apache/bcel/classfile/Attribute.java
> > +++ b/src/main/java/org/apache/bcel/classfile/Attribute.java
> > @@ -27,9 +27,17 @@ import org.apache.bcel.Const;
> > import org.apache.bcel.util.Args;
> >
> > /**
> > - * Abstract super class for Attribute objects. Currently the
> ConstantValue, SourceFile,
> > - * Code, Exceptiontable, LineNumberTable,
> LocalVariableTable, InnerClasses
> > - * and Synthetic attributes are supported. The
> Unknown attribute stands for non-standard-attributes.
> > + * Abstract super class for Attribute objects. Currently the
> ConstantValue, SourceFile, Code,
> Exceptiontable,
> > + * LineNumberTable, LocalVariableTable,
> InnerClasses and Synthetic attributes are supported. The
> Unknown attribute
> > + * stands for non-standard-attributes.
> > + *
> > + *
> > + * attribute_info {
> > + * u2 attribute_name_index;
> > + * u4 attribute_length;
> > + * u1 info[attribute_length];
> > + * }
> > + *
> >*
> >* @see ConstantValue
> >* @see SourceFile
> > @@ -238,10 +246,26 @@ public abstract class Attribute implements
> Cloneable, Node {
> > @java.lang.Deprecated
> > protected ConstantPool constant_pool; // TODO make private (has
> getter & setter)
> >
> > +/**
> > + * Constructs an instance.
> > + *
> > + *
> > + * attribute_info {
> > + * u2 attribute_name_index;
> > + * u4 attribute_length;
> > + * u1 info[attribute_length];
> > + * }
> > + *
> > + *
> > + * @param tag tag.
> > + * @param nameIndex u2 name index.
> > + * @param length u4 length.
> > + * @param constantPool constant pool.
> > + */
> > protected Attribute(final byte tag, final int nameIndex, final int
> length, final ConstantPool constantPool) {
> > this.tag = tag;
> > this.name_index = Args.requireU2(nameIndex, 0,
> constantPool.getLength(), getClass().getSimpleName() + " name index");
> > -this.length = length;
> > +this.length = Args.requireU4(length, getClass().getSimpleName()
> + " attribute length");
> > this.constant_pool = constantPool;
> > }
> >
> > @@ -271,12 +295,13 @@ public abstract class Attribute implements
> Cloneable, Node {
> > }
> >
> > /**
> > - * @return deep copy of this attribute
> > + * @param constantPool constant pool to save.
> > + * @return deep copy of this attribute.
> >*/
> > public abstract Attribute copy(ConstantPool constantPool);
> >
> >
