Ticket #402 (closed enhancement: fixed)

Opened 22 months ago

Last modified 10 months ago

unhashed#user#password in entry extension

Reported by: nhosoi Owned by: nhosoi
Priority: major Milestone: 1.3.1.1
Component: Security - General Version: 1.2.11
Keywords: Cc:
Blocked By: Blocking:
Review: ack Ticket origin: Community
Red Hat Bugzilla: 0

Description

This fix is a hack.

Ticket #378 (closed enhancement: fixed)
unhashed#user#password field

It treats unhashe#user#password specially all over the code, which is not preferable.

Opening a new ticket to enhance the unhashed password handling.

Attachments

0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.2.patch (53.8 KB) - added by nhosoi 22 months ago.
git patch file (master)
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.3.patch (1.2 MB) - added by nhosoi 22 months ago.
git patch file (master) - take 3 including autogen'ed files.
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.4.patch (1.2 MB) - added by nhosoi 22 months ago.
git patch file (master) - take 4 including autogen'ed files.
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.patch (1.2 MB) - added by nhosoi 22 months ago.
git patch file (master) - take 4 including autogen'ed files.
0001-Trac-Ticket-402-nhashed-user-password-in-entry-exten.5.patch (2.6 KB) - added by nhosoi 10 months ago.
git patch file (master)

Change History

comment:1 Changed 22 months ago by nhosoi

Fix description: This patch stashes unhashed password in the
entry extension instead of the ordinary attribute value. The
entry extension was implemented using the factory framework.
Taking the advantage of the framework, the setter and getter
functions (slapi_pw_set/get_entry_ext) are added.

comment:2 Changed 22 months ago by nhosoi

[Example change for unhashed password users]

$ diff -twU4 ./daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_prepost.c /tmp/ipapwd_prepost.c 
--- ./daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_prepost.c	2012-06-28 14:54:57.809671891 -0700
+++ /tmp/ipapwd_prepost.c	2012-06-28 15:54:47.998631419 -0700
@@ -204,11 +204,26 @@
             }
             slapi_ch_free_string(&userpw);
             userpw = tmp;
         } else if (slapi_is_encoded(userpw)) {
+            char *userpw_clear = NULL;
+#if 0
             /* check if we have access to the unhashed user password */
-            char *userpw_clear =
+            userpw_clear =
                 slapi_entry_attr_get_charptr(e, "unhashed#user#password");
+#else
+            Slapi_Value **pwvals = NULL;
+            rc = slapi_pw_get_entry_ext(e, &pwvals);
+            if (LDAP_SUCCESS == rc) {
+                Slapi_ValueSet vset;
+                Slapi_Value *value = NULL;
+                /* pwvals is passed in to vset; 
+                 * thus no need to free vset nor userpw_clear. */
+                valueset_set_valuearray_passin(&vset, pwvals);
+                slapi_valueset_first_value(&vset, &value);
+                userpw_clear = slapi_value_get_string(value);
+            }
+#endif
 
             /* unhashed#user#password doesn't always contain the clear text
              * password, therefore we need to check if its value isn't the same
              * as userPassword to make sure */
@@ -217,11 +232,11 @@
                 slapi_ch_free_string(&userpw);
             } else {
                 userpw = slapi_ch_strdup(userpw_clear);
             }
-
+#if 0
             slapi_ch_free_string(&userpw_clear);
