Fix OCSP nonce handling

Closes #26439

Co-authored-by: Ricardo Martin <rmartinc@redhat.com>
Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

crypto/default/src/main/java/org/keycloak/crypto/def/BCOCSPProvider.java

@@ -53,17 +53,16 @@ import org.bouncycastle.operator.DigestCalculatorProvider;
 import org.bouncycastle.operator.OperatorCreationException;
 import org.bouncycastle.operator.jcajce.JcaContentVerifierProviderBuilder;
 import org.bouncycastle.operator.jcajce.JcaDigestCalculatorProviderBuilder;
+import org.keycloak.jose.jwe.JWEUtils;
 import org.keycloak.common.util.BouncyIntegration;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.utils.OCSPProvider;
 
 import java.io.IOException;
-import java.math.BigInteger;
 import java.net.URI;
 import java.security.GeneralSecurityException;
 import java.security.NoSuchAlgorithmException;
 import java.security.NoSuchProviderException;
-import java.security.SecureRandom;
 import java.security.cert.CRLReason;
 import java.security.cert.CertPath;
 import java.security.cert.CertPathValidatorException;
@@ -120,26 +119,24 @@ public class BCOCSPProvider extends OCSPProvider {
 
             JcaCertificateID certificateID = new JcaCertificateID(digCalc, issuerCertificate, cert.getSerialNumber());
 
-            // Create a nounce extension to protect against replay attacks
-            SecureRandom random = SecureRandom.getInstance("SHA1PRNG");
-            BigInteger nounce = BigInteger.valueOf(Math.abs(random.nextInt()));
-
-            DEROctetString derString = new DEROctetString(nounce.toByteArray());
-            Extension nounceExtension = new Extension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce, false, derString);
-            Extensions extensions = new Extensions(nounceExtension);
-
-            OCSPReq ocspReq = new OCSPReqBuilder().addRequest(certificateID, extensions).build();
-
             URI responderURI = responderURIs.get(0);
-            logger.log(Level.INFO, "OCSP Responder {0}", responderURI);
 
             try {
+                // Create a nonce extension to protect against replay attacks
+                DEROctetString requestNonce = new DEROctetString(new DEROctetString(JWEUtils.generateSecret(16)));
+                Extension nonceExtension = new Extension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce, false, requestNonce);
+                Extensions extensions = new Extensions(nonceExtension);
+
+                OCSPReq ocspReq = new OCSPReqBuilder().addRequest(certificateID).setRequestExtensions(extensions).build();
+
+                logger.log(Level.INFO, "OCSP Responder {0}", responderURI);
+
                 OCSPResp resp = getResponse(session, ocspReq, responderURI);
                 logger.log(Level.FINE, "Received a response from OCSP responder {0}, the response status is {1}", new Object[]{responderURI, resp.getStatus()});
                 switch (resp.getStatus()) {
                     case OCSPResp.SUCCESSFUL:
                         if (resp.getResponseObject() instanceof BasicOCSPResp) {
-                            return processBasicOCSPResponse(issuerCertificate, responderCert, date, certificateID, nounce, (BasicOCSPResp)resp.getResponseObject());
+                            return processBasicOCSPResponse(issuerCertificate, responderCert, date, certificateID, requestNonce, (BasicOCSPResp)resp.getResponseObject());
                         } else {
                             throw new CertPathValidatorException("OCSP responder returned an invalid or unknown OCSP response.");
                         }
@@ -172,7 +169,7 @@ public class BCOCSPProvider extends OCSPProvider {
         }
     }
 
