Remove Bouncy Castle from X.509 key identifier parsing

ws-security-common/src/main/java/org/apache/wss4j/common/crypto/BouncyCastleUtils.java

@@ -21,39 +21,81 @@
 
 import java.security.cert.X509Certificate;
 
-import org.bouncycastle.asn1.ASN1OctetString;
-import org.bouncycastle.asn1.x509.AuthorityKeyIdentifier;
-import org.bouncycastle.asn1.x509.SubjectKeyIdentifier;
+import org.apache.wss4j.common.ext.WSSecurityException;
 
 public final class BouncyCastleUtils {
+    private static final byte TYPE_CONTEXT_SPECIFIC_0 = (byte)0x80;
+    private static final byte TYPE_CONTEXT_SPECIFIC_1 = (byte)0xA1;
+    private static final byte TYPE_CONTEXT_SPECIFIC_2 = (byte)0x82;
 
     private BouncyCastleUtils() {
         // complete
     }
 
     public static byte[] getAuthorityKeyIdentifierBytes(X509Certificate cert) {
         byte[] extensionValue = cert.getExtensionValue("2.5.29.35"); //NOPMD
-        if (extensionValue != null) {
-            byte[] octets = ASN1OctetString.getInstance(extensionValue).getOctets();
-            AuthorityKeyIdentifier authorityKeyIdentifier =
-                AuthorityKeyIdentifier.getInstance(octets);
-            return authorityKeyIdentifier.getKeyIdentifier();
+        if (extensionValue == null) {
+            return new byte[0];
         }
-        return new byte[0];
+        return getAuthorityKeyIdentifierBytes(extensionValue);
     }
 
     public static byte[] getSubjectKeyIdentifierBytes(X509Certificate cert) {
         byte[] extensionValue = cert.getExtensionValue("2.5.29.14"); //NOPMD
-        if (extensionValue != null) {
-            byte[] subjectOctets =
-                ASN1OctetString.getInstance(extensionValue).getOctets();
-            SubjectKeyIdentifier subjectKeyIdentifier =
-                SubjectKeyIdentifier.getInstance(subjectOctets);
-            return subjectKeyIdentifier.getKeyIdentifier();
+        if (extensionValue == null) {
+            return new byte[0];
         }
-        return new byte[0];
+        return getSubjectKeyIdentifierBytes(extensionValue);
     }
 
-}
+    static byte[] getAuthorityKeyIdentifierBytes(byte[] extensionValue) {
+        try {
+            byte[] extensionBytes = readExtensionValue(extensionValue, DERDecoder.TYPE_SEQUENCE);
+            if (extensionBytes.length == 0) {
+                return null; //NOPMD - AuthorityKeyIdentifier#getKeyIdentifier returns null when absent
+            }
+            DERDecoder authorityKeyIdentifier = new DERDecoder(extensionBytes);
+            byte[] keyIdentifier = readOptionalValue(authorityKeyIdentifier, TYPE_CONTEXT_SPECIFIC_0);
+            readOptionalValue(authorityKeyIdentifier, TYPE_CONTEXT_SPECIFIC_1);
+            readOptionalValue(authorityKeyIdentifier, TYPE_CONTEXT_SPECIFIC_2);
+            authorityKeyIdentifier.expectEnd();
+            return keyIdentifier;
+        } catch (WSSecurityException ex) {
+            throw new IllegalArgumentException("Invalid AuthorityKeyIdentifier extension", ex);
+        }
+    }
+
+    static byte[] getSubjectKeyIdentifierBytes(byte[] extensionValue) {
+        try {
+            return readExtensionValue(extensionValue, DERDecoder.TYPE_OCTET_STRING);
+        } catch (WSSecurityException ex) {
+            throw new IllegalArgumentException("Invalid SubjectKeyIdentifier extension", ex);
+        }
+    }
 
+    private static byte[] readExtensionValue(byte[] extensionValue, byte extensionType)
+        throws WSSecurityException {
+        DERDecoder extension = new DERDecoder(extensionValue);
+        extension.expect(DERDecoder.TYPE_OCTET_STRING);
+        int extensionLength = extension.getLength();
+        byte[] extensionBytes = extension.getBytes(extensionLength);
+        extension.expectEnd();
 
+        DERDecoder extensionContents = new DERDecoder(extensionBytes);
+        extensionContents.expect(extensionType);
+        int extensionContentsLength = extensionContents.getLength();
+        byte[] contents = extensionContents.getBytes(extensionContentsLength);
+        extensionContents.expectEnd();
+        return contents;
+    }
+
+    private static byte[] readOptionalValue(DERDecoder decoder, byte type) throws WSSecurityException {
+        if (!decoder.hasRemaining() || !decoder.test(type)) {
+            return null; //NOPMD - an absent optional value is distinct from an empty value
+        }
+        decoder.expect(type);
+        int length = decoder.getLength();
+        return decoder.getBytes(length);
+    }
+
+}

ws-security-common/src/main/java/org/apache/wss4j/common/crypto/DERDecoder.java

@@ -77,11 +77,20 @@ public void reset() {
         pos = 0;
     }
 
+    /**
+     * Return whether there are bytes left to decode.
+     *
+     * @return true if bytes remain.
+     */
+    public boolean hasRemaining() {
+        return pos < arr.length;
+    }
+
     /**
      * Advance the current position by the given number of bytes.
      *
      * @param length the number of bytes to skip.
-     * @throws WSSecurityException if length is negative.
+     * @throws WSSecurityException if length is negative or exceeds the remaining input.
      */
     public void skip(int length) throws WSSecurityException {
         if (length < 0) {
@@ -91,6 +100,13 @@ public void skip(int length) throws WSSecurityException {
                     new Object[] {"Unsupported DER format"}
             );
         }
+        if (length > arr.length - pos) {
+            throw new WSSecurityException(
+                    WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
+                    "noSKIHandling",
+                    new Object[] {"Invalid DER format"}
+            );
+        }
         pos += length;
     }
 
@@ -170,21 +186,41 @@ public int getLength() throws WSSecurityException {
                     new Object[] {"Invalid DER format"}
             );
         }
