From 09d0dab19685574bfe2cd9e2139baee29d997313 Mon Sep 17 00:00:00 2001 From: Les Hazlewood <121180+lhazlewood@users.noreply.github.com> Date: Fri, 1 Sep 2023 19:34:41 -0700 Subject: [PATCH] - Adjusted test case to ensure deterministic outcomes - Consolidated unsigned byte array length calculation for non-negative integers (used in a few places) to a new Bytes#uintLength method. Refactored other classes to use this new method to eliminate code duplication --- .../impl/lang/BigIntegerUBytesConverter.java | 5 ++-- .../java/io/jsonwebtoken/impl/lang/Bytes.java | 15 ++++++------ .../impl/security/EcSignatureAlgorithm.java | 24 +++++++++++++------ .../impl/security/EdwardsCurve.java | 2 +- .../jsonwebtoken/impl/lang/BytesTest.groovy | 16 +++++++++++++ .../security/RsaSignatureAlgorithmTest.groovy | 2 +- 6 files changed, 45 insertions(+), 19 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/BigIntegerUBytesConverter.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/BigIntegerUBytesConverter.java index 781769115..f63d3838f 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/BigIntegerUBytesConverter.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/BigIntegerUBytesConverter.java @@ -34,8 +34,9 @@ public byte[] applyTo(BigInteger bigInt) { final int bitLen = bigInt.bitLength(); final byte[] bytes = bigInt.toByteArray(); - // round bitLen. This gives the minimal number of bytes necessary to represent an unsigned byte array: - final int unsignedByteLen = Bytes.uintLength(bitLen); + // Determine minimal number of bytes necessary to represent an unsigned byte array. + // It must be 1 or more because zero still requires one byte + final int unsignedByteLen = Math.max(1, Bytes.length(bitLen)); // always need at least one byte if (bytes.length == unsignedByteLen) { // already in the form we need return bytes; diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/Bytes.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/Bytes.java index 47a7da6bb..efabdf2ac 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/Bytes.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/Bytes.java @@ -189,8 +189,7 @@ public static long bitLength(byte[] bytes) { } /** - * Returns the minimum number of bytes required to represent the specified non-negative integer as an unsigned - * byte array. + * Returns the minimum number of bytes required to represent the specified number of bits. * *

This is defined/used by many specifications, such as:

