| Summary: | SSLVerifyClient optional_no_ca is broken | ||
|---|---|---|---|
| Product: | Apache httpd-2 | Reporter: | Benjamin Dauvergne <bdauvergne> |
| Component: | mod_ssl | Assignee: | Apache HTTPD Bugs Mailing List <bugs> |
| Status: | NEW --- | ||
| Severity: | normal | CC: | apache-bugzilla, arnis, sascha-web-issues.apache.org, tonibony |
| Priority: | P2 | Keywords: | PatchAvailable |
| Version: | 2.5-HEAD | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Attachments: |
Patch for 2.2.x
Patch for trunk |
||
I've run into the same problem. If 'SSLVerifyClient optional_no_ca' is used, the SSL_CLIENT_VERIFY environment variable will contain either 'SUCCESS' or 'FAILED:' (with no error string). It will never contain 'GENEROUS', as stated by the documentation. As Benjamin stated, this happens because ssl_callback_SSLVerify() in ssl_engine_kernel.c only sets sslconn->verify_info = "GENEROUS" if validation failed with an "optional" error, and only sets sslconn->verify_error if optional_no_ca is used and validation failed with a non-optional error. Thus, the 'if (vrc == X509_V_OK && vinfo != NULL && strEQ(vinfo, "GENEROUS"))' statement in ssl_var_lookup_ssl_cert_verify() in ssl_engine_vars.c will never match because vinfo will not have been set if vrc is set to X509_V_OK, and the FAILED message will not contain an error string since sslconn->verify_error was never set. I get the sense that the original mistake was in the if statement in ssl_engine_vars.c, so the second approach mentioned by Benjamin is probably the best fix. However, the ssl_verify_error_is_optional(vrc) check that Benjamin used isn't actually needed, since sslconn->verify_info will not be set if the error is optional, so you only really need to check for sslconn->verify_info == GENEROUS. I'm attaching two patches (one for 2.2.x, and one for trunk) to correct this. Hopefully someone will apply them, as this is a simple but annoying bug. Created attachment 25278 [details]
Patch for 2.2.x
Created attachment 25279 [details]
Patch for trunk
BTW, I've attached a patch to bug #45922 which includes this fix and also provides the verification error message (only one error message if multiple verification failures occurred) when returning a GENEROUS result. Current possible SSL_CLIENT_VERIFY values when SSLVerifyClient set to optional_no_ca: NONE: if empty client certificate message sent. SUCCESS: successful authentication when client certificate chains up to a trusted root. GENEROUS: initial negotiation when client certificate does not chain up to a trusted root. SUCCESS: when resuming GENEROUS session (bug #53193). FAILED:(null): renegotiation when client certificate does not chain up to a trusted root. |
The documented behavious for the option 'optional_no_ca' is that if the return code of SSL_Accept valid this predicate: ssl_private.h:#define ssl_verify_error_is_optional(errnum) \ ssl_private.h- ((errnum == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) \ ssl_private.h- || (errnum == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN) \ ssl_private.h- || (errnum == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY) \ ssl_private.h- || (errnum == X509_V_ERR_CERT_UNTRUSTED) \ ssl_private.h- || (errnum == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE)) then the error code in SSL_VERIFY_CLIENT should be "FAILED:(an error message)" but "GENEROUS". This functionality is very useful to use certificate as an user-centric authentication token, why forbid it with this code from ssl_engine_io.c: if (ssl_verify_error_is_optional(verify_result) && (sc->server->auth.verify_mode == SSL_CVERIFY_OPTIONAL_NO_CA)) { /* leaving this log message as an error for the moment, * according to the mod_ssl docs: * "level optional_no_ca is actually against the idea * of authentication (but can be used to establish * SSL test pages, etc.)" * optional_no_ca doesn't appear to work as advertised * in 1.x */ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, "SSL client authentication failed, " "accepting certificate based on " "\"SSLVerifyClient optional_no_ca\" " "configuration"); ssl_log_ssl_error(APLOG_MARK, APLOG_INFO, c->base_server); } look that we don't reset the result to something good like X509_V_OK. And this code that return the value of SSL_VERIFY_CLIENT: if (vrc == X509_V_OK && verr == NULL && vinfo == NULL && xs == NULL) /* no client verification done at all */ result = "NONE"; else if (vrc == X509_V_OK && verr == NULL && vinfo == NULL && xs != NULL) /* client verification done successful */ result = "SUCCESS"; else if (vrc == X509_V_OK && vinfo != NULL && strEQ(vinfo, "GENEROUS")) /* client verification done in generous way */ result = "GENEROUS"; else /* client verification failed */ result = apr_psprintf(p, "FAILED:%s", verr); if (xs) X509_free(xs); The third condition can never happen as vrc will be OK and vinfo == GENEROUS at the same time. Two approaches: - reset the result code (we loss information), with a SSL_set_verify_result(ssl, X509_V_OK); inside the first extracted code. - change third condition with this patch: - else if (vrc == X509_V_OK && vinfo != NULL && strEQ(vinfo, "GENEROUS")) + else if (ssl_verify_error_is_optional(vrc) && vinfo != NULL && strEQ(vinfo, "GENEROUS")) - bonus point would be to return the result code inside another variable and completely removing the optional_no_ca option, web app could just use optional and choose actions in function of SSL_VERIFY_CLIENT == FAILED and the error code. What do you think ?