[mod_openssl] ignore client verification error if not enforced

[gportay]

Hi,

Here is the change with every commits squashed in a single commit, as discussed here.

Regards,
Gaël

[gstrauss]

Thanks. That diff is much more readable.

[gportay]

Oops, I pushed a new commit right now... have you already merged it to master?

[gstrauss]

I did push to master, though for some reason the commit hook did not push back to github. It's on git.lighttpd.net.

[gportay]

I did a minor modification to the verify callback.

I reused the err_cert in the if statement.

If it is not too late, you may consider this improvement.

Sorry about that

[gstrauss]

What is the benefit of that change? (Yes, it can be made if it makes sense.)

The patch I took (the earlier patch) followed the example in https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_verify(3)#EXAMPLES and your new commit differs from the example. Would you please explain why?

[gportay]

This benefits are:

1. it reuses the variable instead of calling again a function
2. it compiles with -Werror with the latest version of openssl

[gportay]

I tested with valgrind and there is no leaks and no errors with the latest version of openssl (1.1.0-dev).

==9622== 
==9622== HEAP SUMMARY:
==9622==     in use at exit: 1,736 bytes in 7 blocks
==9622==   total heap usage: 11,208 allocs, 11,201 frees, 889,723 bytes allocated
==9622== 
==9622== Searching for pointers to 7 not-freed blocks
==9622== Checked 186,016 bytes
==9622== 
==9622== LEAK SUMMARY:
==9622==    definitely lost: 0 bytes in 0 blocks
==9622==    indirectly lost: 0 bytes in 0 blocks
==9622==      possibly lost: 0 bytes in 0 blocks
==9622==    still reachable: 1,736 bytes in 7 blocks
==9622==         suppressed: 0 bytes in 0 blocks
==9622== Reachable blocks (those to which a pointer was found) are not shown.
==9622== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==9622== 
==9622== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==9622== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[gstrauss]

it reuses the variable instead of calling again a function

I don't see that in the patch below.

--- a/src/mod_openssl.c
+++ b/src/mod_openssl.c
@@ -237,6 +237,5 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
                           err == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT)) {
-        X509_NAME_oneline(X509_get_issuer_name(ctx->current_cert), buf,
-                          sizeof(buf));
+        X509_NAME_oneline(X509_get_issuer_name(err_cert), buf, sizeof(buf));
         log_error_write(srv, __FILE__, __LINE__, "SS", "SSL: issuer=", buf);
     }
 

it compiles with -Werror with the latest version of openssl

I did not test with latest openssl. What error existed prior to your latest patch?

However, I did accidentally let you insert two literal tabs ('\t') in your patch, in a .c file without literal tabs, so I plan to remove them. Please review the diff of your latest patch with what I committed to 04d510a in https://github.com/gstrauss/lighttpd.1.4

Are there any differences other than what I posted above?

[gportay]

I don't see that in the patch below.

Right, I just reuse the variable err_cert instead of ctx->current_cert which leads to a mismatch type with the newer openssl version (1.1.0).

mod_openssl.c: In function ‘verify_callback’:
mod_openssl.c:239:51: error: dereferencing pointer to incomplete type ‘X509_STORE_CTX {aka struct x509_store_ctx_st}’
         X509_NAME_oneline(X509_get_issuer_name(ctx->current_cert), buf, sizeof(buf));
                                                   ^~

I am fixing the tab and push force on this branch.

[gstrauss]

Instead of force-push, please add a new commit so that I can more easily cherry-pick it. Thanks.

[gportay]

a new commit on top of 04d510a?

[gstrauss]

Just looked at the openssl code. Now i see that err_cert = X509_STORE_CTX_get_current_cert(ctx) is the same result as ctx->current_cert, though that structure is private in openssl 1.1.0.

Thank you for testing with openssl 1.1.0 and for the rest of your thorough testing.

Still, X509_STORE_CTX_get_current_cert() is not present in openssl 1.0.1 (different than 1.1.0), but that interface appears in 1.0.2. Therefore, we need to use ctx->current_cert on versions earlier than openssl 1.0.2.

Also, the openssl manual page notes:

In versions of OpenSSL before 1.0 the current certificate returned by X509_STORE_CTX_get_current_cert() was never NULL. Applications should check the return value before printing out any debugging information relating to the current certificate.

[edit] openssl 1.0.1 reached end-of-life at the end of 2016, but there are still some OS vendors supposedly supporting it until their long term releases reach end-of-life.

[gstrauss]

i need to test this with 1.0.1 before pushing to master:

--- a/src/mod_openssl.c
+++ b/src/mod_openssl.c
@@ -219,10 +219,15 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
     }
 
     if (preverify_ok) {
-       return preverify_ok;
+        return preverify_ok;
     }
 
+  #if OPENSSL_VERSION_NUMBER >= 0x10002000L
     err_cert = X509_STORE_CTX_get_current_cert(ctx);
+  #else
+    err_cert = ctx->current_cert;
+  #endif
+    if (NULL == err_cert) return !hctx->conf.ssl_verifyclient_enforce;
     X509_NAME_oneline(X509_get_subject_name(err_cert), buf, sizeof(buf));
     log_error_write(srv, __FILE__, __LINE__, "SDSSSDSS",
                         "SSL: verify error:num=", err, ":",
@@ -235,8 +241,7 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
      */
     if (!preverify_ok && (err == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY ||
                           err == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT)) {
-        X509_NAME_oneline(X509_get_issuer_name(ctx->current_cert), buf,
-                          sizeof(buf));
+        X509_NAME_oneline(X509_get_issuer_name(err_cert), buf, sizeof(buf));
         log_error_write(srv, __FILE__, __LINE__, "SS", "SSL: issuer=", buf);
     }
 
@@ -574,7 +579,7 @@ network_init_ssl (server *srv, void *p_d)
         }
 
         if (NULL == s->ssl_ca_file_cert_names
-           && !buffer_string_is_empty(s->ssl_ca_file)) {
+            && !buffer_string_is_empty(s->ssl_ca_file)) {
             s->ssl_ca_file_cert_names =
               SSL_load_client_CA_file(s->ssl_ca_file->ptr);
             if (NULL == s->ssl_ca_file_cert_names) {

[gstrauss]

A modification needs to be made to what was committed. X509_NAME_oneline() is deprecated.
The manpage for X509_NAME_oneline() says:

The functions X509_NAME_oneline() and X509_NAME_print() are legacy functions which produce a non standard output form, they don't handle multi character fields and have various quirks and inconsistencies. Their use is strongly discouraged in new applications.

Besides X509_NAME_oneline() function being deprecated, until fairly recently, there was a security issue with the function, too.

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-2176

The X509_NAME_oneline function in crypto/x509/x509_obj.c in OpenSSL before 1.0.1t and 1.0.2 before 1.0.2h allows remote attackers to obtain sensitive information from process stack memory or cause a denial of service (buffer over-read) via crafted EBCDIC ASN.1 data.

Please consider using X509_NAME_print_ex() and propose a reasonable set of flags for a consistent and still-useful result.

The above was also noted in https://redmine.lighttpd.net/issues/2693 and related pull request #63

[gportay]

Okay.

I will check for that.

I am currently on holliday, but I will find a moment to fix and test this issue.

[gstrauss]

Enjoy your holiday. I have what I think is a solution, and I plan to test and push tomorrow.