ocsp: add workaround for OCSP_basic_verify() bugtopic/ocsp-basic-verify-bug

Older versions of OpenSSL have a bug that it doesn't use the
certificates passed to OCSP_basic_verify() for verifying the chain. This
can be a problem when the response is signed by a certificate issued by
an intermediate CA.

      root_ca
        |
  intermediate_ca
        |-------------|
    end_entity    ocsp_signer

When the certificate hierarchy is like this, and the response contains
only ocsp_signer certificate, the following code wrongly fails.

  store = OpenSSL::X509::Store.new; store.add_cert(root_ca)
  basic_response.verify([intermediate_ca], store)

So duplicate the OCSP_BASICRESP and add the certificates to the embedded
list first.

2 files changed, 59 insertions, 0 deletions

diff --git a/ext/openssl/ossl_ocsp.c b/ext/openssl/ossl_ocsp.c
index c0f2dfef..90b24edb 100644
--- a/ext/openssl/ossl_ocsp.c
+++ b/ext/openssl/ossl_ocsp.c
@@ -1065,7 +1065,55 @@ ossl_ocspbres_verify(int argc, VALUE *argv, VALUE self)
     x509st = GetX509StorePtr(store);
     flg = NIL_P(flags) ? 0 : NUM2INT(flags);
     x509s = ossl_x509_ary2sk(certs);
+#if (OPENSSL_VERSION_NUMBER < 0x1000202fL) || defined(LIBRESSL_VERSION_NUMBER)
+    /*
+     * OpenSSL had a bug that it doesn't use the certificates in x509s for
+     * verifying the chain. This can be a problem when the response is signed by
+     * a certificate issued by an intermediate CA.
+     *
+     *       root_ca
+     *         |
+     *   intermediate_ca
+     *         |-------------|
+     *     end_entity    ocsp_signer
+     *
+     * When the certificate hierarchy is like this, and the response contains
+     * only ocsp_signer certificate, the following code wrongly fails.
+     *
+     *   store = OpenSSL::X509::Store.new; store.add_cert(root_ca)
+     *   basic_response.verify([intermediate_ca], store)
+     *
+     * So add the certificates in x509s to the embedded certificates list first.
+     *
+     * This is fixed in OpenSSL 0.9.8zg, 1.0.0s, 1.0.1n, 1.0.2b. But it still
+     * exists in LibreSSL 2.1.10, 2.2.9, 2.3.6, 2.4.1.
+     */
+    if (!(flg & (OCSP_NOCHAIN | OCSP_NOVERIFY)) &&
+	sk_X509_num(x509s) && sk_X509_num(bs->certs)) {
+	int i;
+
+	bs = ASN1_item_dup(ASN1_ITEM_rptr(OCSP_BASICRESP), bs);
+	if (!bs) {
+	    sk_X509_pop_free(x509s, X509_free);
+	    ossl_raise(eOCSPError, "ASN1_item_dup");
+	}
+
+	for (i = 0; i < sk_X509_num(x509s); i++) {
+	    if (!OCSP_basic_add1_cert(bs, sk_X509_value(x509s, i))) {
+		sk_X509_pop_free(x509s, X509_free);
+		OCSP_BASICRESP_free(bs);
+		ossl_raise(eOCSPError, "OCSP_basic_add1_cert");
+	    }
+	}
+	result = OCSP_basic_verify(bs, x509s, x509st, flg);
+	OCSP_BASICRESP_free(bs);
+    }
+    else {
+	result = OCSP_basic_verify(bs, x509s, x509st, flg);
+    }
+#else
     result = OCSP_basic_verify(bs, x509s, x509st, flg);
+#endif
     sk_X509_pop_free(x509s, X509_free);
     if (!result)
 	ossl_clear_error();
diff --git a/test/test_ocsp.rb b/test/test_ocsp.rb
index db7b139e..c59cf1b9 100644
--- a/test/test_ocsp.rb
+++ b/test/test_ocsp.rb
@@ -184,6 +184,17 @@ class OpenSSL::TestOCSP < OpenSSL::TestCase
     assert_equal false, bres.verify([], store1, OpenSSL::OCSP::NOCHAIN)
   end
 
+  def test_basic_response_sign_verify_use_extra_chain
+    # OpenSSL had a bug on this; test that our workaround works
+    cid = OpenSSL::OCSP::CertificateId.new(@cert2, @cert, OpenSSL::Digest::SHA256.new)
+    bres = OpenSSL::OCSP::BasicResponse.new
+    bres.add_status(cid, OpenSSL::OCSP::V_CERTSTATUS_GOOD, nil, -400, -300, 500, [])
+    bres.sign(@ocsp_cert, @ocsp_key, [], 0, "SHA256")
+    store1 = OpenSSL::X509::Store.new; store1.add_cert(@ca_cert)
+    assert_equal true, bres.verify([@cert], store1)
+    assert_equal false, bres.verify([], store1, OpenSSL::OCSP::NOCHAIN)
+  end
+
   def test_basic_response_dup
     bres = OpenSSL::OCSP::BasicResponse.new
     cid = OpenSSL::OCSP::CertificateId.new(@cert, @ca_cert, OpenSSL::Digest::SHA1.new)