Full_Name: Jan Vcelak Version: git master OS: Linux URL: ftp://ftp.openldap.org/incoming/jvcelak-120412-moznss-hostname-check.patch Submission from: (NULL) (209.132.186.34) Hello. With Mozilla NSS crypto backend, the 'tls_checkpeer no' option in 'sudo' tool does not work. I have uploaded a patch which fixes the bug. But some explanation is necessary: sudo initially creates a ldap handle using ldap_initialize() function. Then it uses ldap_set_option() to set all the settings. The TLS configuration is applied without passing a LDAP handle, which means the the global options are changed. In ldap_int_start_tls(): - The ldap_int_tls_connect() is called and the global TLS context is used (LDAP_INT_GLOBAL_OPT()->ldo_tls_ctx). Which is correct. - The ldap_pvt_tls_check_hostname() is called always. "sudo" will set the ldo_tls_require_cert in global options to LDAP_OPT_X_TLS_NEVER, but the handle options (ld->ld_options) are used to check whether the verification is supposed to happen. And ldo_tls_require_cert in handle options is LDAP_OPT_X_TLS_DEMAND (the default), because it was copied from the global options at the time when ldap_initialize() was called. - With OpenSSL, hostname check (tlso_session_chkhost) will succeed because tlso_get_cert() will return NULL. This is true because no certificate was requested when connecting. We do not request the certificate as well in Mozilla NSS, but it seems that the peer can send it anyway and the library will accept and store it. In this case, hostname check (tlsm_session_chkhost) might fail because SSL_PeerCertificate() will return it. I've added an additional check, which skips the hostname verification within tlsm_session_chkhost if we do not request the certificate validation. With this patch, the behavior is consistent with OpenSSL. (Maybe ldap_pvt_tls_check_hostname() should not just be called, because the global context was used and therefore we should use global options. Not sure what is expected, but I think that changing this might break some applications. My fix is safer.) Jan
> ftp://ftp.openldap.org/incoming/jvcelak-120412-moznss-hostname-check.patch The attached file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by Red Hat. Red Hat has not assigned rights and/or interest in this work to any party. I, Jan Vcelak am authorized by Red Hat, my employer, to release this work under the following terms. Red Hat hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice.
jvcelak@redhat.com wrote: > Full_Name: Jan Vcelak > Version: git master > OS: Linux > URL: ftp://ftp.openldap.org/incoming/jvcelak-120412-moznss-hostname-check.patch > Submission from: (NULL) (209.132.186.34) > > > Hello. > > With Mozilla NSS crypto backend, the 'tls_checkpeer no' option in 'sudo' tool > does not work. I have uploaded a patch which fixes the bug. > > But some explanation is necessary: > > sudo initially creates a ldap handle using ldap_initialize() function. Then it > uses ldap_set_option() to set all the settings. The TLS configuration is applied > without passing a LDAP handle, which means the the global options are changed. Sounds like a simple sequencing bug then. Just initialize the global options before the first ldap_initialize() call. > In ldap_int_start_tls(): > > - The ldap_int_tls_connect() is called and the global TLS context is used > (LDAP_INT_GLOBAL_OPT()->ldo_tls_ctx). Which is correct. > > - The ldap_pvt_tls_check_hostname() is called always. "sudo" will set the > ldo_tls_require_cert in global options to LDAP_OPT_X_TLS_NEVER, but the handle > options (ld->ld_options) are used to check whether the verification is supposed > to happen. And ldo_tls_require_cert in handle options is LDAP_OPT_X_TLS_DEMAND > (the default), because it was copied from the global options at the time when > ldap_initialize() was called. > > - With OpenSSL, hostname check (tlso_session_chkhost) will succeed because > tlso_get_cert() will return NULL. This is true because no certificate was > requested when connecting. We do not request the certificate as well in Mozilla > NSS, but it seems that the peer can send it anyway and the library will accept > and store it. In this case, hostname check (tlsm_session_chkhost) might fail > because SSL_PeerCertificate() will return it. > > I've added an additional check, which skips the hostname verification within > tlsm_session_chkhost if we do not request the certificate validation. With this > patch, the behavior is consistent with OpenSSL. > > (Maybe ldap_pvt_tls_check_hostname() should not just be called, because the > global context was used and therefore we should use global options. Not sure > what is expected, but I think that changing this might break some applications. > My fix is safer.) > > Jan > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
> Sounds like a simple sequencing bug then. Just initialize the global > options > before the first ldap_initialize() call. Sudo parses the options in config file and stores them in a table: http://www.sudo.ws/repos/sudo/file/6fa11e8448b9/plugins/sudoers/ldap.c#l225 This table is then iterated and all options are being set. The problem is that some options are set with LDAP handle provided and some are not. This means that the handle has to be created before. The change proposed by you would require the change of this well-arranged and transparent concept. It can be a sequencing bug, but this particular situation is not described anywhere. And OpenSSL has a different behavior. My patch updates Mozilla NSS backend to behave the same as OpenSSL backend. I still think this should be fixed in OpenLDAP rather than in sudo. Jan
Jan Vcelak wrote: >> Sounds like a simple sequencing bug then. Just initialize the global >> options >> before the first ldap_initialize() call. > > Sudo parses the options in config file and stores them in a table: > http://www.sudo.ws/repos/sudo/file/6fa11e8448b9/plugins/sudoers/ldap.c#l225 > > This table is then iterated and all options are being set. The > problem is that some options are set with LDAP handle provided > and some are not. This means that the handle has to be created > before. The change proposed by you would require the change of > this well-arranged and transparent concept. "Elegance" of code does not make it less wrong. The config is simply driven as a list. They can very easily fix this so that sudo_ldap_set_options() is called twice, first with a NULL ld and only the non-connection-oriented parameters, then create the LDAP handle and call again to set the connection-oriented parameters. > It can be a sequencing bug, but this particular situation is not > described anywhere. Perhaps that should be raised as a separate doc bug then. Global options are copied to a handle at the time the handle is created. Any options not explicitly set globally at that time are set to their default value. > And OpenSSL has a different behavior. My patch > updates Mozilla NSS backend to behave the same as OpenSSL backend. > I still think this should be fixed in OpenLDAP rather than in sudo. No. Your patch fixes one possible wrong outcome, but the sudo approach is still fundamentally wrong and if we only patch this one instance, someone else in the future is bound to trip over the sequencing problem again. Fix the right bug, otherwise you will have to keep fixing the wrong bugs over and over again. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
> No. Your patch fixes one possible wrong outcome, but the sudo > approach is > still fundamentally wrong and if we only patch this one instance, > someone else > in the future is bound to trip over the sequencing problem again. Fix > the > right bug, otherwise you will have to keep fixing the wrong bugs over > and over > again. OK, thanks for confirmation. I will report a bug for sudo. Jan
changed notes changed state Open to Test moved from Incoming to Documentation
> OK, thanks for confirmation. I will report a bug for sudo. http://www.sudo.ws/bugs/show_bug.cgi?id=554
changed notes changed state Test to Closed
doc updated in master doc updated in RE24