diff --git a/CHANGES b/CHANGES index 09340bde76..b1c5f03a1f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,13 @@ +2.3-134 | 2014-09-04 16:16:36 -0700 + + * Fixed a number of issues with OCSP reply validation. Addresses + BIT-1212. (Johanna Amann) + + * Fix null pointer dereference in OCSP verification code in case no + certificate is sent as part as the ocsp reply. Addresses BIT-1212. + (Johanna Amann) + 2.3-131 | 2014-09-04 16:10:32 -0700 * Make links in documentation templates protocol relative. (Johanna diff --git a/VERSION b/VERSION index 3647ae8a44..0154ba2563 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.3-131 +2.3-134 diff --git a/src/file_analysis/analyzer/x509/functions.bif b/src/file_analysis/analyzer/x509/functions.bif index 9a8a8e78b7..216f4c69cc 100644 --- a/src/file_analysis/analyzer/x509/functions.bif +++ b/src/file_analysis/analyzer/x509/functions.bif @@ -104,6 +104,39 @@ STACK_OF(X509)* x509_get_untrusted_stack(VectorVal* certs_vec) return untrusted_certs; } +// We need this function to be able to identify the signer certificate of an +// OCSP request out of a list of possible certificates. +X509* x509_get_ocsp_signer(STACK_OF(X509) *certs, OCSP_RESPID *rid) + { + // We support two lookup types - either by response id or by key. + if ( rid->type == V_OCSP_RESPID_NAME ) + return X509_find_by_subject(certs, rid->value.byName); + + // There only should be name and type - but let's be sure... + if ( rid->type != V_OCSP_RESPID_KEY ) + return 0; + + // Just like OpenSSL, we just support SHA-1 lookups and bail out otherwhise. + if ( rid->value.byKey->length != SHA_DIGEST_LENGTH ) + return 0; + + unsigned char* key_hash = rid->value.byKey->data; + for ( int i = 0; i < sk_X509_num(certs); ++i ) + { + unsigned char digest[SHA_DIGEST_LENGTH]; + X509* cert = sk_X509_value(certs, i); + if ( ! X509_pubkey_digest(cert, EVP_sha1(), digest, NULL) ) + // digest failed for this certificate, try with next + continue; + + if ( memcmp(digest, key_hash, SHA_DIGEST_LENGTH) == 0 ) + // keys match, return certificate + return cert; + } + + return 0; + } + %%} ## Parses a certificate into an X509::Certificate structure. @@ -221,6 +254,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c int out = -1; int result = -1; X509* issuer_certificate = 0; + X509* signer = 0; OCSP_RESPONSE *resp = d2i_OCSP_RESPONSE(NULL, &start, ocsp_reply->Len()); if ( ! resp ) { @@ -250,19 +284,47 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c // inject the certificates in the certificate list of the OCSP reply, they actually are used during // the lookup. // Yay. + + if ( ! basic->certs ) + { + basic->certs = sk_X509_new_null(); + if ( ! basic->certs ) + { + rval = x509_result_record(-1, "Could not allocate basic x509 stack"); + goto x509_ocsp_cleanup; + } + } + issuer_certificate = 0; for ( int i = 0; i < sk_X509_num(untrusted_certs); i++) { sk_X509_push(basic->certs, X509_dup(sk_X509_value(untrusted_certs, i))); - if ( X509_NAME_cmp(X509_get_issuer_name(cert), X509_get_subject_name(sk_X509_value(untrusted_certs, i))) ) + if ( X509_NAME_cmp(X509_get_issuer_name(cert), X509_get_subject_name(sk_X509_value(untrusted_certs, i))) == 0 ) issuer_certificate = sk_X509_value(untrusted_certs, i); } // Because we actually want to be able to give nice error messages that show why we were // not able to verify the OCSP response - do our own verification logic first. + signer = x509_get_ocsp_signer(basic->certs, basic->tbsResponseData->responderId); + + /* + Do this perhaps - OpenSSL also cannot do it, so I do not really feel bad about it. + Needs a different lookup because the root store is no stack of X509 certs + + if ( !s igner ) + // if we did not find it in the certificates that were sent, search in the root store + signer = x509_get_ocsp_signer(basic->certs, basic->tbsResponseData->responderId); + */ + + if ( ! signer ) + { + rval = x509_result_record(-1, "Could not find OCSP responder certificate"); + goto x509_ocsp_cleanup; + } + csc = X509_STORE_CTX_new(); - X509_STORE_CTX_init(csc, ctx, sk_X509_value(basic->certs, 0), basic->certs); + X509_STORE_CTX_init(csc, ctx, signer, basic->certs); X509_STORE_CTX_set_time(csc, 0, (time_t) verify_time); X509_STORE_CTX_set_purpose(csc, X509_PURPOSE_OCSP_HELPER); @@ -281,7 +343,6 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c goto x509_ocsp_cleanup; } - // ok, now we verified the OCSP response. This means that we have a valid chain tying it // to a root that we trust and that the signature also hopefully is valid. This does not yet // mean that the ocsp response actually matches the certificate the server send us or that @@ -322,7 +383,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c goto x509_ocsp_cleanup; } - if ( ! OCSP_id_cmp(certid, single->certId) ) + if ( OCSP_id_cmp(certid, single->certId) != 0 ) return x509_result_record(-1, "OCSP reply is not for host certificate"); // next - check freshness of proof... diff --git a/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-digicert.log b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-digicert.log new file mode 100644 index 0000000000..bb0a25ac0c --- /dev/null +++ b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-digicert.log @@ -0,0 +1,10 @@ +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path ssl +#open 2014-09-04-19-17-18 +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name session_id last_alert established cert_chain_fuids client_cert_chain_fuids subject issuer client_subject client_issuer ocsp_status +#types time string addr port addr port string string string string string string bool vector[string] vector[string] string string string string string +1404148886.994021 CXWv6p3arKYeMETxOg 192.168.4.149 51293 72.21.91.29 443 TLSv10 TLS_ECDHE_RSA_WITH_RC4_128_SHA secp256r1 - - - T FhwjYM0FkbvVCvMf2,Fajs2d2lipsadwoK1h (empty) CN=www.digicert.com,O=DigiCert\, Inc.,L=Lehi,ST=Utah,C=US,postalCode=84043,street=2600 West Executive Parkway,street=Suite 500,serialNumber=5299537-0142,1.3.6.1.4.1.311.60.2.1.2=#130455746168,1.3.6.1.4.1.311.60.2.1.3=#13025553,businessCategory=Private Organization CN=DigiCert SHA2 Extended Validation Server CA,OU=www.digicert.com,O=DigiCert Inc,C=US - - good +#close 2014-09-04-19-17-18 diff --git a/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-twimg.log b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-twimg.log new file mode 100644 index 0000000000..4806744a5c --- /dev/null +++ b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-twimg.log @@ -0,0 +1,10 @@ +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path ssl +#open 2014-09-04-19-17-14 +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name session_id last_alert established cert_chain_fuids client_cert_chain_fuids subject issuer client_subject client_issuer ocsp_status +#types time string addr port addr port string string string string string string bool vector[string] vector[string] string string string string string +1409786981.016881 CXWv6p3arKYeMETxOg 192.168.4.149 53106 93.184.216.146 443 TLSv10 TLS_ECDHE_RSA_WITH_RC4_128_SHA secp256r1 - - - T FtaZVlJfywdNmVFr1,FoILekwkdtTuZtlVa (empty) CN=si0.twimg.com,O=Twitter\, Inc.,L=San Francisco,ST=California,C=US CN=DigiCert High Assurance CA-3,OU=www.digicert.com,O=DigiCert Inc,C=US - - good +#close 2014-09-04-19-17-14 diff --git a/testing/btest/Traces/tls/ocsp-stapling-digicert.trace b/testing/btest/Traces/tls/ocsp-stapling-digicert.trace new file mode 100644 index 0000000000..982249c0f5 Binary files /dev/null and b/testing/btest/Traces/tls/ocsp-stapling-digicert.trace differ diff --git a/testing/btest/Traces/tls/ocsp-stapling-twimg.trace b/testing/btest/Traces/tls/ocsp-stapling-twimg.trace new file mode 100644 index 0000000000..f53762f6e0 Binary files /dev/null and b/testing/btest/Traces/tls/ocsp-stapling-twimg.trace differ diff --git a/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro b/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro index b0392f9c27..e7e3c3ff8e 100644 --- a/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro +++ b/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro @@ -1,4 +1,10 @@ # @TEST-EXEC: bro -C -r $TRACES/tls/ocsp-stapling.trace %INPUT # @TEST-EXEC: btest-diff ssl.log +# @TEST-EXEC: bro -C -r $TRACES/tls/ocsp-stapling-twimg.trace %INPUT +# @TEST-EXEC: mv ssl.log ssl-twimg.log +# @TEST-EXEC: btest-diff ssl-twimg.log +# @TEST-EXEC: bro -C -r $TRACES/tls/ocsp-stapling-digicert.trace %INPUT +# @TEST-EXEC: mv ssl.log ssl-digicert.log +# @TEST-EXEC: btest-diff ssl-digicert.log @load protocols/ssl/validate-ocsp