You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gary Gregory <ga...@gmail.com> on 2022/11/21 14:04:38 UTC
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 <ma...@apache.org> wrote:
> On 21/11/2022 13:03, ggregory@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) <gg...@apache.org>
> > 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 & 0xFFFFFFFFL" 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 <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>
> > + <action type="fix" dev="markt" due-to="Mark
> Thomas, Gary Gregory">org.apache.bcel.classfile.Attribute 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/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 <em>Attribute</em> objects. Currently the
> <em>ConstantValue</em>, <em>SourceFile</em>,
> > - * <em>Code</em>, <em>Exceptiontable</em>, <em>LineNumberTable</em>,
> <em>LocalVariableTable</em>, <em>InnerClasses</em>
> > - * and <em>Synthetic</em> attributes are supported. The
> <em>Unknown</em> attribute stands for non-standard-attributes.
> > + * Abstract super class for <em>Attribute</em> objects. Currently the
> <em>ConstantValue</em>, <em>SourceFile</em>, <em>Code</em>,
> <em>Exceptiontable</em>,
> > + * <em>LineNumberTable</em>, <em>LocalVariableTable</em>,
> <em>InnerClasses</em> and <em>Synthetic</em> attributes are supported. The
> <em>Unknown</em> attribute
> > + * stands for non-standard-attributes.
> > + *
> > + * <pre>
> > + * attribute_info {
> > + * u2 attribute_name_index;
> > + * u4 attribute_length;
> > + * u1 info[attribute_length];
> > + * }
> > + * </pre>
> > *
> > * @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.
> > + *
> > + * <pre>
> > + * attribute_info {
> > + * u2 attribute_name_index;
> > + * u4 attribute_length;
> > + * u1 info[attribute_length];
> > + * }
> > + * </pre>
> > + *
> > + * @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);
> >
> > /**
> > - * Dump attribute to file stream in binary format.
> > + * Dumps attribute to file stream in binary format.
> > *
> > * @param file Output file stream
> > * @throws IOException if an I/O error occurs.
> > diff --git a/src/main/java/org/apache/bcel/classfile/Unknown.java
> b/src/main/java/org/apache/bcel/classfile/Unknown.java
> > index df088430..6f72dc24 100644
> > --- a/src/main/java/org/apache/bcel/classfile/Unknown.java
> > +++ b/src/main/java/org/apache/bcel/classfile/Unknown.java
> > @@ -49,7 +49,7 @@ public final class Unknown extends Attribute {
> > public Unknown(final int nameIndex, final int length, final byte[]
> bytes, final ConstantPool constantPool) {
> > super(Const.ATTR_UNKNOWN, nameIndex, length, constantPool);
> > this.bytes = bytes;
> > - name = constantPool.getConstantUtf8(nameIndex).getBytes();
> > + this.name = constantPool.getConstantUtf8(nameIndex).getBytes();
> > }
> >
> > /**
> > @@ -66,14 +66,6 @@ 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/main/java/org/apache/bcel/util/Args.java
> b/src/main/java/org/apache/bcel/util/Args.java
> > index defc071f..642f6508 100644
> > --- a/src/main/java/org/apache/bcel/util/Args.java
> > +++ b/src/main/java/org/apache/bcel/util/Args.java
> > @@ -53,6 +53,20 @@ public class Args {
> > return require(value, 0, message);
> > }
> >
> > + /**
> > + * Requires a u1 value.
> > + *
> > + * @param value The value to test.
> > + * @param message The message prefix
> > + * @return The value to test.
> > + */
> > + public static int requireU1(final int value, final String message) {
> > + if (value < 0 || value > Const.MAX_BYTE) {
> > + throw new ClassFormatException(String.format("%s [Value out
> of range (0 - %,d) for type u1: %,d]", message, Const.MAX_BYTE, value));
> > + }
> > + return value;
> > + }
> > +
> > /**
> > * Requires a u2 value of at least {@code min} and not above
> {@code max}.
> > *
> > @@ -97,4 +111,18 @@ public class Args {
> > public static int requireU2(final int value, final String message)
> {
> > return requireU2(value, 0, message);
> > }
> > +
> > + /**
> > + * Requires a u4 value.
> > + *
> > + * @param value The value to test.
> > + * @param message The message prefix
> > + * @return The value to test.
> > + */
> > + public static int requireU4(final int value, final String message) {
> > + if (value < 0) {
> > + throw new ClassFormatException(String.format("%s [Value out
> of range (0 - %,d) for type u4: %,d]", message, Integer.MAX_VALUE, value));
> > + }
> > + return value;
> > + }
> > }
> >
>
Re: [commons-bcel] branch master updated: Validate the u4 length of all attributes
Posted by Gary Gregory <ga...@gmail.com>.
Makes sense I think, thank you Mark.
Gary
On Tue, Nov 22, 2022, 08:41 Mark Thomas <ma...@apache.org> wrote:
> On 22/11/2022 13:10, Gary D. Gregory wrote:
> > I am concerned that the recent fixes we've made through OSS fuzz and
> code inspection to validate input are semantically incorrect: The verifier
> should catch these errors, not the construction of Java objects. This could
> be a case where fuzzing and low-level code inspections only appear to find
> issues but are ignorant of the usage context.
> >
> > Thoughts?
>
> My understanding of the Javadocs was that these changes are consistent
> with the documented behaviour.
>
> ClassParser.parse() throws ClassFormatException if the class file is
> malformed. I think all the recent changes come under this heading.
>
> Verification is (mostly) concerned with the byte code in Code
> attributes. Those are opaue to the parser.
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [commons-bcel] branch master updated: Validate the u4 length of all attributes
Posted by Mark Thomas <ma...@apache.org>.
On 22/11/2022 13:10, Gary D. Gregory wrote:
> I am concerned that the recent fixes we've made through OSS fuzz and code inspection to validate input are semantically incorrect: The verifier should catch these errors, not the construction of Java objects. This could be a case where fuzzing and low-level code inspections only appear to find issues but are ignorant of the usage context.
>
> Thoughts?
My understanding of the Javadocs was that these changes are consistent
with the documented behaviour.
ClassParser.parse() throws ClassFormatException if the class file is
malformed. I think all the recent changes come under this heading.
Verification is (mostly) concerned with the byte code in Code
attributes. Those are opaue to the parser.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [commons-bcel] branch master updated: Validate the u4 length of all attributes
Posted by "Gary D. Gregory" <gg...@apache.org>.
I am concerned that the recent fixes we've made through OSS fuzz and code inspection to validate input are semantically incorrect: The verifier should catch these errors, not the construction of Java objects. This could be a case where fuzzing and low-level code inspections only appear to find issues but are ignorant of the usage context.
Thoughts?
Gary
On 2022/11/21 14:04:38 Gary Gregory wrote:
> Gotcha, will fix.
>
> Gary
>
> On Mon, Nov 21, 2022, 08:23 Mark Thomas <ma...@apache.org> wrote:
>
> > On 21/11/2022 13:03, ggregory@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) <gg...@apache.org>
> > > 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 & 0xFFFFFFFFL" 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 <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>
> > > + <action type="fix" dev="markt" due-to="Mark
> > Thomas, Gary Gregory">org.apache.bcel.classfile.Attribute 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/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 <em>Attribute</em> objects. Currently the
> > <em>ConstantValue</em>, <em>SourceFile</em>,
> > > - * <em>Code</em>, <em>Exceptiontable</em>, <em>LineNumberTable</em>,
> > <em>LocalVariableTable</em>, <em>InnerClasses</em>
> > > - * and <em>Synthetic</em> attributes are supported. The
> > <em>Unknown</em> attribute stands for non-standard-attributes.
> > > + * Abstract super class for <em>Attribute</em> objects. Currently the
> > <em>ConstantValue</em>, <em>SourceFile</em>, <em>Code</em>,
> > <em>Exceptiontable</em>,
> > > + * <em>LineNumberTable</em>, <em>LocalVariableTable</em>,
> > <em>InnerClasses</em> and <em>Synthetic</em> attributes are supported. The
> > <em>Unknown</em> attribute
> > > + * stands for non-standard-attributes.
> > > + *
> > > + * <pre>
> > > + * attribute_info {
> > > + * u2 attribute_name_index;
> > > + * u4 attribute_length;
> > > + * u1 info[attribute_length];
> > > + * }
> > > + * </pre>
> > > *
> > > * @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.
> > > + *
> > > + * <pre>
> > > + * attribute_info {
> > > + * u2 attribute_name_index;
> > > + * u4 attribute_length;
> > > + * u1 info[attribute_length];
> > > + * }
> > > + * </pre>
> > > + *
> > > + * @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);
> > >
> > > /**
> > > - * Dump attribute to file stream in binary format.
> > > + * Dumps attribute to file stream in binary format.
> > > *
> > > * @param file Output file stream
> > > * @throws IOException if an I/O error occurs.
> > > diff --git a/src/main/java/org/apache/bcel/classfile/Unknown.java
> > b/src/main/java/org/apache/bcel/classfile/Unknown.java
> > > index df088430..6f72dc24 100644
> > > --- a/src/main/java/org/apache/bcel/classfile/Unknown.java
> > > +++ b/src/main/java/org/apache/bcel/classfile/Unknown.java
> > > @@ -49,7 +49,7 @@ public final class Unknown extends Attribute {
> > > public Unknown(final int nameIndex, final int length, final byte[]
> > bytes, final ConstantPool constantPool) {
> > > super(Const.ATTR_UNKNOWN, nameIndex, length, constantPool);
> > > this.bytes = bytes;
> > > - name = constantPool.getConstantUtf8(nameIndex).getBytes();
> > > + this.name = constantPool.getConstantUtf8(nameIndex).getBytes();
> > > }
> > >
> > > /**
> > > @@ -66,14 +66,6 @@ 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/main/java/org/apache/bcel/util/Args.java
> > b/src/main/java/org/apache/bcel/util/Args.java
> > > index defc071f..642f6508 100644
> > > --- a/src/main/java/org/apache/bcel/util/Args.java
> > > +++ b/src/main/java/org/apache/bcel/util/Args.java
> > > @@ -53,6 +53,20 @@ public class Args {
> > > return require(value, 0, message);
> > > }
> > >
> > > + /**
> > > + * Requires a u1 value.
> > > + *
> > > + * @param value The value to test.
> > > + * @param message The message prefix
> > > + * @return The value to test.
> > > + */
> > > + public static int requireU1(final int value, final String message) {
> > > + if (value < 0 || value > Const.MAX_BYTE) {
> > > + throw new ClassFormatException(String.format("%s [Value out
> > of range (0 - %,d) for type u1: %,d]", message, Const.MAX_BYTE, value));
> > > + }
> > > + return value;
> > > + }
> > > +
> > > /**
> > > * Requires a u2 value of at least {@code min} and not above
> > {@code max}.
> > > *
> > > @@ -97,4 +111,18 @@ public class Args {
> > > public static int requireU2(final int value, final String message)
> > {
> > > return requireU2(value, 0, message);
> > > }
> > > +
> > > + /**
> > > + * Requires a u4 value.
> > > + *
> > > + * @param value The value to test.
> > > + * @param message The message prefix
> > > + * @return The value to test.
> > > + */
> > > + public static int requireU4(final int value, final String message) {
> > > + if (value < 0) {
> > > + throw new ClassFormatException(String.format("%s [Value out
> > of range (0 - %,d) for type u4: %,d]", message, Integer.MAX_VALUE, value));
> > > + }
> > > + return value;
> > > + }
> > > }
> > >
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org