Several OCSP improvements

This MR solves two major pain points on the OpenSSL backend. We move on-line OCSP verification out of the openssl_revocation_check_fn callback, and we take into account stapled responses sent by the server and avoid sending OCSP requests for those (#578).

We are doing OCSP verification in the openssl_revocation_check_fn callback. This callback is not the best place to check OCSP because the peer's X.509 cert stack that we get there is incomplete, and cannot be fully trusted. Hence, we move OCSP checking to the end of the wget_ssl_open function, just after the TLS handshake has successfully completed. At that point we do have the full, verified, cert stack (can be obtained with SSL_get0_verified_chain).

In addition, the fact that the on-line OCSP checks for the certificates were being carried out after the handshake completed caused that any stapled OCSP response sent by the server was not taking into account (all certificates were being OCSP-checked regardless of there was a stapled response for any of them or not). This was also happening before, when we used the openssl_revocation_check_fn callback, because it was always called after the OCSP verification callback. Hence, we create a vector and store all the stapled OCSP responses we receive, and then, during on-line OCSP verification, we check if a stapled response exists for each certificate before contacting OCSP servers.

Approver's checklist:

• The author has submitted the FSF Copyright Assignment and is listed in AUTHORS
• There is a test suite reasonably covering new functionality or modifications
• Function naming, parameters, return values, types, etc., are consistent with existing code
• This feature/change has adequate documentation added (if appropriate)
• No obvious mistakes / misspelling in the code

requested review from @rockdaboot

assigned to @juaristi

marked the checklist item The author has submitted the FSF Copyright Assignment and is listed in AUTHORS as completed

Thanks Ander ! I just reviewed the code (overall LGTM) and would build/test when the comments are addressed.

added 2 commits

• 0c3afa4d - Remove unused variables
• b81a0474 - Rewrite find_issuer_cert

Compare with previous version

resolved all threads

@rockdaboot Would you please review again?

added 13 commits

• b81a0474...c43d1d8f - 8 commits from branch master
• cc403a3b - Check OCSP at the end of handshake
• 6305d911 - Better way to find issuer cert
• cca5ca0b - Do not request on-line OCSP for stapled responses
• ef624e06 - Remove unused variables
• bc9f9a8a - Rewrite find_issuer_cert

Compare with previous version

@juaristi Code looks good, but could you fix the (gcc) warnings ?

ssl_openssl.c:584:21: warning: function declaration isn't a prototype [-Wstrict-prototypes]
  584 | static wget_vector *ocsp_create_stapled_response_vector()
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ssl_openssl.c: In function 'ocsp_create_stapled_response_vector':
ssl_openssl.c:584:21: warning: old-style function definition [-Wold-style-definition]
ssl_openssl.c: In function 'ocsp_lookup_in_cache':
ssl_openssl.c:639:31: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  639 |                 *cache_origin = "stapled";
      |                               ^
ssl_openssl.c:653:39: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  653 |                         *cache_origin = "cached";

clang-15 is even more picky, but we can care about this later:

ssl_openssl.c:530:28: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
        unsigned cert_list_size = sk_X509_num(certs);
                                  ^
/usr/include/openssl/x509.h:76:40: note: expanded from macro 'sk_X509_num'
#define sk_X509_num(sk) OPENSSL_sk_num(ossl_check_const_X509_sk_type(sk))
                                       ^
ssl_openssl.c:533:10: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
                cert = sk_X509_value(certs, i);
                       ^
/usr/include/openssl/x509.h:77:58: note: expanded from macro 'sk_X509_value'
#define sk_X509_value(sk, idx) ((X509 *)OPENSSL_sk_value(ossl_check_const_X509_sk_type(sk), (idx)))
                                                         ^
ssl_openssl.c:584:56: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
static wget_vector *ocsp_create_stapled_response_vector()
                                                       ^
                                                        void
ssl_openssl.c:1063:19: warning: 'ossl_check_const_OPENSSL_STRING_sk_type' was marked unused but was used [-Wused-but-marked-unused]
        if (str_stack && sk_OPENSSL_STRING_num(str_stack) > 0) {
                         ^
/usr/include/openssl/safestack.h:206:50: note: expanded from macro 'sk_OPENSSL_STRING_num'
#define sk_OPENSSL_STRING_num(sk) OPENSSL_sk_num(ossl_check_const_OPENSSL_STRING_sk_type(sk))
                                                 ^
ssl_openssl.c:1064:27: warning: 'ossl_check_const_OPENSSL_STRING_sk_type' was marked unused but was used [-Wused-but-marked-unused]
                char *uri = wget_strdup(sk_OPENSSL_STRING_value(str_stack, 0));
                                        ^
/usr/include/openssl/safestack.h:207:68: note: expanded from macro 'sk_OPENSSL_STRING_value'
#define sk_OPENSSL_STRING_value(sk, idx) ((char *)OPENSSL_sk_value(ossl_check_const_OPENSSL_STRING_sk_type(sk), (idx)))
                                                                   ^
ssl_openssl.c:1134:29: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
        unsigned cert_chain_size = sk_X509_num(certs), next = starting_idx;
                                   ^
/usr/include/openssl/x509.h:76:40: note: expanded from macro 'sk_X509_num'
#define sk_X509_num(sk) OPENSSL_sk_num(ossl_check_const_X509_sk_type(sk))
                                       ^
ssl_openssl.c:1138:15: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
                candidate = sk_X509_value(certs, next);
                            ^
/usr/include/openssl/x509.h:77:58: note: expanded from macro 'sk_X509_value'
#define sk_X509_value(sk, idx) ((X509 *)OPENSSL_sk_value(ossl_check_const_X509_sk_type(sk), (idx)))
                                                         ^
ssl_openssl.c:1156:28: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
        unsigned cert_list_size = sk_X509_num(certs);
                                  ^
/usr/include/openssl/x509.h:76:40: note: expanded from macro 'sk_X509_num'
#define sk_X509_num(sk) OPENSSL_sk_num(ossl_check_const_X509_sk_type(sk))
                                       ^
ssl_openssl.c:1159:10: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
                cert = sk_X509_value(certs, i);
                       ^
/usr/include/openssl/x509.h:77:58: note: expanded from macro 'sk_X509_value'
#define sk_X509_value(sk, idx) ((X509 *)OPENSSL_sk_value(ossl_check_const_X509_sk_type(sk), (idx)))
                                                         ^
ssl_openssl.c:1199:11: warning: variable 'fingerprint' may be uninitialized when used here [-Wconditional-uninitialized]
                                xfree(fingerprint);
                                      ^~~~~~~~~~~
./private.h:48:27: note: expanded from macro 'xfree'
#define xfree(a) do { if (a) { wget_free((void *)(a)); a=NULL; } } while (0)
                          ^
ssl_openssl.c:1153:15: note: initialize the variable 'fingerprint' to silence this warning
                *fingerprint,
                            ^
                             = NULL
ssl_openssl.c:1281:28: warning: 'ossl_check_const_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
        vflags->cert_chain_size = sk_X509_num(certs);
                                  ^
/usr/include/openssl/x509.h:76:40: note: expanded from macro 'sk_X509_num'
#define sk_X509_num(sk) OPENSSL_sk_num(ossl_check_const_X509_sk_type(sk))
                                       ^
ssl_openssl.c:1293:2: warning: 'ossl_check_X509_sk_type' was marked unused but was used [-Wused-but-marked-unused]
        sk_X509_pop_free(certs, X509_free);
        ^
/usr/include/openssl/x509.h:90:60: note: expanded from macro 'sk_X509_pop_free'
#define sk_X509_pop_free(sk, freefunc) OPENSSL_sk_pop_free(ossl_check_X509_sk_type(sk),ossl_check_X509_freefunc_type(freefunc))
                                                           ^
ssl_openssl.c:1293:2: warning: 'ossl_check_X509_freefunc_type' was marked unused but was used [-Wused-but-marked-unused]
/usr/include/openssl/x509.h:90:88: note: expanded from macro 'sk_X509_pop_free'
#define sk_X509_pop_free(sk, freefunc) OPENSSL_sk_pop_free(ossl_check_X509_sk_type(sk),ossl_check_X509_freefunc_type(freefunc))
                                                                                       ^
ssl_openssl.c:1732:13: warning: 'ERR_GET_REASON' was marked unused but was used [-Wused-but-marked-unused]
                retval = (ERR_GET_REASON(ERR_peek_last_error()) == SSL_R_CERTIFICATE_VERIFY_FAILED ?
                          ^

HTTPS checks are failing, reproducible with

make='make -j$(nproc)'
./configure --with-ssl=openssl
make clean
make
make check-valgrind

e.g. from tests/test-ocsp-server.log:

==73655== Invalid read of size 1
==73655==    at 0x4849794: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==73655==    by 0x48672C4: copy_string (buffer_printf.c:65)
==73655==    by 0x48677D3: wget_buffer_vprintf_append (buffer_printf.c:455)
==73655==    by 0x4867963: wget_buffer_vprintf (buffer_printf.c:522)
==73655==    by 0x48768D2: logger_vprintf_func (logger.c:46)
==73655==    by 0x487681B: wget_debug_printf (log.c:105)
==73655==    by 0x488151D: ssl_set_alpn_selected_protocol (ssl_openssl.c:1539)
==73655==    by 0x4881E5D: wget_ssl_open (ssl_openssl.c:1772)
==73655==    by 0x4878358: wget_tcp_connect (net.c:761)
==73655==    by 0x4871DD8: wget_http_open (http.c:657)
==73655==    by 0x119370: try_connection (wget.c:1555)
==73655==    by 0x1195BA: establish_connection (wget.c:1626)
==73655==  Address 0x6b1e5d8 is 0 bytes after a block of size 8 alloc'd
==73655==    at 0x48437B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==73655==    by 0x4DC46B0: ??? (in /usr/lib/x86_64-linux-gnu/libssl.so.3)
==73655==    by 0x4DC03DF: ??? (in /usr/lib/x86_64-linux-gnu/libssl.so.3)
==73655==    by 0x4DD27CE: ??? (in /usr/lib/x86_64-linux-gnu/libssl.so.3)
==73655==    by 0x4DCADB1: ??? (in /usr/lib/x86_64-linux-gnu/libssl.so.3)
==73655==    by 0x4881D8D: wget_ssl_open (ssl_openssl.c:1714)
==73655==    by 0x4878358: wget_tcp_connect (net.c:761)
==73655==    by 0x4871DD8: wget_http_open (http.c:657)
==73655==    by 0x119370: try_connection (wget.c:1555)
==73655==    by 0x1195BA: establish_connection (wget.c:1626)
==73655==    by 0x121FE8: downloader_thread (wget.c:2311)
==73655==    by 0x4A7B849: start_thread (pthread_create.c:442)

Not sure if this is due to your changes, but it stems from openssl code.

This openssl bug indeed may be older, see also https://github.com/rockdaboot/wget2/issues/265#issuecomment-1264338832

Hm, we need a new CI runner with OpenSSL and valgrind.

Fixed the bug (the above stacktrace) that was triggered by our openssl code: !514 (merged).

Please rebase when the !514 (merged) got merged.

The GH issue is something else.

I assume that the vflags should be per-connection data. But it is used as global data:

store = SSL_CTX_get_cert_store(_ctx);
X509_STORE_set_ex_data(store, store_userdata_idx, (void *) vflags)

What are your thoughts ?

I am fine with opening another issue and get this MR merged independently.

Hi @rockdaboot I can't get any warnings, even though I'm always passing -Wall. Same with clang, I don't see any warnings. Maybe because of versions?

added 14 commits

• bc9f9a8a...b8839418 - 3 commits from branch master
• d53e5776 - Check OCSP at the end of handshake
• ddd5d53a - Better way to find issuer cert
• c8991a15 - Do not request on-line OCSP for stapled responses
• ba9a80e9 - Remove unused variables
• be1884fd - Rewrite find_issuer_cert
• 7266e59a - Fix memory leak
• 8ab8dfc9 - Check HPKP at the end
• 4bd89be2 - Fix: Pass per-connection user data to OCSP callback
• 29340765 - Remove unneeded ex_data indexes
• 83282176 - Fix warnings
• 8e25ab36 - Remove unused fields from vflags

Compare with previous version