-    private OCSPRevocationStatus processBasicOCSPResponse(X509Certificate issuerCertificate, X509Certificate responderCertificate, Date date, JcaCertificateID certificateID, BigInteger nounce, BasicOCSPResp basicOcspResponse)
+    private OCSPRevocationStatus processBasicOCSPResponse(X509Certificate issuerCertificate, X509Certificate responderCertificate, Date date, JcaCertificateID certificateID, DEROctetString requestNonce, BasicOCSPResp basicOcspResponse)
             throws OCSPException, NoSuchProviderException, NoSuchAlgorithmException, CertificateNotYetValidException, CertificateExpiredException, CertPathValidatorException {
         SingleResp expectedResponse = null;
         for (SingleResp singleResponse : basicOcspResponse.getResponses()) {
@@ -183,7 +180,7 @@ public class BCOCSPProvider extends OCSPProvider {
         }
 
         if (expectedResponse != null) {
-            verifyResponse(basicOcspResponse, issuerCertificate, responderCertificate, nounce.toByteArray(), date);
+            verifyResponse(basicOcspResponse, issuerCertificate, responderCertificate, requestNonce, date);
             return singleResponseToRevocationStatus(expectedResponse);
         } else {
             throw new CertPathValidatorException("OCSP response does not include a response for a certificate supplied in the OCSP request");
@@ -201,7 +198,7 @@ public class BCOCSPProvider extends OCSPProvider {
                 idLeft.getSerialNumber().equals(idRight.getSerialNumber());
     }
 
-    private void verifyResponse(BasicOCSPResp basicOcspResponse, X509Certificate issuerCertificate, X509Certificate responderCertificate, byte[] requestNonce, Date date) throws NoSuchProviderException, NoSuchAlgorithmException, CertificateNotYetValidException, CertificateExpiredException, CertPathValidatorException {
+    private void verifyResponse(BasicOCSPResp basicOcspResponse, X509Certificate issuerCertificate, X509Certificate responderCertificate, DEROctetString requestNonce, Date date) throws NoSuchProviderException, NoSuchAlgorithmException, CertificateNotYetValidException, CertificateExpiredException, CertPathValidatorException {
 
         List<X509CertificateHolder> certs = new ArrayList<>(Arrays.asList(basicOcspResponse.getCerts()));
         X509Certificate signingCert = null;
@@ -332,7 +329,10 @@ public class BCOCSPProvider extends OCSPProvider {
                 throw new CertPathValidatorException("Error verifying OCSP Response\'s signature");
             } else {
                 Extension responseNonce = basicOcspResponse.getExtension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce);
-                if (responseNonce != null && requestNonce != null && !Arrays.equals(requestNonce, responseNonce.getExtnValue().getOctets())) {
+                if (responseNonce == null && requestNonce != null) {
+                    logger.log(Level.FINE, "No OCSP nonce in response");
+                }
+                if (responseNonce != null && requestNonce != null && !requestNonce.equals(responseNonce.getExtnValue())) {
                     throw new CertPathValidatorException("Nonces do not match.");
                 } else {
                     // See Sun's OCSP implementation.

crypto/fips1402/src/main/java/org/keycloak/crypto/fips/BCFIPSOCSPProvider.java

@@ -52,17 +52,16 @@ import org.bouncycastle.operator.DigestCalculatorProvider;
 import org.bouncycastle.operator.OperatorCreationException;
 import org.bouncycastle.operator.jcajce.JcaContentVerifierProviderBuilder;
 import org.bouncycastle.operator.jcajce.JcaDigestCalculatorProviderBuilder;
+import org.keycloak.jose.jwe.JWEUtils;
 import org.keycloak.common.util.BouncyIntegration;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.utils.OCSPProvider;
 
 import java.io.IOException;
-import java.math.BigInteger;
 import java.net.URI;
 import java.security.GeneralSecurityException;
 import java.security.NoSuchAlgorithmException;
 import java.security.NoSuchProviderException;
-import java.security.SecureRandom;
 import java.security.cert.CRLReason;
 import java.security.cert.CertPath;
 import java.security.cert.CertPathValidatorException;
@@ -92,7 +91,7 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
 
     private final static Logger logger = Logger.getLogger(BCFIPSOCSPProvider.class.getName());
 
-    protected OCSPResp getResponse(KeycloakSession session, OCSPReq ocspReq, URI responderUri) throws IOException, InterruptedException {
+    protected OCSPResp getResponse(KeycloakSession session, OCSPReq ocspReq, URI responderUri) throws IOException {
         byte[] data = getEncodedOCSPResponse(session, ocspReq.getEncoded(), responderUri);
         return new OCSPResp(data);
     }
@@ -116,28 +115,27 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
             DigestCalculatorProvider dcp = new JcaDigestCalculatorProviderBuilder().build();
 
             DigestCalculator digCalc = dcp.get(CertificateID.HASH_SHA1);
+
             JcaCertificateID certificateID = new JcaCertificateID(digCalc, issuerCertificate, cert.getSerialNumber());
 
-            // Create a nounce extension to protect against replay attacks
-            SecureRandom random = SecureRandom.getInstance("SHA1PRNG");
-            BigInteger nounce = BigInteger.valueOf(Math.abs(random.nextInt()));
-
-            DEROctetString derString = new DEROctetString(nounce.toByteArray());
-            Extension nounceExtension = new Extension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce, false, derString);
-            Extensions extensions = new Extensions(nounceExtension);
-
-            OCSPReq ocspReq = new OCSPReqBuilder().addRequest(certificateID, extensions).build();
-
             URI responderURI = responderURIs.get(0);
-            logger.log(Level.INFO, "OCSP Responder {0}", responderURI);
 
             try {
+                // Create a nonce extension to protect against replay attacks
+                DEROctetString requestNonce = new DEROctetString(new DEROctetString(JWEUtils.generateSecret(16)));
+                Extension nonceExtension = new Extension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce, false, requestNonce);
+                Extensions extensions = new Extensions(nonceExtension);
+
+                OCSPReq ocspReq = new OCSPReqBuilder().addRequest(certificateID).setRequestExtensions(extensions).build();
+
+                logger.log(Level.INFO, "OCSP Responder {0}", responderURI);
+
                 OCSPResp resp = getResponse(session, ocspReq, responderURI);
                 logger.log(Level.FINE, "Received a response from OCSP responder {0}, the response status is {1}", new Object[]{responderURI, resp.getStatus()});
                 switch (resp.getStatus()) {
                     case OCSPResp.SUCCESSFUL:
                         if (resp.getResponseObject() instanceof BasicOCSPResp) {
-                            return processBasicOCSPResponse(issuerCertificate, responderCert, date, certificateID, nounce, (BasicOCSPResp)resp.getResponseObject());
+                            return processBasicOCSPResponse(issuerCertificate, responderCert, date, certificateID, requestNonce, (BasicOCSPResp)resp.getResponseObject());
                         } else {
                             throw new CertPathValidatorException("OCSP responder returned an invalid or unknown OCSP response.");
                         }
@@ -157,7 +155,7 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
                         throw new CertPathValidatorException("OCSP request is malformed. OCSP response error: " + resp.getStatus(), (Throwable) null, (CertPath) null, -1, CertPathValidatorException.BasicReason.UNSPECIFIED);
                 }
             }
-            catch(IOException | InterruptedException e) {
+            catch(IOException e) {
                 logger.log(Level.FINE, "OCSP Responder \"{0}\" failed to return a valid OCSP response\n{1}",
                         new Object[] {responderURI, e.getMessage()});
                 throw new CertPathValidatorException("OCSP check failed", e);
@@ -170,7 +168,7 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
         }
     }
 
-    private OCSPRevocationStatus processBasicOCSPResponse(X509Certificate issuerCertificate, X509Certificate responderCertificate, Date date, JcaCertificateID certificateID, BigInteger nounce, BasicOCSPResp basicOcspResponse)
+    private OCSPRevocationStatus processBasicOCSPResponse(X509Certificate issuerCertificate, X509Certificate responderCertificate, Date date, JcaCertificateID certificateID, DEROctetString requestNonce, BasicOCSPResp basicOcspResponse)
             throws OCSPException, NoSuchProviderException, NoSuchAlgorithmException, CertificateNotYetValidException, CertificateExpiredException, CertPathValidatorException {
         SingleResp expectedResponse = null;
         for (SingleResp singleResponse : basicOcspResponse.getResponses()) {
@@ -181,7 +179,7 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
         }
 
         if (expectedResponse != null) {
-            verifyResponse(basicOcspResponse, issuerCertificate, responderCertificate, nounce.toByteArray(), date);
+            verifyResponse(basicOcspResponse, issuerCertificate, responderCertificate, requestNonce, date);
             return singleResponseToRevocationStatus(expectedResponse);
         } else {
             throw new CertPathValidatorException("OCSP response does not include a response for a certificate supplied in the OCSP request");
@@ -199,7 +197,7 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
                 idLeft.getSerialNumber().equals(idRight.getSerialNumber());
     }
 
-    private void verifyResponse(BasicOCSPResp basicOcspResponse, X509Certificate issuerCertificate, X509Certificate responderCertificate, byte[] requestNonce, Date date) throws NoSuchProviderException, NoSuchAlgorithmException, CertificateNotYetValidException, CertificateExpiredException, CertPathValidatorException {
+    private void verifyResponse(BasicOCSPResp basicOcspResponse, X509Certificate issuerCertificate, X509Certificate responderCertificate, DEROctetString requestNonce, Date date) throws NoSuchProviderException, NoSuchAlgorithmException, CertificateNotYetValidException, CertificateExpiredException, CertPathValidatorException {
 
         List<X509CertificateHolder> certs = new ArrayList<>(Arrays.asList(basicOcspResponse.getCerts()));
         X509Certificate signingCert = null;
@@ -330,7 +328,10 @@ public class BCFIPSOCSPProvider extends OCSPProvider {
                 throw new CertPathValidatorException("Error verifying OCSP Response\'s signature");
             } else {
                 Extension responseNonce = basicOcspResponse.getExtension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce);
-                if (responseNonce != null && requestNonce != null && !Arrays.equals(requestNonce, responseNonce.getExtnValue().getOctets())) {
+                if (responseNonce == null && requestNonce != null) {
+                    logger.log(Level.FINE, "No OCSP nonce in response");
+                }
+                if (responseNonce != null && requestNonce != null && !requestNonce.equals(responseNonce.getExtnValue())) {
                     throw new CertPathValidatorException("Nonces do not match.");
                 } else {
                     // See Sun's OCSP implementation.

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/x509/OcspHandler.java

@@ -30,6 +30,8 @@ import java.util.Map;
 
 import com.google.common.collect.ImmutableMap;
 
+import org.bouncycastle.asn1.ASN1OctetString;
+import org.bouncycastle.asn1.DEROctetString;
 import org.bouncycastle.asn1.nist.NISTObjectIdentifiers;
 import org.bouncycastle.asn1.ocsp.OCSPObjectIdentifiers;
 import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers;
@@ -105,6 +107,27 @@ final class OcspHandler implements HttpHandler {
         }
     }
 
+    private Extension checkNonce(Extension nonce) {
+        if (nonce == null) {
+            return null;
+        }
+        try {
+            // check the nonce value is an octet string that encapsulates another octet string
+            // see: https://github.com/pyca/cryptography/issues/6404
+            ASN1OctetString value = nonce.getExtnValue();
+            if (!(value instanceof DEROctetString)) {
+                return null;
+            }
+            int length = ASN1OctetString.getInstance(value.getOctets()).getOctetsLength();
+            if (length < 16 || length > 32) {
+                return null;
+            }
+            return nonce;
+        } catch (Exception e) {
+            return null;
+        }
+    }
+
     @Override
     public void handleRequest(final HttpServerExchange exchange) throws Exception {
         if (exchange.isInIoThread()) {
@@ -120,7 +143,7 @@ final class OcspHandler implements HttpHandler {
         final OCSPReq request = new OCSPReq(buffy);
         final Req[] requested = request.getRequestList();
 
-        final Extension nonce = request.getExtension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce);
+        final Extension nonce = checkNonce(request.getExtension(OCSPObjectIdentifiers.id_pkix_ocsp_nonce));
 
         final DigestCalculator sha1Calculator = new JcaDigestCalculatorProviderBuilder().build()
                 .get(AlgorithmIdentifier.getInstance(RespID.HASH_SHA1));
@@ -142,8 +165,11 @@ final class OcspHandler implements HttpHandler {
                 new AlgorithmIdentifier(PKCSObjectIdentifiers.sha256WithRSAEncryption),
                 new AlgorithmIdentifier(NISTObjectIdentifiers.id_sha256)).build(privateKey);
 
-        final OCSPResp response = new OCSPRespBuilder().build(OCSPResp.SUCCESSFUL,
+        // nonce is mandatory for testing that the nonce is properly set
-                responseBuilder.build(contentSigner, chain, new Date()));
+        final OCSPResp response = nonce != null
+                ? new OCSPRespBuilder().build(OCSPResp.SUCCESSFUL,
+                        responseBuilder.build(contentSigner, chain, new Date()))
+                : new OCSPRespBuilder().build(OCSPResp.MALFORMED_REQUEST, null);
 
         final byte[] responseBytes = response.getEncoded();