You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2022/11/21 13:03:09 UTC
[commons-bcel] branch master updated: Validate the u4 length of all attributes
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
---
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
Re: [commons-bcel] branch master updated: Validate the u4 length of all attributes
Posted by Gary Gregory <ga...@gmail.com>.
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 Mark Thomas <ma...@apache.org>.
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;
> + }
> }
>