This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 6eaa4e36b6efeeb343d609c1f261e4793737812f Author: opencode <[email protected]> AuthorDate: Wed Oct 14 10:38:25 2026 +0200 Fix DER INTEGER encoding in Asn1Writer.writeInteger() The byte emission loop right-shifted the value on each iteration while also shifting by 8 * (valueSize - 1) of the already shifted value, so every content byte received a copy of the most significant byte: writeInteger(0x1234) produced 02 02 12 12 instead of 02 02 12 34 and writeInteger(256) produced 02 02 01 01 instead of 02 02 01 00. Only single byte values and values whose bytes are all equal were encoded correctly. Emit the bytes by shifting the unmodified value. Also add the leading zero byte that DER (X.6.90) requires for positive INTEGER values whose most significant content bit is set. Without it, writeInteger(128) produced 02 01 80 which decodes as -128. While testing the fix, two related infinite loops were found and fixed. The value size calculation loops in both writeInteger() and writeTag() test (value >> (size * 8)) > 0 without an upper bound. Since the Java shift distance is evaluated modulo 32, once the size counter reaches 4 the shift distance wraps back to 0 and the loop never terminates: writeInteger() spun forever for values >= 0x1000000 and writeTag() for data lengths >= 0x1000000. The size counters are now bounded (4 bytes is the maximum for any non-negative int value or any Java array length) and the shifts are unsigned. Adds TestAsn1Writer with explicit expected encodings, an exhaustive round-trip of the values 0 to 65535 plus transition boundaries and fixed-seed random values, comparing the content octets against BigInteger.toByteArray() (canonical minimal DER) and decoding the result with Asn1Parser, and a regression test for the 16 MiB length case in writeTag(). Output for the only in-repo callers, PEMFile with values 0 and 1, is byte-identical to before; TestPEMFile passes. --- java/org/apache/tomcat/util/buf/Asn1Writer.java | 24 ++-- .../org/apache/tomcat/util/buf/TestAsn1Writer.java | 159 +++++++++++++++++++++ webapps/docs/changelog.xml | 9 ++ 3 files changed, 183 insertions(+), 9 deletions(-) diff --git a/java/org/apache/tomcat/util/buf/Asn1Writer.java b/java/org/apache/tomcat/util/buf/Asn1Writer.java index e200466cc2..602ddbaac5 100644 --- a/java/org/apache/tomcat/util/buf/Asn1Writer.java +++ b/java/org/apache/tomcat/util/buf/Asn1Writer.java @@ -66,18 +66,22 @@ public class Asn1Writer { } // How many bytes required to write the value? No more than 4 for int. + // Note the limit is required to terminate the loop since the shift + // distance is evaluated modulo 32. int valueSize = 1; - while ((value >> (valueSize * 8)) > 0) { + while (valueSize < 4 && (value >>> (valueSize * 8)) != 0) { valueSize++; } - byte[] valueBytes = new byte[valueSize]; - int i = 0; - while (valueSize > 0) { - valueBytes[i] = (byte) (value >> (8 * (valueSize - 1))); - value = value >> 8; - valueSize--; - i++; + /* + * DER INTEGER content octets are a two's complement value. A positive value whose most significant bit is set + * requires a leading zero byte so that it is not decoded as a negative number. + */ + boolean pad = ((value >>> ((valueSize - 1) * 8)) & 0x80) != 0; + byte[] valueBytes = new byte[pad ? valueSize + 1 : valueSize]; + int offset = pad ? 1 : 0; + for (int i = 0; i < valueSize; i++) { + valueBytes[offset + i] = (byte) (value >> (8 * (valueSize - 1 - i))); } return writeTag((byte) 0x02, valueBytes); @@ -108,9 +112,11 @@ public class Asn1Writer { // 1 byte we have is now used to record how many bytes we need to // record a length > 127 // Result is lengthSize = 1 + number of bytes to record length + // Note the limit is required to terminate the loop since the shift + // distance is evaluated modulo 32. do { lengthSize++; - } while ((dataSize >> (lengthSize * 8)) > 0); + } while (lengthSize < 4 && (dataSize >>> (lengthSize * 8)) > 0); } // 1 for tag + lengthSize + dataSize diff --git a/test/org/apache/tomcat/util/buf/TestAsn1Writer.java b/test/org/apache/tomcat/util/buf/TestAsn1Writer.java new file mode 100644 index 0000000000..517ce39462 --- /dev/null +++ b/test/org/apache/tomcat/util/buf/TestAsn1Writer.java @@ -0,0 +1,159 @@ +/* + * 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 + * + * http://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.tomcat.util.buf; + +import java.math.BigInteger; +import java.util.Arrays; +import java.util.Random; + +import org.junit.Assert; +import org.junit.Test; + +/** + * Test cases for {@link Asn1Writer}. + */ +public class TestAsn1Writer { + + @Test + public void testWriteInteger0() { + // 02 01 00 + assertIntegerEncoding(0, new byte[] { 0x02, 0x01, 0x00 }); + } + + + @Test + public void testWriteIntegerSingleByte() { + // 127 -> 02 01 7F + assertIntegerEncoding(127, new byte[] { 0x02, 0x01, 0x7F }); + } + + + @Test + public void testWriteIntegerHighBitSetNeedsPadding() { + // 128 -> 02 02 00 80 (a leading zero byte is needed so the value is + // not decoded as -128) + assertIntegerEncoding(128, new byte[] { 0x02, 0x02, 0x00, (byte) 0x80 }); + } + + + @Test + public void testWriteIntegerTwoBytes() { + // 0x1234 -> 02 02 12 34 + assertIntegerEncoding(0x1234, new byte[] { 0x02, 0x02, 0x12, 0x34 }); + } + + + @Test + public void testWriteInteger256() { + // 256 -> 02 02 01 00 + assertIntegerEncoding(256, new byte[] { 0x02, 0x02, 0x01, 0x00 }); + } + + + @Test + public void testWriteIntegerThreeBytes() { + // 0x123456 -> 02 03 12 34 56 + assertIntegerEncoding(0x123456, new byte[] { 0x02, 0x03, 0x12, 0x34, 0x56 }); + } + + + @Test + public void testWriteIntegerFourBytes() { + // Integer.MAX_VALUE -> 02 04 7F FF FF FF + assertIntegerEncoding(Integer.MAX_VALUE, new byte[] { 0x02, 0x04, 0x7F, (byte) 0xFF, (byte) 0xFF, + (byte) 0xFF }); + } + + + @Test + public void testWriteIntegerRoundTrip() { + // Every value that encodes in one or two bytes + for (int i = 0; i <= 0xFFFF; i++) { + assertIntegerRoundTrip(i); + } + // Boundary values around the three and four byte transitions + assertIntegerRoundTrip(0x10000); + assertIntegerRoundTrip(0xFFFFFF); + assertIntegerRoundTrip(0x1000000); + assertIntegerRoundTrip(0x7FFFFFFF); + // Random values (fixed seed to keep the test reproducible) + Random random = new Random(20261014); + for (int i = 0; i < 1000; i++) { + assertIntegerRoundTrip(random.nextInt()); + } + } + + + @Test + public void testWriteIntegerNegative() { + try { + Asn1Writer.writeInteger(-1); + Assert.fail("Expected IllegalArgumentException for a negative value"); + } catch (IllegalArgumentException e) { + // Expected + } + try { + Asn1Writer.writeInteger(Integer.MIN_VALUE); + Assert.fail("Expected IllegalArgumentException for a negative value"); + } catch (IllegalArgumentException e) { + // Expected + } + } + + + // Regression test for an infinite loop in the length size calculation: + // the shift distance is evaluated modulo 32, so a data length of 16 MiB + // or more made the loop run forever. + @Test + public void testWriteOctetStringSixteenMegabytes() { + byte[] data = new byte[0x1000000]; + byte[] encoded = Asn1Writer.writeOctetString(data); + // 04 84 01 00 00 00 - 4 length bytes for 0x1000000 + Assert.assertEquals(0x04, encoded[0] & 0xFF); + Assert.assertEquals(0x84, encoded[1] & 0xFF); + Assert.assertArrayEquals(new byte[] { 0x01, 0x00, 0x00, 0x00 }, + Arrays.copyOfRange(encoded, 2, 6)); + Assert.assertEquals(6 + data.length, encoded.length); + } + + + private static void assertIntegerEncoding(int value, byte[] expected) { + Assert.assertArrayEquals("Unexpected encoding for " + value, expected, Asn1Writer.writeInteger(value)); + } + + + private static void assertIntegerRoundTrip(int value) { + // Random.nextInt() can produce negative values which must be rejected + if (value < 0) { + try { + Asn1Writer.writeInteger(value); + Assert.fail("Expected IllegalArgumentException for " + value); + } catch (IllegalArgumentException e) { + return; + } + } + byte[] encoded = Asn1Writer.writeInteger(value); + // The header is always two bytes for these short content lengths. + // BigInteger.toByteArray() produces the canonical minimal DER content + // octets for a non-negative value, so compare against that to also + // verify the encoding is minimal. + Assert.assertArrayEquals("Non-minimal encoding for " + value, + BigInteger.valueOf(value).toByteArray(), Arrays.copyOfRange(encoded, 2, encoded.length)); + BigInteger decoded = new Asn1Parser(encoded).parseInt(); + Assert.assertEquals("Round-trip failed for " + value, BigInteger.valueOf(value), decoded); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 4115eb3ba9..bf5d87ec43 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -311,6 +311,15 @@ closes the entire connection. These frames are already permitted when the stream is closed in the other direction. (remm) </fix> + <fix> + Correct the DER INTEGER encoding produced by + <code>Asn1Writer.writeInteger()</code> for multi-byte values and add + the leading zero byte required by DER for positive values whose most + significant bit is set. The length calculation loops in + <code>writeInteger()</code> and <code>writeTag()</code> were also + corrected to terminate, since the Java shift distance is evaluated + modulo 32. (remm) + </fix> </changelog> </subsection> <subsection name="Web applications"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
