fix segfault in ngx_http_auth_ldap_ssl_handshake_handler and add "chain" option for ssl_check_cert #237

mmguero · 2020-05-05T17:43:43Z

This pull request addresses two things relating to ngx_http_auth_ldap_ssl_handshake_handler:

Fixes segmentation fault in ngx_http_auth_ldap_ssl_handshake_handler with ssl_check_cert and ssl_ca_dir #236 by not assuming conn->sockaddr is non-NULL. Instead, it tries to get it from conn->sockaddr, c->conn.sockaddr, and finally c->server->parsed_url.sockaddr.sockaddr, in that order:

struct sockaddr *conn_sockaddr = NULL;
if (conn->sockaddr != NULL) conn_sockaddr = conn->sockaddr;
else if (c->conn.sockaddr != NULL) conn_sockaddr = c->conn.sockaddr;
else conn_sockaddr = &c->server->parsed_url.sockaddr.sockaddr;

Additionally, it uses the correct AF_INET and AF_INET6 constants for checking sa_family for IPv4 vs. IPv6, rather than the incorrect 4 and 6 integer literals.

ssl_check_cert now accepts chain in addition to on and off:

on - full certificate verification in ngx_http_auth_ldap_ssl_handshake_handler, meaning checking the certificate chain with SSL_get_verify_result and X509_check_host or X509_check_ip (same as previous on behavior; full can also be used to mean the same thing as on)
off - no certificate verification (same as previous off behavior)
chain - perform certificate chain verification with SSL_get_verify_result, but don't do host/IP verification (i.e., don't call X509_check_host or X509_check_ip)

This should not break backwards compatibility. I've been testing it in conjunction with an issue in my project (idaholab/Malcolm#128) and it addresses my needs. I've left the checks in place for OPENSSL_VERSION_NUMBER to leave the warnings in place if compiled with too old a version of openssl.

Let me know if you've got any issues or questions.

This reverts commit bf64cf2, reversing changes made to f022103. This change isn't right -- it an LDAP setup when `group_attribute_is_dn on` is enabled, which is what this section of code (kvspb@bf64cf2#diff-c05c0daefb48996cbf510b81002b49bcR2230) is conditionally targeting. This original PR kvspb#199 changed the underlying LDAP query (eg `user_val`) from looking up the user's DN as a group attribute in LDAP (eg set via the `group_attribute` directive in nginx) to looking up the _group's_ DN, which isn't right and won't work. This PR reverts the previous change to make this work correctly again. Fwiw, the originally-referenced issue kvspb#180 seems to be a completely different issue, relating to escaping and parentheses.

…on) or chain (verify cert chain but not hostname/IP)

…l_handshake_handler with ssl_check_cert and ssl_ca_dir

mmguero

I've outlined all of the code changes for the PR, let me know if you have any concerns or questions.

mmguero · 2020-05-05T17:44:36Z

ngx_http_auth_ldap_module.c

@@ -26,6 +26,7 @@
 * SUCH DAMAGE.
 */

+#include <sys/socket.h>


Added sys/socket.h for AF_INET and AF_INET6 later in ngx_http_auth_ldap_ssl_handshake_handler

mmguero · 2020-05-05T17:45:37Z

ngx_http_auth_ldap_module.c

+#define SSL_CERT_VERIFY_OFF    0
+#define SSL_CERT_VERIFY_FULL   1
+#define SSL_CERT_VERIFY_CHAIN  2
+


These constants are used as possible values for ngx_http_auth_ldap_server_t.ssl_check_cert to indicate off, on/full and chain verification as described in the PR body.

mmguero · 2020-05-05T17:46:56Z

ngx_http_auth_ldap_module.c

+          "http_auth_ldap: 'ssl_cert_check': cannot verify remote certificate's domain name because "
+          "your version of OpenSSL is too old. "
+          "Please install OpenSSL >= 1.0.2 and recompile nginx.");
+        #endif