* * - * @param i the integer to represent as an unsigned byte array, must be >= 0 - * @return the minimum number of bytes required to represent the specified integer as an unsigned byte array. - * @throws IllegalArgumentException if {@code i} is less than zero. + * @param bitLength the number of bits to represent as a byte array, must be >= 0 + * @return the minimum number of bytes required to represent the specified number of bits. + * @throws IllegalArgumentException if {@code bitLength} is less than zero. */ - public static int uintLength(int i) { - if (i < 0) throw new IllegalArgumentException("uint argument must be >= 0"); - return (i + 7) / Byte.SIZE; + public static int length(int bitLength) { + if (bitLength < 0) throw new IllegalArgumentException("bitLength argument must be >= 0"); + return (bitLength + 7) / Byte.SIZE; } public static String bitsMsg(long bitLength) { diff --git a/impl/src/main/java/io/jsonwebtoken/impl/security/EcSignatureAlgorithm.java b/impl/src/main/java/io/jsonwebtoken/impl/security/EcSignatureAlgorithm.java index 061dface1..68ca7a549 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/security/EcSignatureAlgorithm.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/security/EcSignatureAlgorithm.java @@ -127,13 +127,14 @@ private EcSignatureAlgorithm(int orderBitLength, String oid) { String curveName = "secp" + orderBitLength + "r1"; this.KEY_PAIR_GEN_PARAMS = new ECGenParameterSpec(curveName); this.orderBitLength = orderBitLength; - this.sigFieldByteLength = Bytes.uintLength(this.orderBitLength); + this.sigFieldByteLength = Bytes.length(this.orderBitLength); this.signatureByteLength = this.sigFieldByteLength * 2; // R bytes + S bytes = concat signature bytes } @Override public KeyPairBuilder keyPair() { - return new DefaultKeyPairBuilder(ECCurve.KEY_PAIR_GENERATOR_JCA_NAME, this.KEY_PAIR_GEN_PARAMS).random(Randoms.secureRandom()); + return new DefaultKeyPairBuilder(ECCurve.KEY_PAIR_GENERATOR_JCA_NAME, this.KEY_PAIR_GEN_PARAMS) + .random(Randoms.secureRandom()); } @Override @@ -146,11 +147,14 @@ protected void validateKey(Key key, boolean signing) { ECKey ecKey = (ECKey) key; BigInteger order = ecKey.getParams().getOrder(); int orderBitLength = order.bitLength(); - int sigFieldByteLength = Bytes.uintLength(orderBitLength); + int sigFieldByteLength = Bytes.length(orderBitLength); int concatByteLength = sigFieldByteLength * 2; if (concatByteLength != this.signatureByteLength) { - String msg = "The provided Elliptic Curve " + keyType(signing) + " key's size (aka Order bit length) is " + Bytes.bitsMsg(orderBitLength) + ", but the '" + name + "' algorithm requires EC Keys with " + Bytes.bitsMsg(this.orderBitLength) + " per " + "[RFC 7518, Section 3.4](https://www.rfc-editor.org/rfc/rfc7518.html#section-3.4)."; + String msg = "The provided Elliptic Curve " + keyType(signing) + + " key's size (aka Order bit length) is " + Bytes.bitsMsg(orderBitLength) + ", but the '" + + name + "' algorithm requires EC Keys with " + Bytes.bitsMsg(this.orderBitLength) + + " per [RFC 7518, Section 3.4](https://www.rfc-editor.org/rfc/rfc7518.html#section-3.4)."; throw new InvalidKeyException(msg); } } @@ -201,10 +205,14 @@ public Boolean apply(Signature sig) { * the risk of CVE-2022-21449 attacks on early JVM versions 15, 17 and 18. */ // TODO: remove for 1.0 (DER-encoding support is not in the JWT RFCs) - if (concatSignature[0] == 0x30 && "true".equalsIgnoreCase(System.getProperty(DER_ENCODING_SYS_PROPERTY_NAME))) { + if (concatSignature[0] == 0x30 && + "true".equalsIgnoreCase(System.getProperty(DER_ENCODING_SYS_PROPERTY_NAME))) { derSignature = concatSignature; } else { - String msg = "Provided signature is " + Bytes.bytesMsg(concatSignature.length) + " but " + getId() + " signatures must be exactly " + Bytes.bytesMsg(signatureByteLength) + " per " + "[RFC 7518, Section 3.4 (validation)](https://www.rfc-editor.org/rfc/rfc7518.html#section-3.4)."; + String msg = "Provided signature is " + Bytes.bytesMsg(concatSignature.length) + " but " + + getId() + " signatures must be exactly " + Bytes.bytesMsg(signatureByteLength) + + " per [RFC 7518, Section 3.4 (validation)]" + + "(https://www.rfc-editor.org/rfc/rfc7518.html#section-3.4)."; throw new SignatureException(msg); } } else { @@ -273,7 +281,9 @@ public static byte[] transcodeDERToConcat(final byte[] derSignature, int outputL int rawLen = Math.max(i, j); rawLen = Math.max(rawLen, outputLength / 2); - if ((derSignature[offset - 1] & 0xff) != derSignature.length - offset || (derSignature[offset - 1] & 0xff) != 2 + rLength + 2 + sLength || derSignature[offset] != 2 || derSignature[offset + 2 + rLength] != 2) { + if ((derSignature[offset - 1] & 0xff) != derSignature.length - offset || + (derSignature[offset - 1] & 0xff) != 2 + rLength + 2 + sLength || + derSignature[offset] != 2 || derSignature[offset + 2 + rLength] != 2) { throw new JwtException("Invalid ECDSA signature format"); } diff --git a/impl/src/main/java/io/jsonwebtoken/impl/security/EdwardsCurve.java b/impl/src/main/java/io/jsonwebtoken/impl/security/EdwardsCurve.java index 36f05d9ad..fe7fe3859 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/security/EdwardsCurve.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/security/EdwardsCurve.java @@ -186,7 +186,7 @@ private static byte[] privateKeyPkcs8Prefix(int byteLength, byte[] ASN1_OID, boo this.signatureCurve = (oidTerminalNode == 112 || oidTerminalNode == 113); byte[] suffix = new byte[]{(byte) oidTerminalNode}; this.ASN1_OID = Bytes.concat(ASN1_OID_PREFIX, suffix); - this.encodedKeyByteLength = Bytes.uintLength(this.keyBitLength); + this.encodedKeyByteLength = Bytes.length(this.keyBitLength); this.PUBLIC_KEY_ASN1_PREFIX = publicKeyAsn1Prefix(this.encodedKeyByteLength, this.ASN1_OID); this.PRIVATE_KEY_ASN1_PREFIX = privateKeyPkcs8Prefix(this.encodedKeyByteLength, this.ASN1_OID, true); diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/BytesTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/BytesTest.groovy index 596ce493c..ba8778b14 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/BytesTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/BytesTest.groovy @@ -274,4 +274,20 @@ class BytesTest { assertFalse Bytes.startsWith(A, A, -1) assertFalse Bytes.startsWith(C, A) } + + @Test + void testBytesLength() { + // zero bits means we don't need any bytes: + assertEquals 0, Bytes.length(0) // zero bits means we don't need any bytes + assertEquals 1, Bytes.length(1) // one bit needs at least 1 byte + assertEquals 1, Bytes.length(8) // 8 bits fits into 1 byte + assertEquals 2, Bytes.length(9) // need at least 2 bytes for 9 bits + assertEquals 66, Bytes.length(521) // P-521 curve order bit length + } + + @Test(expected = IllegalArgumentException) + void testBytesLengthNegative() { + Bytes.length(-1) + } + } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/security/RsaSignatureAlgorithmTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/security/RsaSignatureAlgorithmTest.groovy index ffdca721c..0626328fc 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/security/RsaSignatureAlgorithmTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/security/RsaSignatureAlgorithmTest.groovy @@ -158,7 +158,7 @@ class RsaSignatureAlgorithmTest { for (def alg : algs) { def pair = TestKeys.forAlgorithm(alg).pair int bitlen = alg.preferredKeyBitLength + 1 // one more bit than required - int len = Bytes.uintLength(bitlen) + int len = Bytes.length(bitlen) def mag = new byte[len] Randoms.secureRandom().nextBytes(mag) mag[0] = 0x01 // ensure first byte is non-zero so BigInteger doesnt discard leading zero bytes