-
+#endif
             if (rc != LDAP_SUCCESS) {
                 /* we don't have access to the clear text password;
                  * let it slide if migration is enabled, but don't
                  * generate kerberos keys */

comment:3 Changed 22 months ago by nhosoi

  • Review set to review?

Changed 22 months ago by nhosoi

git patch file (master)

comment:4 Changed 22 months ago by nhosoi

Fix description: This patch adds the method to use entry
extension to stash the unhashed password in addition to the
existing one which uses the ordinary attribute.

It introduces the definition "USE_OLD_UNHASHED" in configure.ac
to keep the old method to use the attribute.

Once all the plugins' migration is done, the old method can be
disabled by removing the definition. We could also remove the
code in "#if defined(USE_OLD_UNHASHED)" then.

comment:5 follow-up: ↓ 7 Changed 22 months ago by nkinder

There appears to have been a problem merging your changes in ldap/servers/slapd/ldaputil.c with the patch for ticket #399. Your patch reverts the fix made to always get the bind result, which will cause a regression.

comment:6 Changed 22 months ago by nkinder

It looks odd that the slapi_entry_apply_mod_extension() and entry_apply_mod_wsi() functions expect SLAPI_PW_* specific extensions since these are generic functions. Is this a safe assumption?

If pw_entry_constructor() fails to allocate a lock, should we just return NULL without allocating and returning pw_extp? The slapi_rwlock_*() functions called by the extension get/set functions do check if the lock is NULL before attempting to use them, but this will behave with no protection from the lock.

Does pw_copy_entry_ext() need to use the rwlock from the source and destination extensions?

comment:7 in reply to: ↑ 5 Changed 22 months ago by nhosoi

Replying to nkinder:

There appears to have been a problem merging your changes in ldap/servers/slapd/ldaputil.c with the patch for ticket #399. Your patch reverts the fix made to always get the bind result, which will cause a regression.

Sorry, I don't know why the file was reverted. I recreated the patch which has no change on ldaputil.c.

It looks odd that the slapi_entry_apply_mod_extension() and entry_apply_mod_wsi() functions expect SLAPI_PW_* specific extensions since these are generic functions. Is this a safe assumption?

Yes, you are right! I replaced SLAPI_PW_SET_ADD with SLAPI_EXT_SET_ADD and SLAPI_PW_SET_REPLACE with SLAPI_EXT_SET_REPLACE.

If pw_entry_constructor() fails to allocate a lock, should we just return NULL without allocating and returning pw_extp? The slapi_rwlock_*() functions called by the extension get/set functions do check if the lock is NULL before attempting to use them, but this will behave with no protection from the lock.

A good point. Thanks for pointing it out. I've modified constructor to return NULL when slapi_new_rwlock fails. If it happens, "struct slapi_pw_entry_ext" won't be allocated. And the following unhashed password set/get fails with ""pw_entry_extension is not set".

Does pw_copy_entry_ext() need to use the rwlock from the source and destination extensions?

Another good point! I added to call slapi_rwlock_wrlock to protect valuearray_add_valuearray.

Changed 22 months ago by nhosoi

git patch file (master) - take 3 including autogen'ed files.

comment:8 follow-up: ↓ 9 Changed 22 months ago by nkinder

Does pw_copy_entry_ext() also need to obtain a reader lock on src_extp? I would think it should hold a reader lock before it attempts to copy src_extp->pw_entry_values.

Aside from that, the patch looks good.

Changed 22 months ago by nhosoi

git patch file (master) - take 4 including autogen'ed files.

Changed 22 months ago by nhosoi

git patch file (master) - take 4 including autogen'ed files.

comment:9 in reply to: ↑ 8 Changed 22 months ago by nhosoi

Replying to nkinder:

Does pw_copy_entry_ext() also need to obtain a reader lock on src_extp? I would think it should hold a reader lock before it attempts to copy src_extp->pw_entry_values.

Aside from that, the patch looks good.

Thanks a lot, Nathan! I added the read lock and the new patch is attached to this ticket.

comment:10 Changed 22 months ago by nkinder

  • Review changed from review? to ack

comment:11 Changed 22 months ago by nhosoi

  • Resolution set to fixed
  • Status changed from new to closed

Reviewed by Nathan (Thank you!!!)

Pushed to master.

$ git merge trac402
Updating 6ba24eb..091c749
Fast-forward

Makefile.in | 7 +-
aclocal.m4 | 40 +-
config.h.in | 9 +
configure |18950 ++++++++------------
configure.ac | 1 +
ldap/servers/plugins/acl/acleffectiverights.c | 10 +-
ldap/servers/plugins/deref/deref.c | 5 +-
.../plugins/replication/windows_protocol_util.c | 139 +-
ldap/servers/slapd/add.c | 114 +-
ldap/servers/slapd/attr.c | 4 +
ldap/servers/slapd/entry.c | 181 +-
ldap/servers/slapd/entrywsi.c | 47 +-
ldap/servers/slapd/opshared.c | 2 +
ldap/servers/slapd/pblock.c | 2 +
ldap/servers/slapd/proto-slap.h | 3 +-
ldap/servers/slapd/pw.c | 238 +
ldap/servers/slapd/pw_mgmt.c | 10 +-
ldap/servers/slapd/pw_retry.c | 2 +-
ldap/servers/slapd/schema.c | 4 +
ldap/servers/slapd/slap.h | 9 +
ldap/servers/slapd/slapi-plugin.h | 88 +
ldap/servers/slapd/slapi-private.h | 4 +
ldap/servers/slapd/util.c | 32 +
ldap/servers/slapd/valueset.c | 32 +
ltmain.sh | 3968 +++--
25 files changed, 10982 insertions(+), 12919 deletions(-)

$ git push
Counting objects: 67, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (33/33), done.
Writing objects: 100% (34/34), 74.75 KiB, done.
Total 34 (delta 31), reused 1 (delta 1)
To ssh://git.fedorahosted.org/git/389/ds.git

6ba24eb..091c749 master -> master

comment:12 Changed 20 months ago by nkinder

  • screened set to 1

Added initial screened field value.

comment:13 Changed 14 months ago by nkinder

  • Red Hat Bugzilla set to 0

Changed 10 months ago by nhosoi

git patch file (master)

comment:14 Changed 10 months ago by nhosoi

  • Ticket origin set to Community
  • Resolution fixed deleted
  • Review changed from ack to review?
  • Status changed from closed to reopened

Description: commit 091c749adcc53ef3332cab6d34b25dadcb69c696
had a logic flaw: entry_apply_mod_wsi checks whether modify
candidate attribute is to be stored in an entry extension or
not. If it is supposed to be in the entry extension, it removes
the attribute from the entry attribute list (e_attrs), and put
it into the entry extension. The steps have to be done under
any condition, but entry_apply_mod_wsi used to check if the
entry extension was configured properly and the attribute
existed in the extension, first. If both were not satisfied,
the attribute was not removed from the attribute list.

This patch eliminated the check and the attribute to be stored
in the entry extension is always removed from the attribute
list in the entry.

comment:15 Changed 10 months ago by nkinder

  • Review changed from review? to ack

comment:16 Changed 10 months ago by nhosoi

  • Resolution set to fixed
  • Milestone changed from 1.3.0.a1 to 1.3.1.1
  • Status changed from reopened to closed

Reviewed by Nathan (Thank you!!)

Pushed to master: commit c4667c08c36294e460a43a378a5f426c841c850c

Pushed to 389-ds-base-1.3.1: commit 539419f3453d3620f56995026431de82a2ce017c

Note: See TracTickets for help on using tickets.