+        int firstByte = arr[pos++] & 0xFF;
         int len;
-        if ((arr[pos] & 0xFF) <= 0x7F) {
-            len = arr[pos++];
+        if (firstByte <= 0x7F) {
+            len = firstByte;
         } else {
-            int nbytes = arr[pos++] & 0x7F;
+            int nbytes = firstByte & 0x7F;
+            if (nbytes == 0) {
+                return -1;
+            }
             if (pos + nbytes > arr.length) {
                 throw new WSSecurityException(
                         WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
                         "noSKIHandling",
                         new Object[] {"Invalid DER format"}
                 );
             }
+            if (nbytes > Integer.BYTES || arr[pos] == 0) {
+                throw new WSSecurityException(
+                    WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
+                    "noSKIHandling",
+                    new Object[] {"Invalid DER format"}
+                );
+            }
             byte[] lenBytes = new byte[nbytes];
             System.arraycopy(arr, pos, lenBytes, 0, lenBytes.length);
-            len = new BigInteger(1, lenBytes).intValue();
+            BigInteger bigIntegerLength = new BigInteger(1, lenBytes);
+            if (bigIntegerLength.compareTo(BigInteger.valueOf(0x80)) < 0
+                || bigIntegerLength.compareTo(BigInteger.valueOf(Integer.MAX_VALUE)) > 0) {
+                throw new WSSecurityException(
+                    WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
+                    "noSKIHandling",
+                    new Object[] {"Invalid DER format"}
+                );
+            }
+            len = bigIntegerLength.intValue();
             pos += nbytes;
         }
         return len;
@@ -201,23 +237,38 @@ public int getLength() throws WSSecurityException {
      *         length is negative.
      */
     public byte[] getBytes(int length) throws WSSecurityException {
-        if (pos + length > arr.length) {
+        if (length < 0) {
             throw new WSSecurityException(
                     WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
                     "noSKIHandling",
-                    new Object[] {"Invalid DER format"}
-             );
-        } else if (length < 0) {
+                    new Object[] {"Unsupported DER format"}
+            );
+        } else if (length > arr.length - pos) {
             throw new WSSecurityException(
                     WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
                     "noSKIHandling",
-                    new Object[] {"Unsupported DER format"}
-            );
+                    new Object[] {"Invalid DER format"}
+             );
         }
         byte[] value = new byte[length];
         System.arraycopy(arr, pos, value, 0, length);
         pos += length;
         return value;
     }
 
+    /**
+     * Confirm that the current position is at the end of the DER value.
+     *
+     * @throws WSSecurityException if unconsumed bytes remain.
+     */
+    public void expectEnd() throws WSSecurityException {
+        if (pos != arr.length) {
+            throw new WSSecurityException(
+                    WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN,
+                    "noSKIHandling",
+                    new Object[] {"Invalid DER format"}
+            );
+        }
+    }
+
 }

