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, 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) <ggreg...@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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org