This is an automated email from the ASF dual-hosted git repository.
garydgregory 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 49a8bcb6 Opcodes tableswitch and lookupswitch add boundary checks
(f003).
49a8bcb6 is described below
commit 49a8bcb631c1d0858813b12e0a2c94d8adee66d8
Author: Gary Gregory <[email protected]>
AuthorDate: Fri Sep 4 15:35:11 2026 -0400
Opcodes tableswitch and lookupswitch add boundary checks (f003).
---
src/changes/changes.xml | 1 +
.../java/org/apache/bcel/generic/LOOKUPSWITCH.java | 6 ++
.../java/org/apache/bcel/generic/TABLESWITCH.java | 11 ++-
.../apache/bcel/generic/SwitchAllocationTest.java | 93 ++++++++++++++++++++++
4 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 81430405..e76b7f18 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -90,6 +90,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="Gary
Gregory">Fix SpotBugs USO_UNSAFE_METHOD_SYNCHRONIZATION in
ConstantUtf8.</action>
<action type="fix" dev="ggregory" due-to="Gary
Gregory">Nested Code/Record attributes drive unbounded parse-time recursion in
ClassParser (f001).</action>
<action type="fix" dev="ggregory" due-to="Gary
Gregory">Nested annotation element values recurse unboundedly;
MAX_ARRAY_DIMENSIONS cap bypassed (f002).</action>
+ <action type="fix" dev="ggregory" due-to="Gary
Gregory">Opcodes tableswitch and lookupswitch add boundary checks
(f003).</action>
<!-- ADD -->
<action type="add" dev="ggregory" due-to="nbauma109,
Gary Gregory">Add support for permitted subclasses #493.</action>
<action type="add" dev="ggregory" due-to="nbauma109,
Gary Gregory">Add RecordComponentInfo.getAttribute(byte tag)#494.</action>
diff --git a/src/main/java/org/apache/bcel/generic/LOOKUPSWITCH.java
b/src/main/java/org/apache/bcel/generic/LOOKUPSWITCH.java
index 602b4a06..67e7a20c 100644
--- a/src/main/java/org/apache/bcel/generic/LOOKUPSWITCH.java
+++ b/src/main/java/org/apache/bcel/generic/LOOKUPSWITCH.java
@@ -21,6 +21,7 @@ package org.apache.bcel.generic;
import java.io.DataOutputStream;
import java.io.IOException;
+import org.apache.bcel.classfile.ClassFormatException;
import org.apache.bcel.util.ByteSequence;
/**
@@ -89,6 +90,11 @@ public class LOOKUPSWITCH extends Select {
protected void initFromFile(final ByteSequence bytes, final boolean wide)
throws IOException {
super.initFromFile(bytes, wide); // reads padding
final int matchLength = bytes.readInt();
+ // Require the match table to actually fit into the remaining code
bytes (8 bytes per match-offset pair). The npairs field is attacker-controlled
in
+ // a malicious class file and could otherwise request a multi-gigabyte
allocation, or a negative array size, before a single pair is read.
+ if (matchLength < 0 || matchLength > bytes.available() / 8) {
+ throw new ClassFormatException("Invalid lookupswitch: npairs=" +
matchLength + ", but only " + bytes.available() + " bytes of code remain.");
+ }
setMatchLength(matchLength);
final short fixedLength = (short) (9 + matchLength * 8);
setFixedLength(fixedLength);
diff --git a/src/main/java/org/apache/bcel/generic/TABLESWITCH.java
b/src/main/java/org/apache/bcel/generic/TABLESWITCH.java
index b43742f8..736fbc68 100644
--- a/src/main/java/org/apache/bcel/generic/TABLESWITCH.java
+++ b/src/main/java/org/apache/bcel/generic/TABLESWITCH.java
@@ -21,6 +21,7 @@ package org.apache.bcel.generic;
import java.io.DataOutputStream;
import java.io.IOException;
+import org.apache.bcel.classfile.ClassFormatException;
import org.apache.bcel.util.ByteSequence;
/**
@@ -92,7 +93,15 @@ public class TABLESWITCH extends Select {
super.initFromFile(bytes, wide);
final int low = bytes.readInt();
final int high = bytes.readInt();
- final int matchLength = high - low + 1;
+ // Compute in long arithmetic to guard against integer overflow, and
require the match table to actually fit into the remaining code bytes (4 bytes
+ // per jump offset). The low and high fields are attacker-controlled
in a malicious class file and could otherwise request a multi-gigabyte
+ // allocation, or a negative array size, before a single table entry
is read.
+ final long matchLengthLong = (long) high - low + 1;
+ if (matchLengthLong < 0 || matchLengthLong > bytes.available() / 4) {
+ throw new ClassFormatException(
+ "Invalid tableswitch: low=" + low + ", high=" + high + ",
but only " + bytes.available() + " bytes of code remain.");
+ }
+ final int matchLength = (int) matchLengthLong;
setMatchLength(matchLength);
final short fixedLength = (short) (13 + matchLength * 4);
setFixedLength(fixedLength);
diff --git a/src/test/java/org/apache/bcel/generic/SwitchAllocationTest.java
b/src/test/java/org/apache/bcel/generic/SwitchAllocationTest.java
new file mode 100644
index 00000000..3ff7fd4d
--- /dev/null
+++ b/src/test/java/org/apache/bcel/generic/SwitchAllocationTest.java
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bcel.generic;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.ClassFormatException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that {@link TABLESWITCH} and {@link LOOKUPSWITCH} validate their
match table counts against the remaining code bytes instead of allocating
whatever
+ * amount of memory the raw count fields request.
+ */
+class SwitchAllocationTest {
+
+ private static byte[] switchBytes(final short opcode, final int...
operands) throws IOException {
+ final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (DataOutputStream dos = new DataOutputStream(baos)) {
+ dos.writeByte(opcode);
+ for (int i = 0; i < 3; i++) {
+ dos.writeByte(0); // padding up to the next 4-byte boundary
+ }
+ for (final int operand : operands) {
+ dos.writeInt(operand);
+ }
+ }
+ return baos.toByteArray();
+ }
+
+ @Test
+ void testLookupswitchHugeNpairsRejected() throws IOException {
+ // default = 0, npairs = Integer.MAX_VALUE, no pairs present.
+ final byte[] code = switchBytes(Const.LOOKUPSWITCH, 0,
Integer.MAX_VALUE);
+ assertThrows(ClassFormatException.class, () -> new
InstructionList(code));
+ }
+
+ @Test
+ void testLookupswitchNegativeNpairsRejected() throws IOException {
+ final byte[] code = switchBytes(Const.LOOKUPSWITCH, 0, -1);
+ assertThrows(ClassFormatException.class, () -> new
InstructionList(code));
+ }
+
+ @Test
+ void testLookupswitchValidAccepted() throws IOException {
+ // default = 0, npairs = 1, one (match, offset) pair branching back to
the instruction itself.
+ final byte[] code = switchBytes(Const.LOOKUPSWITCH, 0, 1, 5, 0);
+ assertEquals(1, new InstructionList(code).getLength());
+ }
+
+ @Test
+ void testTableswitchHugeRangeRejected() throws IOException {
+ // default = 0, low = 0, high = Integer.MAX_VALUE - 1: requests
roughly 8 GB across the three tables.
+ final byte[] code = switchBytes(Const.TABLESWITCH, 0, 0,
Integer.MAX_VALUE - 1);
+ assertThrows(ClassFormatException.class, () -> new
InstructionList(code));
+ }
+
+ @Test
+ void testTableswitchOverflowingRangeRejected() throws IOException {
+ // high - low + 1 overflows int arithmetic to a negative value.
+ final byte[] code = switchBytes(Const.TABLESWITCH, 0,
Integer.MIN_VALUE, Integer.MAX_VALUE);
+ assertThrows(ClassFormatException.class, () -> new
InstructionList(code));
+ }
+
+ @Test
+ void testTableswitchValidAccepted() throws IOException {
+ // default = 0, low = 0, high = 1, two jump offsets branching back to
the instruction itself.
+ final byte[] code = switchBytes(Const.TABLESWITCH, 0, 0, 1, 0, 0);
+ assertEquals(1, new InstructionList(code).getLength());
+ }
+}