ws-security-common/src/test/java/org/apache/wss4j/common/crypto/AuthorityKeyIdentifierTest.java

@@ -28,13 +28,16 @@
 import org.apache.wss4j.common.util.Loader;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * This is a test for extracting AuthorityKeyIdentifier/SubjectKeyIdentifier information from
- * the certs using BouncyCastle.
+ * certificates.
  */
 public class AuthorityKeyIdentifierTest {
 
@@ -70,6 +73,53 @@ public void testExtractKeyIdentifiers() throws Exception {
         assertTrue(Arrays.equals(keyIdentifierBytes, subjectKeyIdentifierBytes));
     }
 
+    @Test
+    public void testExtractKeyIdentifiersFromDer() {
+        byte[] expectedKeyIdentifier = {1, 2, 3};
+        byte[] authorityKeyIdentifier = {4, 7, 48, 5, (byte)0x80, 3, 1, 2, 3};
+        byte[] subjectKeyIdentifier = {4, 5, 4, 3, 1, 2, 3};
+
+        assertArrayEquals(
+            expectedKeyIdentifier,
+            BouncyCastleUtils.getAuthorityKeyIdentifierBytes(authorityKeyIdentifier)
+        );
+        assertArrayEquals(
+            expectedKeyIdentifier,
+            BouncyCastleUtils.getSubjectKeyIdentifierBytes(subjectKeyIdentifier)
+        );
+    }
+
+    @Test
+    public void testAuthorityKeyIdentifierWithoutKeyIdentifier() {
+        byte[] authorityKeyIdentifier = {4, 5, 48, 3, (byte)0x82, 1, 1};
+        byte[] authorityIssuerAndSerial = {4, 10, 48, 8, (byte)0xA1, 3, 48, 1, 0, (byte)0x82, 1, 1};
+        byte[] emptyAuthorityKeyIdentifier = {4, 2, 48, 0};
+
+        assertNull(BouncyCastleUtils.getAuthorityKeyIdentifierBytes(authorityKeyIdentifier));
+        assertNull(BouncyCastleUtils.getAuthorityKeyIdentifierBytes(authorityIssuerAndSerial));
+        assertNull(BouncyCastleUtils.getAuthorityKeyIdentifierBytes(emptyAuthorityKeyIdentifier));
+    }
+
+    @Test
+    public void testRejectMalformedKeyIdentifiers() {
+        byte[] truncatedSubjectKeyIdentifier = {4, 5, 4, 3, 1, 2};
+        byte[] trailingAuthorityKeyIdentifier = {4, 7, 48, 5, (byte)0x80, 3, 1, 2, 3, 0};
+        byte[] indefiniteLengthSubjectKeyIdentifier = {4, (byte)0x80, 4, 0, 0, 0};
+
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> BouncyCastleUtils.getSubjectKeyIdentifierBytes(truncatedSubjectKeyIdentifier)
+        );
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> BouncyCastleUtils.getAuthorityKeyIdentifierBytes(trailingAuthorityKeyIdentifier)
+        );
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> BouncyCastleUtils.getSubjectKeyIdentifierBytes(indefiniteLengthSubjectKeyIdentifier)
+        );
+    }
+
     @Test
     public void testMerlinAKI() throws Exception {
         // Load the keystore
@@ -110,4 +160,4 @@ private KeyStore loadKeyStore(String path, String password) throws Exception {
 
         return keyStore;
     }
-}
+}