From 998c2aa1f832042a9a95644b56c2ca0910c895b4 Mon Sep 17 00:00:00 2001 From: David Cooper Date: Tue, 29 May 2018 16:39:46 -0400 Subject: [PATCH 1/2] Fix false "revoked" results for CRL and OCSP checking This PR fixes problems with check_revocation_crl() sometimes reporting that a certificate is revoked even when it isn't, and with check_revocation_ocsp() sometimes reporting "error querying OCSP responder" even if the OCSP responder provided a good response. The most common reason for this to happen is that OpenSSL cannot validate the server's certificate (even without status checking). PR #1051 attempted to get status checking to work even in cases in which the server's certificate could not be validated. This PR instead addresses the problem by not checking status if determine_trust() was unable to validate the server's certificate. In some cases the server's certificate can be validated using some, but not all of the bundles of trusted certificates. For example, I have encountered some sites that can be validated using the Microsoft and Apple bundles, but not the Linux or Mozilla bundles. This PR introduces GOOD_CA_BUNDLE to store a bundle that could be used to successfully validate the server's certificate. If there is no such bundle, then neither check_revocation_crl() nor check_revocation_ocsp() is run. When check_revocation_crl() and check_revocation_ocsp() are called, the status checks within them closely match the validation check in determine_trust(), which helps to ensure that if the check fails it is because of the status information. As noted in #1057, at least one CA provides incorrect information when the CRL is downloaded, so validation could fail for a reason other than the certificate being revoked. So, this PR adds a check of the reason that validation failed and only reports "revoked" if the validation failed for that reason. As noted in #1056, it is not possible to perform an OCSP query without access to the certificate issuer's public key. So, with this PR check_revocation_ocsp() is only called if the server's provided at least one intermediate certificate (i.e., the issuer's certificate, which contains the issuer's public key). --- testssl.sh | 46 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/testssl.sh b/testssl.sh index 3e7ca69..eabffa9 100755 --- a/testssl.sh +++ b/testssl.sh @@ -247,6 +247,7 @@ UNBRACKTD_IPV6=${UNBRACKTD_IPV6:-false} # some versions of OpenSSL (like Gentoo) NO_ENGINE=${NO_ENGINE:-false} # if there are problems finding the (external) openssl engine set this to true declare -r CLIENT_MIN_PFS=5 # number of ciphers needed to run a test for PFS CAPATH="${CAPATH:-/etc/ssl/certs/}" # Does nothing yet (FC has only a CA bundle per default, ==> openssl version -d) +GOOD_CA_BUNDLE="" # A bundle of CA certificates that can be used to validate the server's certificate MEASURE_TIME_FILE=${MEASURE_TIME_FILE:-""} if [[ -n "$MEASURE_TIME_FILE" ]] && [[ -z "$MEASURE_TIME" ]]; then MEASURE_TIME=true @@ -1451,7 +1452,7 @@ check_revocation_crl() { local crl="$1" local jsonID="$2" local tmpfile="" - local scheme + local scheme retcode local -i success "$PHONE_OUT" || return 0 @@ -1474,22 +1475,38 @@ check_revocation_crl() { return 1 fi # -crl_download could be more elegant but is supported from 1.0.2 onwards only - $OPENSSL crl -inform DER -in "$tmpfile" -outform PEM -out "${tmpfile%%.crl}.pem" + $OPENSSL crl -inform DER -in "$tmpfile" -outform PEM -out "${tmpfile%%.crl}.pem" &>$ERRFILE if [[ $? -ne 0 ]]; then pr_warning "conversion of "$tmpfile" failed" fileout "$jsonID" "WARN" "conversion of CRL to PEM format failed" return 1 fi - cat $TEMPDIR/intermediatecerts.pem "${tmpfile%%.crl}.pem" >$TEMPDIR/${NODE}-${NODEIP}-CRL-chain.pem - $OPENSSL verify -crl_check -CAfile $TEMPDIR/${NODE}-${NODEIP}-CRL-chain.pem $TEMPDIR/host_certificate.pem &>$ERRFILE + if grep -q "\-\-\-\-\-BEGIN CERTIFICATE\-\-\-\-\-" $TEMPDIR/intermediatecerts.pem; then + $OPENSSL verify -crl_check -CAfile <(cat $ADDITIONAL_CA_FILES "$GOOD_CA_BUNDLE" "${tmpfile%%.crl}.pem") -untrusted $TEMPDIR/intermediatecerts.pem $HOSTCERT &> "${tmpfile%%.crl}.err" + else + $OPENSSL verify -crl_check -CAfile <(cat $ADDITIONAL_CA_FILES "$GOOD_CA_BUNDLE" "${tmpfile%%.crl}.pem") $HOSTCERT &> "${tmpfile%%.crl}.err" + fi if [[ $? -eq 0 ]]; then out ", " pr_svrty_good "not revoked" fileout "$jsonID" "OK" "not revoked" else - out ", " - pr_svrty_critical "revoked" - fileout "$jsonID" "CRITICAL" "revoked" + retcode=$(awk '/error [1-9][0-9]? at [0-9]+ depth lookup:/ { if (!found) {print $2; found=1} }' "${tmpfile%%.crl}.err") + if [[ "$retcode" == "23" ]]; then # see verify_retcode_helper() + out ", " + pr_svrty_critical "revoked" + fileout "$jsonID" "CRITICAL" "revoked" + else + retcode="$(verify_retcode_helper "$retcode")" + out " $retcode" + retcode="${retcode#(}" + retcode="${retcode%)}" + fileout "$jsonID" "WARN" "$retcode" + if [[ $DEBUG -ge 2 ]]; then + outln + cat "${tmpfile%%.crl}.err" + fi + fi fi return 0 } @@ -1513,7 +1530,7 @@ check_revocation_ocsp() { fi $OPENSSL ocsp -no_nonce ${host_header} -url "$uri" \ -issuer $TEMPDIR/hostcert_issuer.pem -verify_other $TEMPDIR/intermediatecerts.pem \ - -CAfile $TEMPDIR/intermediatecerts.pem -cert $HOSTCERT -text &> "$tmpfile" + -CAfile <(cat $ADDITIONAL_CA_FILES "$GOOD_CA_BUNDLE") -cert $HOSTCERT -text &> "$tmpfile" if [[ $? -eq 0 ]] && grep -Fq "Response verify OK" "$tmpfile"; then response="$(grep -F "$HOSTCERT: " "$tmpfile")" response="${response#$HOSTCERT: }" @@ -6218,6 +6235,7 @@ verify_retcode_helper() { case $retcode in # codes from ./doc/apps/verify.pod | verify(1ssl) + 44) tm_out "(different CRL scope)" ;; # X509_V_ERR_DIFFERENT_CRL_SCOPE 26) tm_out "(unsupported certificate purpose)" ;; # X509_V_ERR_INVALID_PURPOSE 24) tm_out "(certificate unreadable)" ;; # X509_V_ERR_INVALID_CA 23) tm_out "(certificate revoked)" ;; # X509_V_ERR_CERT_REVOKED @@ -6292,6 +6310,7 @@ determine_trust() { if [[ ${verify_retcode[i]} -eq 0 ]]; then trust[i]=true some_ok=true + [[ -z "$GOOD_CA_BUNDLE" ]] && GOOD_CA_BUNDLE="$bundle_fname" debugme tm_svrty_good "Ok " debugme tmln_out "${verify_retcode[i]}" else @@ -6977,6 +6996,7 @@ certificate_info() { spaces=" " fi + GOOD_CA_BUNDLE="" cert_sig_algo="$(awk -F':' '/Signature Algorithm/ { print $2; if (++Match >= 1) exit; }' <<< "$cert_txt")" cert_sig_algo="${cert_sig_algo// /}" cert_key_algo="$(awk -F':' '/Public Key Algorithm:/ { print $2; if (++Match >= 1) exit; }' <<< "$cert_txt")" @@ -7577,7 +7597,7 @@ certificate_info() { else if [[ $(count_lines "$crl") -eq 1 ]]; then out "$crl" - if [[ "$expfinding" != "expired" ]]; then + if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]]; then check_revocation_crl "$crl" "cert_crlRevoked${json_postfix}" ret=$((ret +$?)) fi @@ -7591,7 +7611,7 @@ certificate_info() { out "$spaces" fi out "$line" - if [[ "$expfinding" != "expired" ]]; then + if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]]; then check_revocation_crl "$line" "cert_crlRevoked${json_postfix}" ret=$((ret +$?)) fi @@ -7610,7 +7630,8 @@ certificate_info() { else if [[ $(count_lines "$ocsp_uri") -eq 1 ]]; then out "$ocsp_uri" - if [[ "$expfinding" != "expired" ]]; then + if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]] && \ + grep -q "\-\-\-\-\-BEGIN CERTIFICATE\-\-\-\-\-" $TEMPDIR/intermediatecerts.pem; then check_revocation_ocsp "$ocsp_uri" "cert_ocspRevoked${json_postfix}" fi ret=$((ret +$?)) @@ -7624,7 +7645,8 @@ certificate_info() { out "$spaces" fi out "$line" - if [[ "$expfinding" != "expired" ]]; then + if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]] && \ + grep -q "\-\-\-\-\-BEGIN CERTIFICATE\-\-\-\-\-" $TEMPDIR/intermediatecerts.pem; then check_revocation_ocsp "$line" "cert_ocspRevoked${json_postfix}" ret=$((ret +$?)) fi From 02d1071b9c6275292ea173c36b8c0c166c07747b Mon Sep 17 00:00:00 2001 From: David Cooper Date: Wed, 30 May 2018 08:55:15 -0400 Subject: [PATCH 2/2] Reduce redundant code Move some checks into functions so that the code doesn't have to be repeated. --- testssl.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/testssl.sh b/testssl.sh index eabffa9..32ec3aa 100755 --- a/testssl.sh +++ b/testssl.sh @@ -1456,6 +1456,7 @@ check_revocation_crl() { local -i success "$PHONE_OUT" || return 0 + [[ -n "$GOOD_CA_BUNDLE" ]] || return 0 scheme="$(tolower "${crl%%://*}")" # The code for obtaining CRLs only supports LDAP, HTTP, and HTTPS URLs. [[ "$scheme" == "http" ]] || [[ "$scheme" == "https" ]] || [[ "$scheme" == "ldap" ]] || return 0 @@ -1520,6 +1521,8 @@ check_revocation_ocsp() { local host_header="" "$PHONE_OUT" || return 0 + [[ -n "$GOOD_CA_BUNDLE" ]] || return 0 + grep -q "\-\-\-\-\-BEGIN CERTIFICATE\-\-\-\-\-" $TEMPDIR/intermediatecerts.pem || return 0 tmpfile=$TEMPDIR/${NODE}-${NODEIP}.${uri##*\/} || exit $ERR_FCREATE host_header=${uri##http://} host_header=${host_header%%/*} @@ -7597,7 +7600,7 @@ certificate_info() { else if [[ $(count_lines "$crl") -eq 1 ]]; then out "$crl" - if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]]; then + if [[ "$expfinding" != "expired" ]]; then check_revocation_crl "$crl" "cert_crlRevoked${json_postfix}" ret=$((ret +$?)) fi @@ -7611,7 +7614,7 @@ certificate_info() { out "$spaces" fi out "$line" - if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]]; then + if [[ "$expfinding" != "expired" ]]; then check_revocation_crl "$line" "cert_crlRevoked${json_postfix}" ret=$((ret +$?)) fi @@ -7630,8 +7633,7 @@ certificate_info() { else if [[ $(count_lines "$ocsp_uri") -eq 1 ]]; then out "$ocsp_uri" - if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]] && \ - grep -q "\-\-\-\-\-BEGIN CERTIFICATE\-\-\-\-\-" $TEMPDIR/intermediatecerts.pem; then + if [[ "$expfinding" != "expired" ]]; then check_revocation_ocsp "$ocsp_uri" "cert_ocspRevoked${json_postfix}" fi ret=$((ret +$?)) @@ -7645,8 +7647,7 @@ certificate_info() { out "$spaces" fi out "$line" - if [[ "$expfinding" != "expired" ]] && [[ -n "$GOOD_CA_BUNDLE" ]] && \ - grep -q "\-\-\-\-\-BEGIN CERTIFICATE\-\-\-\-\-" $TEMPDIR/intermediatecerts.pem; then + if [[ "$expfinding" != "expired" ]]; then check_revocation_ocsp "$line" "cert_ocspRevoked${json_postfix}" ret=$((ret +$?)) fi