Leave directives for checking OPENSSL_VERSION_NUMBER in place. If ssl_check_cert is specified, on and full indicate full certificate checking (chain and host/IP), chain indicates checking certificate chain only but not host/IP, and anything else means off (don't verify the certificate).

mmguero · 2020-05-05T17:49:08Z

ngx_http_auth_ldap_module.c

-            else { // very unlikely indeed
-              ngx_http_auth_ldap_close_connection(c);
-              return;
+          if (c->server->ssl_check_cert == SSL_CERT_VERIFY_CHAIN) {


handle chain (SSL_CERT_VERIFY_CHAIN) case: we've already called SSL_get_verify_result and got chain_verified assigned, which will be checked below. Set addr_verified to 1 (success) so it will get ignored in the checks later. Don't call X509_check_host or X509_check_ip.

mmguero · 2020-05-05T17:50:39Z

ngx_http_auth_ldap_module.c

+            if (!addr_verified) { // domain not in cert? try IP
+              size_t len; // get IP length
+
+              struct sockaddr *conn_sockaddr = NULL;


This next few lines of code was what was causing the segfault in kvspb/nginx-auth-ldap/#236. conn->sockaddr was always NULL for me. So check a few places that (I think) should have the value we need and use one of them, if set. If there's anywhere my logic might be wrong, it would be here, in assuming the values of these sockaddr structs represent the same thing. But I think it's right.

mmguero · 2020-05-05T17:51:13Z

ngx_http_auth_ldap_module.c

+
+              if (conn_sockaddr->sa_family == AF_INET) len = 4;
+              else if (conn_sockaddr->sa_family == AF_INET6) len = 16;
+              else { // very unlikely indeed


We are now checking AF_INET and AF_INET6 rather than just 4 and 6, which wouldn't have ever been correct anyway.

mmguero · 2020-05-05T17:51:30Z

ngx_http_auth_ldap_module.c

+                ngx_http_auth_ldap_close_connection(c);
+                return;
+              }
+              addr_verified = X509_check_ip(cert, (const unsigned char*)conn_sockaddr->sa_data, len, 0);


See the comment above where I assign conn_sockaddr

mmguero · 2020-05-05T17:51:50Z

ngx_http_auth_ldap_module.c

-                    update counter (this is always reset in 
-                    ngx_http_auth_ldap_connect() for a successful ldap 
-                    connection  
+                c->server->max_down_retries_count < c->server->max_down_retries) {


this is just my editor stripping trailing spaces

mmguero · 2020-05-05T17:51:57Z

ngx_http_auth_ldap_module.c

@@ -1542,7 +1566,7 @@ ngx_http_auth_ldap_read_handler(ngx_event_t *rev)
                // timer call to this read handler again
                ngx_http_auth_ldap_reconnect_handler(rev);
                return;
-            } 
+            }


this is just my editor stripping trailing spaces

This patches in kvspb/nginx-auth-ldap#237 to handle segfaults produced by failure to make SSL connections to LDAP servers.

mmguero · 2022-07-13T21:00:08Z

ping... project still active?

PR: kvspb#237 from mmguero-dev

mmguero added 6 commits April 20, 2020 14:54

Merge remote-tracking branch 'jcu-eresearch/fix-group-lookup'

0c9d333

Merge branch 'master' of https://github.com/kvspb/nginx-auth-ldap

bbeabad

Allow ssl_check_cert to be on (full verification), off (no verificati…

456514a

…on) or chain (verify cert chain but not hostname/IP)

testing fix for kvspb#236 segmentation fault in ngx_http_auth_ldap_ss…

3686bd0

…l_handshake_handler with ssl_check_cert and ssl_ca_dir

fix compiler error

61b70e4

mmguero commented May 5, 2020

View reviewed changes

mmguero mentioned this pull request May 5, 2020

segmentation fault in ngx_http_auth_ldap_ssl_handshake_handler with ssl_check_cert and ssl_ca_dir #236

Open

davidjb added a commit to jcu-eresearch/nginx-dynamic-modules that referenced this pull request Nov 19, 2020

Update ldap module to handle segfaults

86f569a

This patches in kvspb/nginx-auth-ldap#237 to handle segfaults produced by failure to make SSL connections to LDAP servers.

mmguero mentioned this pull request Mar 24, 2022

add this into docker container? #243

Open

Ericbla mentioned this pull request Jul 26, 2022

PR: #237 from mmguero-dev Ericbla/nginx-auth-ldap#1

Merged

Ericbla added a commit to Ericbla/nginx-auth-ldap that referenced this pull request Jul 26, 2022

Merge pull request #1 from mmguero-dev/master

5cc107c

PR: kvspb#237 from mmguero-dev

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix segfault in ngx_http_auth_ldap_ssl_handshake_handler and add "chain" option for ssl_check_cert #237

fix segfault in ngx_http_auth_ldap_ssl_handshake_handler and add "chain" option for ssl_check_cert #237

mmguero commented May 5, 2020

mmguero left a comment

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero May 5, 2020

mmguero commented Jul 13, 2022

fix segfault in ngx_http_auth_ldap_ssl_handshake_handler and add "chain" option for ssl_check_cert #237

Are you sure you want to change the base?

fix segfault in ngx_http_auth_ldap_ssl_handshake_handler and add "chain" option for ssl_check_cert #237

Conversation

mmguero commented May 5, 2020

mmguero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmguero commented Jul 13, 2022