Bug 217796 - Inconsistent clear password storage causes SASL errors
Inconsistent clear password storage causes SASL errors
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Security - SASL (Show other bugs)
1.0.4
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nathan Kinder
Viktor Ashirov
:
: 243792 (view as bug list)
Depends On:
Blocks: 152373 240316 FDS1.1.0
  Show dependency treegraph
 
Reported: 2006-11-29 17:37 EST by Nathan Kinder
Modified: 2015-12-07 12:16 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-07 12:16:14 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
CVS Diffs (4.44 KB, patch)
2006-11-29 17:37 EST, Nathan Kinder
no flags Details | Diff
Revised Diffs (6.08 KB, patch)
2006-12-01 19:05 EST, Nathan Kinder
no flags Details | Diff
Revised Diffs (6.00 KB, patch)
2006-12-05 16:09 EST, Nathan Kinder
no flags Details | Diff

  None (edit)
Description Nathan Kinder 2006-11-29 17:37:42 EST
Depending on the way a cleartext password is stored, SASL authentication may or
may not work.  There are two issues causing this problem.

The first issue is that we aren't consistent with the way we store cleartext
passwords.  If you set the global password storage scheme to "clear" and use
ldapmodify to add a userpassword with a value of "password", then you get this
stored in the entry:

    userPassword: password

Regardless of the global storage scheme setting, using ldapmodify to add a
userpassword with a value of "{clear}password" results in:

    userPassword: {clear}password

When doing a simple bind, the server-side bind code will strip off the storage
scheme from the userPassword attribute value before doing a comparison with the
password provided by the client attempting to bind.

This brings us to the second problem.  When doing a SASL bind using a mechanism
that actually requires a userPassword, we don't strip off the storage scheme
before setting the SASL password property.  This causes SASL authentication to
fail unless the client application used "{clear}password" as their secret when
generating their credentials.

The attached diffs address both of these issues.  Here's what the changes do:

- When the storage scheme is set to clear on the server, append the "{clear}"
storage scheme prefix to the password unless a different scheme was specified in
the operation.

- Strip off the "{clear}" storage scheme prefix if it is present when setting
the SASL password property.  The code will still deal with cleartext passwords
that do not have a storage scheme prefix.

- Print a warning message in the errors log at the trace level when a SASL bind
is attempted for an entry that has a password present that uses a storage scheme
other than "{clear}".

- If a password is using a storage scheme other than "{clear}", don't copy the
userPassword value into the SASL password property.  This will ensure that
someone can't do something such as use the SHA hashed password that is stored on
the server to create the client side SASL credentials to authenticate.

- The SASL logging callback was printing informational and trace messages at the
LDAP_DEBUG_ANY level.  This would cause spurious logging such as a message for
each step of the DIGEST-MD5 authentication process.  I changed the behavior so
that these non-fatal SASL messages are logged at the LDAP_DEBUG_TRACE level instead.
Comment 1 Nathan Kinder 2006-11-29 17:37:42 EST
Created attachment 142445 [details]
CVS Diffs
Comment 2 Noriko Hosoi 2006-11-29 18:44:00 EST
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?
Comment 3 Rich Megginson 2006-11-29 22:34:49 EST
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().
Comment 4 Nathan Kinder 2006-11-30 11:59:45 EST
(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.

Comment 5 Rich Megginson 2006-11-30 12:06:00 EST
(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.
Comment 6 Nathan Kinder 2006-12-01 19:05:16 EST
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.
Comment 7 Noriko Hosoi 2006-12-01 19:21:19 EST
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 ;).
Comment 10 Nathan Kinder 2006-12-05 16:09:20 EST
Created attachment 142888 [details]
Revised Diffs

This set of revised diffs adds Noriko's suggestion from Comment#7.
Comment 11 Nathan Kinder 2006-12-05 18:59:33 EST
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
Comment 13 Nathan Kinder 2007-06-12 13:35:19 EDT
*** Bug 243792 has been marked as a duplicate of this bug. ***
Comment 15 Michael Gregg 2008-01-07 19:28:13 EST
 
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 

Note You need to log in before you can comment on or make changes to this bug.