Bug 217796
Summary: | Inconsistent clear password storage causes SASL errors | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Retired] 389 | Reporter: | Nathan Kinder <nkinder> | ||||||||
Component: | Security - SASL | Assignee: | Nathan Kinder <nkinder> | ||||||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Viktor Ashirov <vashirov> | ||||||||
Severity: | medium | Docs Contact: | |||||||||
Priority: | medium | ||||||||||
Version: | 1.0.4 | CC: | nhosoi, rmeggins | ||||||||
Target Milestone: | --- | ||||||||||
Target Release: | --- | ||||||||||
Hardware: | All | ||||||||||
OS: | Linux | ||||||||||
Whiteboard: | |||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||
Doc Text: | Story Points: | --- | |||||||||
Clone Of: | Environment: | ||||||||||
Last Closed: | 2015-12-07 17:16:14 UTC | Type: | --- | ||||||||
Regression: | --- | Mount Type: | --- | ||||||||
Documentation: | --- | CRM: | |||||||||
Verified Versions: | Category: | --- | |||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||
Embargoed: | |||||||||||
Bug Depends On: | |||||||||||
Bug Blocks: | 152373, 240316, 427409 | ||||||||||
Attachments: |
|
Description
Nathan Kinder
2006-11-29 22:37:42 UTC
Created attachment 142445 [details]
CVS Diffs
Looks good!
> if (pw == strstr( pw, "{clear}" )) {
One silly question. :) I'd like to double check "{CLEAR}" never be used here...
I quickly checked with RFC3112 "LDAP Authentication Password Schema", but I
could not find the answer... :p Do you happen to know where I should check?
I'd also like to point out that the code will fail with a password with the following value: "{xxxxxxxxxxxx{clear}" I think we should use if (!strncmp(pw, "{clear}", 7)) { clear = pw + 7; } else { /* something else - just assume the whole thing is the clear text pw */ clear = pw; } If you need case insensitive, you can use PL_strncasecmp(). (In reply to comment #3) > I think we should use > if (!strncmp(pw, "{clear}", 7)) { > clear = pw + 7; > } else { > /* something else - just assume the whole thing is the clear text pw */ > clear = pw; > } The problem with this else block is that it will allow a client to use a password hash (assuming that's what is stored as the userPassword attribute value) to authenticate with SASL. This seems bad. The whole point of hashing the password is that the hashed password should not be usable for anything other than comparing another hash against it. This does cause passwords such as the example you mentioned to not work with SASL mechanisms such as DIGEST-MD5 and CRAM-MD5 though. (In reply to comment #4) > The problem with this else block is that it will allow a client to use a > password hash (assuming that's what is stored as the userPassword attribute > value) to authenticate with SASL. This seems bad. The whole point of hashing > the password is that the hashed password should not be usable for anything other > than comparing another hash against it. This does cause passwords such as the > example you mentioned to not work with SASL mechanisms such as DIGEST-MD5 and > CRAM-MD5 though. > Ok. Then the original is fine, and you can use PL_strcasestr if you need to do a case insensitive comparison. Created attachment 142641 [details]
Revised Diffs
After quite a bit of RFC reading (particularly 2256, 2307 and 3112) and
discussion with Rich and Noriko, I've come to the conclusion that we shouldn't
be adding the "{clear}" prefix to passwords. This new set of diffs fixes the
issues with SASL being able to use password hash values to authenticate. WHen
authenticating with SASL, the server will ensure that the password is a
clear-text password.
I've also made the server strip the "{clear}" prefix off of the userPassword
attribute value if a client tries to add the prefix itself. This will ensure
that any newly stored passwords will not contain the "{clear}" prefix.
diff -u -5 -t -r1.5 clear_pwd.c --- ldap/servers/plugins/pwdstorage/clear_pwd.c 10 Nov 2006 23:45:10 -0000 1.5 +++ ldap/servers/plugins/pwdstorage/clear_pwd.c 1 Dec 2006 23:50:44 -0000 @@ -58,7 +58,18 @@ } char * clear_pw_enc( char *pwd ) { - return( slapi_ch_strdup( pwd )); + /* Just return NULL if pwd is NULL */ + if (!pwd) + return NULL; + + /* If the modify operation specified the "{clear}" storage scheme + * prefix, we should strip it off. + */ + if ((*pwd == PWD_HASH_PREFIX_START) && PL_strcasestr( pwd, "{clear}" )) { ==> if ((*pwd == PWD_HASH_PREFIX_START) && pwd == PL_strcasestr( pwd, "{clear}" )) { I wonder if we strictly expect to have {<scheme>} as a prefix. If so, I'd feel safer if we check the position of "{clear}"... The rest looks good to me. And it'd be nice if you could open a new bug for implementing authpassword for 7.3 or later (if not yet ;). Created attachment 142888 [details] Revised Diffs This set of revised diffs adds Noriko's suggestion from Comment#7. Checked into ldapserver (HEAD). Thanks to Rich and Noriko for their reviews! Checking in servers/slapd/pw.c; /cvs/dirsec/ldapserver/ldap/servers/slapd/pw.c,v <-- pw.c new revision: 1.15; previous revision: 1.14 done Checking in servers/slapd/saslbind.c; /cvs/dirsec/ldapserver/ldap/servers/slapd/saslbind.c,v <-- saslbind.c new revision: 1.22; previous revision: 1.21 done *** Bug 243792 has been marked as a duplicate of this bug. *** Confirmed that the server no longer stores {clear} in with the password when stored as clear. configured the server by adding the following ldif: dn: cn=config changetype: modify add: passwordstoragescheme passwordstoragescheme: clear then after adding: dn: cn=Kerberos uid mapping,cn=mapping,cn=sasl,cn=config changetype: modify replace: nsSaslMapBaseDNTemplate nsSaslMapBaseDNTemplate: o=sasl.com dn: cn=rfc 2829 dn syntax,cn=mapping,cn=sasl,cn=config changetype: modify replace: nsSaslMapBaseDNTemplate nsSaslMapBaseDNTemplate: o=sasl.com dn: cn=rfc 2829 u syntax,cn=mapping,cn=sasl,cn=config changetype: modify replace: nsSaslMapBaseDNTemplate nsSaslMapBaseDNTemplate: o=sasl.com dn: cn=uid mapping,cn=mapping,cn=sasl,cn=config changetype: modify replace: nsSaslMapBaseDNTemplate nsSaslMapBaseDNTemplate: o=sasl.com I was able to auth with: /usr/lib/mozldap6/ldappasswd -h brandywine.dsqa.sjc2.redhat.com -p 1111 -s 9998 "uid=mgreggtest,ou=people,o=redhat" -a 1234 -m CRAM-M5 -o "mech=CRAM-MD5" -o "authid=ttest1" -w 1234 I was also able to dump the DB and verify that the entries are not stored with {clear} in front of them. Verified aginst: 1199305764 idm-console-framework-1.1.0-7.el4idm Wed Jan 02 2008 1199305765 redhat-ds-console-8.0.0-9.el4dsrv Wed Jan 02 2008 1199305766 redhat-admin-console-8.0.0-9.el4dsrv Wed Jan 02 2008 1199305768 redhat-idm-console-1.0.0-21.el4idm Wed Jan 02 2008 1199390834 net-snmp-libs-5.1.2-11.el4_6.11.1 Thu Jan 03 2008 1199390890 net-snmp-5.1.2-11.el4_6.11.1 Thu Jan 03 2008 1199391027 db4-4.2.52-7.3.el4 Thu Jan 03 2008 1199391030 db4-utils-4.2.52-7.3.el4 Thu Jan 03 2008 1199391031 db4-tcl-4.2.52-7.3.el4 Thu Jan 03 2008 1199391034 db4-java-4.2.52-7.3.el4 Thu Jan 03 2008 1199391044 db4-devel-4.2.52-7.3.el4 Thu Jan 03 2008 1199391134 cyrus-sasl-2.1.19-14 Thu Jan 03 2008 1199391137 cyrus-sasl-md5-2.1.19-14 Thu Jan 03 2008 1199391138 cyrus-sasl-gssapi-2.1.19-14 Thu Jan 03 2008 1199391138 cyrus-sasl-plain-2.1.19-14 Thu Jan 03 2008 1199391138 cyrus-sasl-sql-2.1.19-14 Thu Jan 03 2008 1199391139 cyrus-sasl-ntlm-2.1.19-14 Thu Jan 03 2008 1199391140 cyrus-sasl-devel-2.1.19-14 Thu Jan 03 2008 |