You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ma...@apache.org on 2022/11/21 10:50:30 UTC
[commons-bcel] branch master updated: Unknown attributes with invalid length now trigger ClassFormatException
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
Re: [commons-bcel] branch master updated: Unknown attributes with invalid length now trigger ClassFormatException
Posted by "Gary D. Gregory" <gg...@apache.org>.
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 <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
> >
> >
>
---------------------------------------------------------------------
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: Unknown attributes with invalid length now trigger ClassFormatException
Posted by Gary Gregory <ga...@gmail.com>.
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
>
>