This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch 3_0_x-fixes in repository https://gitbox.apache.org/repos/asf/ws-wss4j.git
commit 90ec147e2993ff6ca0fbec636a128eac5e526c36 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Tue Sep 22 09:28:40 2026 +0100 Follow-on from last DER patch (#726) --- src/site/asciidoc/migration.adoc | 1 + src/site/asciidoc/wss4j40.adoc | 67 +++++++++ .../org/apache/wss4j/common/crypto/CryptoBase.java | 34 +++-- .../org/apache/wss4j/common/crypto/Merlin.java | 29 +++- .../org/apache/wss4j/common/crypto/MerlinAKI.java | 39 ++++- ...CastleUtils.java => X509KeyIdentifierUtil.java} | 60 ++++++-- .../org/apache/wss4j/common/token/DOMX509SKI.java | 17 ++- .../common/crypto/AuthorityKeyIdentifierTest.java | 20 +-- .../apache/wss4j/common/crypto/DERDecoderTest.java | 166 +++++++++++++++++++++ .../common/crypto/MalformedKeyIdentifierTest.java | 154 +++++++++++++++++++ .../wss4j/common/crypto/NameConstraintsTest.java | 46 ++++++ .../org/apache/wss4j/common/crypto/SKITest.java | 48 ++++++ 12 files changed, 642 insertions(+), 39 deletions(-) diff --git a/src/site/asciidoc/migration.adoc b/src/site/asciidoc/migration.adoc index 2b1245b3a..bc474ff56 100644 --- a/src/site/asciidoc/migration.adoc +++ b/src/site/asciidoc/migration.adoc @@ -21,6 +21,7 @@ Information about migrating to various new versions of WSS4J is provided in this section. +include::wss4j40.adoc[] include::wss4j22.adoc[] include::wss4j21.adoc[] include::wss4j20.adoc[] diff --git a/src/site/asciidoc/wss4j40.adoc b/src/site/asciidoc/wss4j40.adoc new file mode 100644 index 000000000..9d25ca610 --- /dev/null +++ b/src/site/asciidoc/wss4j40.adoc @@ -0,0 +1,67 @@ +// +// 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. +// + +=== Apache WSS4J 4.0.0 Migration Guide + +This section is a migration guide for helping Apache WSS4J 3.0.x users to migrate +to the 4.0.x releases. + +==== Stricter DER decoding of X.509 key identifiers + +The SubjectKeyIdentifier (2.5.29.14) and AuthorityKeyIdentifier (2.5.29.35) +extensions are no longer decoded with Bouncy Castle. WSS4J now decodes them +itself, which removes the last compile-time dependency on the optional Bouncy +Castle artifacts from these code paths. + +RFC 5280 requires certificate extensions to be DER encoded, and the WSS4J decoder +enforces that. Bouncy Castle accepted the broader BER encoding, so a certificate +that is not strictly DER encoded may now be rejected where it previously worked. +The encodings that change behaviour are: + +* a length in long form where the short form would do (for example `81 03` + instead of `03`), or a length carrying a leading zero byte; +* a SubjectKeyIdentifier encoded as a constructed OCTET STRING (tag `24`) whose + segments Bouncy Castle would have concatenated; +* an AuthorityKeyIdentifier `keyIdentifier` encoded as a constructed + `[0]` (tag `A0`) rather than a primitive one (tag `80`); +* an AuthorityKeyIdentifier SEQUENCE using indefinite length; +* an AuthorityKeyIdentifier whose fields appear out of order, or which repeats + the `[0]` field. + +Such a certificate now causes an `IllegalArgumentException` from +`BouncyCastleUtils.getSubjectKeyIdentifierBytes` and +`BouncyCastleUtils.getAuthorityKeyIdentifierBytes`, and a `WSSecurityException` +from `Crypto.getSKIBytesFromCert`. This surfaces when signing with an SKI key +identifier (`DOMX509SKI`) and when establishing trust with `MerlinAKI`. + +If you hit this, the certificate is malformed and should be reissued. There is no +configuration option to restore the previous tolerance. + +==== Crypto.getSKIBytesFromCert shares the key identifier decoder + +`CryptoBase.getSKIBytesFromCert` previously had its own SubjectKeyIdentifier +decoder, which was more tolerant than the one used to emit an SKI key identifier. +Both now use the same decoder, so the identifier WSS4J writes and the one it +accepts agree. A certificate whose SubjectKeyIdentifier has trailing bytes after +the extension value, or whose inner OCTET STRING is not exactly the length the +wrapper declares, is now rejected with a `WSSecurityException` instead of being +decoded on a best-effort basis. + +The behaviour for well formed certificates is unchanged, as is the SHA-1 fallback +for certificates that carry no SubjectKeyIdentifier extension. diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/CryptoBase.java b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/CryptoBase.java index 7279e75f1..3a00688b9 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/CryptoBase.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/CryptoBase.java @@ -208,6 +208,8 @@ public abstract class CryptoBase implements Crypto { * * @param cert The certificate to read SKI * @return The byte array containing the binary SKI data + * @throws WSSecurityException if the SubjectKeyIdentifier extension is present but is not + * valid DER, or if no SKI extension is present and SHA-1 is unavailable */ public byte[] getSKIBytesFromCert(X509Certificate cert) throws WSSecurityException { // @@ -232,15 +234,18 @@ public abstract class CryptoBase implements Crypto { } // - // Strip away first (four) bytes from the DerValue (tag and length of - // ExtensionValue OCTET STRING and KeyIdentifier OCTET STRING) + // Strip away the tag and length of the ExtensionValue OCTET STRING and of the + // KeyIdentifier OCTET STRING it wraps. X509KeyIdentifierUtil is the single decoder for + // this extension, so that the identifier we emit and the one we accept agree. // - DERDecoder extVal = new DERDecoder(derEncodedValue); - extVal.expect(DERDecoder.TYPE_OCTET_STRING); // ExtensionValue OCTET STRING - extVal.getLength(); - extVal.expect(DERDecoder.TYPE_OCTET_STRING); // KeyIdentifier OCTET STRING - int keyIDLen = extVal.getLength(); - return extVal.getBytes(keyIDLen); + try { + return X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(derEncodedValue); + } catch (IllegalArgumentException ex) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN, ex, "noSKIHandling", + new Object[] {"Invalid SubjectKeyIdentifier certificate extension"} + ); + } } /** @@ -387,8 +392,10 @@ public abstract class CryptoBase implements Crypto { * that it is a single byte comparison, the performance hit is negligible. * * @param cert the certificate to extract NameConstraints from - * @return the NameConstraints, or null if not present - * @throws WSSecurityException if a processing error occurs decoding the Octet String + * @return the DER-encoded NameConstraints sequence, or an empty array if the certificate + * carries no NameConstraints extension + * @throws WSSecurityException if a processing error occurs decoding the Octet String, or if + * the extension is tagged as something other than a SEQUENCE or an OCTET STRING */ protected byte[] getNameConstraints(final X509Certificate cert) throws WSSecurityException { byte[] bytes = cert.getExtensionValue(NAME_CONSTRAINTS_OID); @@ -405,8 +412,11 @@ public abstract class CryptoBase implements Crypto { case DERDecoder.TYPE_SEQUENCE: return bytes; default: - throw new IllegalArgumentException( - "Invalid type for NameConstraints; must be Sequence or OctetString-encoded Sequence"); + throw new WSSecurityException( + WSSecurityException.ErrorCode.FAILURE, "certpath", + new Object[] {"Invalid type for NameConstraints; must be a SEQUENCE or an " + + "OCTET STRING-encoded SEQUENCE"} + ); } } } diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java index 2fa2f204b..e75628382 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java @@ -1429,8 +1429,7 @@ public class Merlin extends CryptoBase { (X509Certificate) keyStore.getCertificate(alias); if (cert != null) { if (certProviderHandlesNameConstraints) { - TrustAnchor anchor = new TrustAnchor(cert, getNameConstraints(cert)); - set.add(anchor); + set.add(createTrustAnchor(cert, alias)); } else { TrustAnchor anchor = new TrustAnchor(cert, null); set.add(anchor); @@ -1439,6 +1438,32 @@ public class Merlin extends CryptoBase { } } + /** + * Build a {@code TrustAnchor} carrying the certificate's NameConstraints. + * <p> + * {@code getNameConstraints} reports "no constraints" as an empty array, which + * {@code TrustAnchor} rejects; it wants null. It also signals constraints it cannot decode + * with an IllegalArgumentException, which would otherwise escape a method declared to throw + * WSSecurityException. + * + * @param cert the certificate to anchor trust in + * @param alias the keystore alias it was read from, used to identify it in any error + */ + private TrustAnchor createTrustAnchor(X509Certificate cert, String alias) throws WSSecurityException { + byte[] nameConstraints = getNameConstraints(cert); + if (nameConstraints != null && nameConstraints.length == 0) { + nameConstraints = null; //NOPMD - TrustAnchor reads null, not empty, as "unconstrained" + } + try { + return new TrustAnchor(cert, nameConstraints); + } catch (IllegalArgumentException ex) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.FAILURE, ex, "certpath", + new Object[] {"Invalid NameConstraints extension on keystore entry " + alias} + ); + } + } + /** * Get an implementation-specific identifier that corresponds to the X509Certificate. In * this case, the identifier is the KeyStore alias. diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java index 5e77f8abc..27bf7d7bf 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java @@ -124,8 +124,7 @@ public class MerlinAKI extends Merlin { String issuerString = certs[0].getIssuerX500Principal().getName(); try { if (certs.length == 1) { - byte[] keyIdentifierBytes = - BouncyCastleUtils.getAuthorityKeyIdentifierBytes(certs[0]); + byte[] keyIdentifierBytes = getAuthorityKeyIdentifier(certs[0]); X509Certificate[] foundCerts = getX509CertificatesFromKeyIdentifier(keyIdentifierBytes); // If the certs have not been found, the issuer is not in the keystore/truststore @@ -248,7 +247,7 @@ public class MerlinAKI extends Merlin { if (certs != null && certs.length > 0 && certs[0] instanceof X509Certificate) { byte[] subjectKeyIdentifier = - BouncyCastleUtils.getSubjectKeyIdentifierBytes((X509Certificate)certs[0]); + getSubjectKeyIdentifier((X509Certificate)certs[0], alias); if (subjectKeyIdentifier != null && Arrays.equals(subjectKeyIdentifier, keyIdentifier)) { return certs; @@ -263,4 +262,38 @@ public class MerlinAKI extends Merlin { return new Certificate[]{}; } + /** + * Read the AuthorityKeyIdentifier of a received certificate. + * <p> + * The certificate is attacker supplied, so a malformed extension is reported as a + * WSSecurityException rather than the IllegalArgumentException the decoder raises. Otherwise + * an unchecked exception escapes a method declared to throw WSSecurityException. + */ + private static byte[] getAuthorityKeyIdentifier(X509Certificate cert) throws WSSecurityException { + try { + return X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(cert); + } catch (IllegalArgumentException ex) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.FAILURE, ex, "certpath", + new Object[] {"Invalid AuthorityKeyIdentifier certificate extension"} + ); + } + } + + /** + * Read the SubjectKeyIdentifier of a certificate held in the keystore or truststore. + * <p> + * A certificate we cannot decode cannot match the identifier we are looking for, so the entry + * is skipped rather than failing the whole lookup. Trust still fails if no other entry + * matches. + */ + private static byte[] getSubjectKeyIdentifier(X509Certificate cert, String alias) { + try { + return X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(cert); + } catch (IllegalArgumentException ex) { + LOG.debug("Skipping alias {} with an invalid SubjectKeyIdentifier extension", alias, ex); + return null; //NOPMD - no usable identifier is distinct from an empty one + } + } + } diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/BouncyCastleUtils.java b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/X509KeyIdentifierUtil.java similarity index 59% rename from ws-security-common/src/main/java/org/apache/wss4j/common/crypto/BouncyCastleUtils.java rename to ws-security-common/src/main/java/org/apache/wss4j/common/crypto/X509KeyIdentifierUtil.java index 521a47702..68aa4a4e5 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/BouncyCastleUtils.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/X509KeyIdentifierUtil.java @@ -23,15 +23,33 @@ import java.security.cert.X509Certificate; 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; +/** + * Decodes the X.509 key identifier extensions. + */ +public final class X509KeyIdentifierUtil { + /** AuthorityKeyIdentifier keyIdentifier [0] IMPLICIT KeyIdentifier - primitive, context-specific 0. */ + private static final byte TAG_KEY_IDENTIFIER = (byte)0x80; + /** AuthorityKeyIdentifier authorityCertIssuer [1] GeneralNames - constructed, context-specific 1. */ + private static final byte TAG_AUTHORITY_CERT_ISSUER = (byte)0xA1; + /** AuthorityKeyIdentifier authorityCertSerialNumber [2] IMPLICIT CertificateSerialNumber - + * primitive, context-specific 2. */ + private static final byte TAG_AUTHORITY_CERT_SERIAL_NUMBER = (byte)0x82; - private BouncyCastleUtils() { + private X509KeyIdentifierUtil() { // complete } + /** + * Read the keyIdentifier of the AuthorityKeyIdentifier extension (2.5.29.35) of the + * given certificate. + * <p> + * X.509 extensions are required to use DER; BER encodings are rejected. + * + * @param cert the certificate to read the AuthorityKeyIdentifier from. + * @return an empty array if the certificate has no AuthorityKeyIdentifier extension, null if + * the extension is present but carries no keyIdentifier, otherwise the keyIdentifier. + * @throws IllegalArgumentException if the extension is present but is not valid DER. + */ public static byte[] getAuthorityKeyIdentifierBytes(X509Certificate cert) { byte[] extensionValue = cert.getExtensionValue("2.5.29.35"); //NOPMD if (extensionValue == null) { @@ -40,6 +58,16 @@ public final class BouncyCastleUtils { return getAuthorityKeyIdentifierBytes(extensionValue); } + /** + * Read the SubjectKeyIdentifier extension (2.5.29.14) of the given certificate. + * <p> + * X.509 extensions are required to use DER; BER encodings are rejected. + * + * @param cert the certificate to read the SubjectKeyIdentifier from. + * @return an empty array if the certificate has no SubjectKeyIdentifier extension, otherwise + * the key identifier. + * @throws IllegalArgumentException if the extension is present but is not valid DER. + */ public static byte[] getSubjectKeyIdentifierBytes(X509Certificate cert) { byte[] extensionValue = cert.getExtensionValue("2.5.29.14"); //NOPMD if (extensionValue == null) { @@ -55,9 +83,10 @@ public final class BouncyCastleUtils { 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); + byte[] keyIdentifier = readOptionalValue(authorityKeyIdentifier, TAG_KEY_IDENTIFIER); + // The remaining fields are not used by WSS4J, but must still be well formed. + skipOptionalValue(authorityKeyIdentifier, TAG_AUTHORITY_CERT_ISSUER); + skipOptionalValue(authorityKeyIdentifier, TAG_AUTHORITY_CERT_SERIAL_NUMBER); authorityKeyIdentifier.expectEnd(); return keyIdentifier; } catch (WSSecurityException ex) { @@ -90,7 +119,7 @@ public final class BouncyCastleUtils { } private static byte[] readOptionalValue(DERDecoder decoder, byte type) throws WSSecurityException { - if (!decoder.hasRemaining() || !decoder.test(type)) { + if (!startsWith(decoder, type)) { return null; //NOPMD - an absent optional value is distinct from an empty value } decoder.expect(type); @@ -98,4 +127,17 @@ public final class BouncyCastleUtils { return decoder.getBytes(length); } + private static void skipOptionalValue(DERDecoder decoder, byte type) throws WSSecurityException { + if (!startsWith(decoder, type)) { + return; + } + decoder.expect(type); + int length = decoder.getLength(); + decoder.skip(length); + } + + private static boolean startsWith(DERDecoder decoder, byte type) throws WSSecurityException { + return decoder.hasRemaining() && decoder.test(type); + } + } diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/token/DOMX509SKI.java b/ws-security-common/src/main/java/org/apache/wss4j/common/token/DOMX509SKI.java index b9bfc2476..0de945499 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/token/DOMX509SKI.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/token/DOMX509SKI.java @@ -20,7 +20,8 @@ package org.apache.wss4j.common.token; import org.apache.wss4j.common.WSS4JConstants; -import org.apache.wss4j.common.crypto.BouncyCastleUtils; +import org.apache.wss4j.common.crypto.X509KeyIdentifierUtil; +import org.apache.wss4j.common.ext.WSSecurityException; import org.apache.wss4j.common.util.DOM2Writer; import org.w3c.dom.Document; import org.apache.wss4j.common.util.XMLUtils; @@ -38,9 +39,19 @@ public final class DOMX509SKI { /** * Constructor. + * + * @throws WSSecurityException if the certificate's SubjectKeyIdentifier extension is present + * but is not valid DER. */ - public DOMX509SKI(Document doc, X509Certificate remoteCertificate) { - skiBytes = BouncyCastleUtils.getSubjectKeyIdentifierBytes(remoteCertificate); + public DOMX509SKI(Document doc, X509Certificate remoteCertificate) throws WSSecurityException { + try { + skiBytes = X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(remoteCertificate); + } catch (IllegalArgumentException ex) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.UNSUPPORTED_SECURITY_TOKEN, ex, "noSKIHandling", + new Object[] {"Invalid SubjectKeyIdentifier certificate extension"} + ); + } element = doc.createElementNS(WSS4JConstants.SIG_NS, "ds:X509SKI"); element.setTextContent( diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/AuthorityKeyIdentifierTest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/AuthorityKeyIdentifierTest.java index ff0f0d340..95c6e1a37 100644 --- a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/AuthorityKeyIdentifierTest.java +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/AuthorityKeyIdentifierTest.java @@ -55,7 +55,7 @@ public class AuthorityKeyIdentifierTest { assertNotNull(cert); // Get AuthorityKeyIdentifier from the cert - byte[] keyIdentifierBytes = BouncyCastleUtils.getAuthorityKeyIdentifierBytes(cert); + byte[] keyIdentifierBytes = X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(cert); assertNotNull(keyIdentifierBytes); // Now load the CA cert @@ -67,7 +67,7 @@ public class AuthorityKeyIdentifierTest { // Get SubjectKeyIdentifier from the CA cert byte[] subjectKeyIdentifierBytes = - BouncyCastleUtils.getSubjectKeyIdentifierBytes(caCert); + X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(caCert); assertNotNull(subjectKeyIdentifierBytes); assertTrue(Arrays.equals(keyIdentifierBytes, subjectKeyIdentifierBytes)); @@ -81,11 +81,11 @@ public class AuthorityKeyIdentifierTest { assertArrayEquals( expectedKeyIdentifier, - BouncyCastleUtils.getAuthorityKeyIdentifierBytes(authorityKeyIdentifier) + X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(authorityKeyIdentifier) ); assertArrayEquals( expectedKeyIdentifier, - BouncyCastleUtils.getSubjectKeyIdentifierBytes(subjectKeyIdentifier) + X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(subjectKeyIdentifier) ); } @@ -95,9 +95,9 @@ public class AuthorityKeyIdentifierTest { 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)); + assertNull(X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(authorityKeyIdentifier)); + assertNull(X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(authorityIssuerAndSerial)); + assertNull(X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(emptyAuthorityKeyIdentifier)); } @Test @@ -108,15 +108,15 @@ public class AuthorityKeyIdentifierTest { assertThrows( IllegalArgumentException.class, - () -> BouncyCastleUtils.getSubjectKeyIdentifierBytes(truncatedSubjectKeyIdentifier) + () -> X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(truncatedSubjectKeyIdentifier) ); assertThrows( IllegalArgumentException.class, - () -> BouncyCastleUtils.getAuthorityKeyIdentifierBytes(trailingAuthorityKeyIdentifier) + () -> X509KeyIdentifierUtil.getAuthorityKeyIdentifierBytes(trailingAuthorityKeyIdentifier) ); assertThrows( IllegalArgumentException.class, - () -> BouncyCastleUtils.getSubjectKeyIdentifierBytes(indefiniteLengthSubjectKeyIdentifier) + () -> X509KeyIdentifierUtil.getSubjectKeyIdentifierBytes(indefiniteLengthSubjectKeyIdentifier) ); } diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/DERDecoderTest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/DERDecoderTest.java new file mode 100644 index 000000000..2462b3cb5 --- /dev/null +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/DERDecoderTest.java @@ -0,0 +1,166 @@ +/** + * 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.wss4j.common.crypto; + +import org.apache.wss4j.common.ext.WSSecurityException; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Tests for the bounds handling of DERDecoder. A DER length is attacker-controlled wherever the + * decoder is pointed at a certificate extension, so an over-long or non-minimal length must be + * rejected rather than acted upon. + */ +public class DERDecoderTest { + + /** + * A declared length of Integer.MAX_VALUE from an eleven byte input. The bounds check must not + * be computed as "pos + length", which overflows to a negative number and lets the decoder + * through to a two gigabyte allocation. + */ + @Test + public void testGetBytesRejectsLengthThatWouldOverflowThePosition() throws Exception { + // 04 09 | 04 84 7FFFFFFF | 01 02 03 -- the shape CryptoBase.getSKIBytesFromCert decodes + byte[] extension = {4, 9, 4, (byte)0x84, 0x7F, (byte)0xFF, (byte)0xFF, (byte)0xFF, 1, 2, 3}; + + DERDecoder decoder = new DERDecoder(extension); + decoder.expect(DERDecoder.TYPE_OCTET_STRING); + decoder.getLength(); + decoder.expect(DERDecoder.TYPE_OCTET_STRING); + int keyIdentifierLength = decoder.getLength(); + assertEquals(Integer.MAX_VALUE, keyIdentifierLength); + + assertThrows(WSSecurityException.class, () -> decoder.getBytes(keyIdentifierLength)); + } + + @Test + public void testGetBytesRejectsLengthBeyondTheRemainingInput() throws Exception { + DERDecoder decoder = new DERDecoder(new byte[] {1, 2, 3, 4}); + decoder.skip(2); + + assertArrayEquals(new byte[] {3, 4}, new DERDecoder(new byte[] {3, 4}).getBytes(2)); + assertThrows(WSSecurityException.class, () -> decoder.getBytes(3)); + } + + @Test + public void testGetBytesRejectsNegativeLength() throws Exception { + DERDecoder decoder = new DERDecoder(new byte[] {1, 2, 3, 4}); + + assertThrows(WSSecurityException.class, () -> decoder.getBytes(-1)); + } + + /** + * Skipping past the end must fail rather than leave the position out of bounds, where a large + * enough length would wrap it negative and turn a later read into an ArrayIndexOutOfBounds. + */ + @Test + public void testSkipRejectsLengthBeyondTheRemainingInput() throws Exception { + DERDecoder decoder = new DERDecoder(new byte[] {1, 2, 3, 4}); + decoder.skip(4); + assertThrows(WSSecurityException.class, () -> decoder.skip(1)); + + DERDecoder overflowing = new DERDecoder(new byte[] {1, 2, 3, 4}); + overflowing.skip(2); + assertThrows(WSSecurityException.class, () -> overflowing.skip(Integer.MAX_VALUE)); + } + + @Test + public void testGetLengthReadsShortAndLongForm() throws Exception { + assertEquals(0, new DERDecoder(new byte[] {0}).getLength()); + assertEquals(127, new DERDecoder(new byte[] {0x7F}).getLength()); + assertEquals(128, new DERDecoder(new byte[] {(byte)0x81, (byte)0x80}).getLength()); + assertEquals(256, new DERDecoder(new byte[] {(byte)0x82, 1, 0}).getLength()); + assertEquals(Integer.MAX_VALUE, + new DERDecoder(new byte[] {(byte)0x84, 0x7F, (byte)0xFF, (byte)0xFF, (byte)0xFF}).getLength()); + } + + /** + * Indefinite length is reported as -1, as the method contract has always stated. Every caller + * feeds the result to getBytes or skip, both of which reject a negative length. + */ + @Test + public void testGetLengthReportsIndefiniteLengthAsMinusOne() throws Exception { + assertEquals(-1, new DERDecoder(new byte[] {(byte)0x80, 0, 0}).getLength()); + + DERDecoder decoder = new DERDecoder(new byte[] {(byte)0x80, 0, 0}); + int length = decoder.getLength(); + assertThrows(WSSecurityException.class, () -> decoder.getBytes(length)); + } + + /** + * DER requires the shortest possible length encoding, so a long form that could have been + * short, or one carrying a leading zero, is invalid. + */ + @Test + public void testGetLengthRejectsNonMinimalEncodings() { + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x81, 0x7F}).getLength()); + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x82, 0, (byte)0x80}).getLength()); + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x83, 0, 1, 0}).getLength()); + } + + @Test + public void testGetLengthRejectsLengthsThatDoNotFitInAnInt() { + // 0x80000000 is one past Integer.MAX_VALUE + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x84, (byte)0x80, 0, 0, 0}).getLength()); + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x84, (byte)0xFF, (byte)0xFF, (byte)0xFF, (byte)0xFF}).getLength()); + // more length bytes than an int can hold + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x85, 1, 0, 0, 0, 0}).getLength()); + } + + @Test + public void testGetLengthRejectsTruncatedLengthSpecification() { + assertThrows(WSSecurityException.class, () -> new DERDecoder(new byte[0]).getLength()); + assertThrows(WSSecurityException.class, + () -> new DERDecoder(new byte[] {(byte)0x82, 1}).getLength()); + } + + @Test + public void testExpectEnd() throws Exception { + DERDecoder decoder = new DERDecoder(new byte[] {4, 1, 9}); + decoder.expect(DERDecoder.TYPE_OCTET_STRING); + decoder.getBytes(decoder.getLength()); + decoder.expectEnd(); + + DERDecoder trailing = new DERDecoder(new byte[] {4, 1, 9, 0}); + trailing.expect(DERDecoder.TYPE_OCTET_STRING); + trailing.getBytes(trailing.getLength()); + assertThrows(WSSecurityException.class, trailing::expectEnd); + } + + @Test + public void testHasRemaining() throws Exception { + DERDecoder decoder = new DERDecoder(new byte[] {1, 2}); + decoder.skip(1); + assertEquals(true, decoder.hasRemaining()); + decoder.skip(1); + assertEquals(false, decoder.hasRemaining()); + + assertEquals(false, new DERDecoder(new byte[0]).hasRemaining()); + } +} diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/MalformedKeyIdentifierTest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/MalformedKeyIdentifierTest.java new file mode 100644 index 000000000..b3616f37f --- /dev/null +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/MalformedKeyIdentifierTest.java @@ -0,0 +1,154 @@ +/** + * 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.wss4j.common.crypto; + +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.security.KeyStore; +import java.security.cert.CertificateFactory; +import java.security.cert.X509Certificate; +import java.util.Base64; + +import javax.xml.parsers.DocumentBuilderFactory; + +import org.apache.wss4j.common.ext.WSSecurityException; +import org.apache.wss4j.common.token.DOMX509SKI; +import org.apache.wss4j.common.util.Loader; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Test; +import org.w3c.dom.Document; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * A certificate carrying a malformed key identifier extension reaches these paths from the wire. + * The decoder signals a malformed extension with an IllegalArgumentException, which must not + * escape a trust path declared to throw WSSecurityException. + */ +public class MalformedKeyIdentifierTest { + + /** + * Self-signed, CN=malformed-aki. Its AuthorityKeyIdentifier extnValue is + * 04 08 30 05 80 03 01 02 03 00 -- a well formed SEQUENCE followed by a trailing byte. + */ + private static final String MALFORMED_AKI_CERT = + "MIIBuTCCASKgAwIBAgIBATANBgkqhkiG9w0BAQsFADAYMRYwFAYDVQQDDA1tYWxmb3JtZWQtYWtp" + + "MB4XDTI1MDEwMTAwMDAwMFoXDTM1MDEwMTAwMDAwMFowGDEWMBQGA1UEAwwNbWFsZm9ybWVkLWFr" + + "aTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAq6urq6urq6urq6urq6urq6urq6urq6urq6ur" + + "q6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6ur" + + "q6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6sCAwEAAaMTMBEwDwYD" + + "VR0jBAgwBYADAQIDADANBgkqhkiG9w0BAQsFAAOBgQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=="; + + /** + * Self-signed, CN=malformed-ski. Its SubjectKeyIdentifier extnValue is + * 04 09 04 84 7F FF FF FF 01 02 03 -- an inner OCTET STRING declaring Integer.MAX_VALUE bytes. + */ + private static final String MALFORMED_SKI_CERT = + "MIIBujCCASOgAwIBAgIBATANBgkqhkiG9w0BAQsFADAYMRYwFAYDVQQDDA1tYWxmb3JtZWQtc2tp" + + "MB4XDTI1MDEwMTAwMDAwMFoXDTM1MDEwMTAwMDAwMFowGDEWMBQGA1UEAwwNbWFsZm9ybWVkLXNr" + + "aTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAq6urq6urq6urq6urq6urq6urq6urq6urq6ur" + + "q6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6ur" + + "q6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6sCAwEAAaMUMBIwEAYD" + + "VR0OBAkEhH////8BAgMwDQYJKoZIhvcNAQELBQADgYEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; + + /** + * A received certificate whose AuthorityKeyIdentifier will not decode must fail trust + * verification with a WSSecurityException, not an unchecked exception. + */ + @Test + public void testMerlinAKIRejectsMalformedAuthorityKeyIdentifier() throws Exception { + X509Certificate cert = decode(MALFORMED_AKI_CERT, "2.5.29.35"); + + MerlinAKI crypto = new MerlinAKI(); + crypto.setTrustStore(loadKeyStore("keys/wss40CA.jks", "security")); + + WSSecurityException ex = assertThrows( + WSSecurityException.class, + () -> crypto.verifyTrust(new X509Certificate[]{cert}, false, null) + ); + assertNotNull(ex); + } + + /** + * A truststore entry whose SubjectKeyIdentifier will not decode is skipped rather than + * failing the lookup. The certificate under test is not trusted, so every alias is visited + * and the malformed entry is guaranteed to be reached; trust must then fail with a + * WSSecurityException rather than an unchecked exception from the skipped entry. + */ + @Test + public void testMerlinAKISkipsTruststoreEntryWithMalformedSubjectKeyIdentifier() throws Exception { + X509Certificate untrusted = (X509Certificate)loadKeyStore("keys/wss86.keystore", "security") + .getCertificate("wss86"); + assertNotNull(untrusted); + + KeyStore trustStore = loadKeyStore("keys/wss40CA.jks", "security"); + trustStore.setCertificateEntry("malformed-ski", decode(MALFORMED_SKI_CERT, "2.5.29.14")); + + MerlinAKI crypto = new MerlinAKI(); + crypto.setTrustStore(trustStore); + + WSSecurityException ex = assertThrows( + WSSecurityException.class, + () -> crypto.verifyTrust(new X509Certificate[]{untrusted}, false, null) + ); + assertEquals(WSSecurityException.ErrorCode.FAILURE, ex.getErrorCode()); + } + + /** + * Writing an SKI key identifier for a certificate we cannot decode must fail with a + * WSSecurityException rather than an unchecked exception. + */ + @Test + public void testDOMX509SKIRejectsMalformedSubjectKeyIdentifier() throws Exception { + X509Certificate cert = decode(MALFORMED_SKI_CERT, "2.5.29.14"); + + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + Document doc = factory.newDocumentBuilder().newDocument(); + + assertThrows(WSSecurityException.class, () -> new DOMX509SKI(doc, cert)); + } + + private X509Certificate decode(String base64Certificate, String expectedExtensionOid) throws Exception { + CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509"); + X509Certificate cert; + try (InputStream input = new ByteArrayInputStream(Base64.getDecoder().decode(base64Certificate))) { + cert = (X509Certificate)certificateFactory.generateCertificate(input); + } + // a JDK that rejects the malformed extension outright leaves nothing for us to decode + Assumptions.assumeTrue(cert.getExtensionValue(expectedExtensionOid) != null); + return cert; + } + + private KeyStore loadKeyStore(String path, String password) throws Exception { + KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); + ClassLoader loader = Loader.getClassLoader(MalformedKeyIdentifierTest.class); + try (InputStream input = Merlin.loadInputStream(loader, path)) { + keyStore.load(input, password.toCharArray()); + } + return keyStore; + } +} diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/NameConstraintsTest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/NameConstraintsTest.java index f9b4a3b23..ed7132145 100644 --- a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/NameConstraintsTest.java +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/NameConstraintsTest.java @@ -33,8 +33,10 @@ import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; +import java.util.HashSet; import java.util.List; import java.util.Properties; +import java.util.Set; import java.util.regex.Pattern; import org.apache.wss4j.common.ext.WSSecurityException; @@ -262,6 +264,50 @@ public class NameConstraintsTest { getMerlinAkiBc()); } + /** + * getNameConstraints reports "no constraints" as an empty array, but TrustAnchor reads an + * empty array as an undecodable extension and only accepts null. Most CA certificates carry + * no NameConstraints, so building anchors for them must still work with the + * cert.provider.nameconstraints property enabled. + */ + @Test + public void testTrustAnchorsForCertificatesWithoutNameConstraints() throws Exception { + assumeFalse(isIBMJdK); + + Merlin merlin = merlinHandlingNameConstraints(); + Set<TrustAnchor> anchors = new HashSet<>(); + + merlin.addTrustAnchors(anchors, getSelfKeyStore()); + + assertThat(anchors.size(), equalTo(1)); + assertNull(anchors.iterator().next().getNameConstraints()); + } + + /** + * The constraints of a certificate that does carry the extension must still reach the anchor. + */ + @Test + public void testTrustAnchorsRetainNameConstraints() throws Exception { + assumeFalse(isIBMJdK); + + Merlin merlin = merlinHandlingNameConstraints(); + Set<TrustAnchor> anchors = new HashSet<>(); + + merlin.addTrustAnchors(anchors, getRootKeyStore()); + + assertThat(anchors.size(), equalTo(1)); + TrustAnchor anchor = anchors.iterator().next(); + assertNotNull(anchor.getNameConstraints()); + assertThat("Tag byte is wrong", anchor.getNameConstraints()[0], is(DERDecoder.TYPE_SEQUENCE)); + } + + private Merlin merlinHandlingNameConstraints() throws Exception { + Properties properties = new Properties(); + properties.setProperty("org.apache.wss4j.crypto.merlin.cert.provider.nameconstraints", + "true"); + return new Merlin(properties, this.getClass().getClassLoader(), null); + } + @Test public void testNameConstraintsWithKeyStoreUsingMerlinBreaking() throws Exception { assumeFalse(isIBMJdK); diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/SKITest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/SKITest.java index 14345287c..208a24611 100644 --- a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/SKITest.java +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/SKITest.java @@ -19,16 +19,23 @@ package org.apache.wss4j.common.crypto; +import java.io.ByteArrayInputStream; import java.io.InputStream; import java.security.KeyStore; import java.security.Security; +import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; +import java.util.Base64; +import org.apache.wss4j.common.ext.WSSecurityException; import org.apache.wss4j.common.util.Loader; import org.bouncycastle.jce.provider.BouncyCastleProvider; +import org.junit.jupiter.api.Assumptions; 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.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -82,6 +89,47 @@ public class SKITest { assertTrue(knownBase64Encoding.equals(org.apache.xml.security.utils.XMLUtils.encodeToString(skiBytes))); } + /** + * A self-signed certificate whose SubjectKeyIdentifier extension declares a key identifier of + * Integer.MAX_VALUE bytes. The certificate itself is 442 bytes. Its extnValue is + * 04 09 04 84 7F FF FF FF 01 02 03: an OCTET STRING wrapping an OCTET STRING whose long-form + * length is 0x7FFFFFFF. Certificates reach this code path from an inbound + * wsse:BinarySecurityToken, so the declared length is attacker-controlled. + */ + private static final String HOSTILE_SKI_CERT = + "MIIBtjCCAR+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAWMRQwEgYDVQQDDAtob3N0aWxlLXNraTAe" + + "Fw0yNTAxMDEwMDAwMDBaFw0zNTAxMDEwMDAwMDBaMBYxFDASBgNVBAMMC2hvc3RpbGUtc2tpMIGf" + + "MA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCrq6urq6urq6urq6urq6urq6urq6urq6urq6urq6ur" + + "q6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6ur" + + "q6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urq6urqwIDAQABoxQwEjAQBgNVHQ4E" + + "CQSEf////wECAzANBgkqhkiG9w0BAQsFAAOBgQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=="; + + /** + * An over-long declared length in the SubjectKeyIdentifier must produce a WSSecurityException, + * not a multi-gigabyte allocation and not an unchecked exception. + */ + @Test + public void testSKIWithOverlongDeclaredLength() throws Exception { + CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509"); + X509Certificate cert; + try (InputStream input = new ByteArrayInputStream(Base64.getDecoder().decode(HOSTILE_SKI_CERT))) { + cert = (X509Certificate)certificateFactory.generateCertificate(input); + } + + byte[] extensionValue = cert.getExtensionValue(CryptoBase.SKI_OID); + // A JDK that rejects the malformed extension outright leaves nothing for us to decode + Assumptions.assumeTrue(extensionValue != null); + assertArrayEquals( + new byte[] {4, 9, 4, (byte)0x84, 0x7F, (byte)0xFF, (byte)0xFF, (byte)0xFF, 1, 2, 3}, + extensionValue + ); + + Crypto crypto = new Merlin(); + assertThrows(WSSecurityException.class, () -> crypto.getSKIBytesFromCert(cert)); + } + @Test public void testBouncyCastlePKCS12() throws Exception { try {
