Bug 514824 - Multi-macro ACIs can cause double free if macro attribute does not exist
Summary: Multi-macro ACIs can cause double free if macro attribute does not exist
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Security - Access Control (ACL)
Version: 1.2.1
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Nathan Kinder
QA Contact: Viktor Ashirov
URL:
Whiteboard:
Depends On:
Blocks: 434914 389_1.2.1
TreeView+ depends on / blocked
 
Reported: 2009-07-30 22:30 UTC by Nathan Kinder
Modified: 2015-12-07 16:43 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-12-07 16:43:53 UTC


Attachments (Terms of Use)
Patch (1.62 KB, patch)
2009-07-30 22:31 UTC, Nathan Kinder
no flags Details | Diff
Test LDIF (861 bytes, application/octet-stream)
2009-07-30 22:34 UTC, Nathan Kinder
no flags Details
Test LDIF (974 bytes, text/plain)
2009-07-30 22:37 UTC, Nathan Kinder
no flags Details
Revised patch (2.25 KB, patch)
2009-07-31 00:09 UTC, Nathan Kinder
no flags Details | Diff

Description Nathan Kinder 2009-07-30 22:30:03 UTC
If you have an ACI with multiple macros in it and the second attribtue does not exist in the entry you are bound as, the in-memory list used for macro substitution is free'd twice.

The code swaps hands the charray it plans to return after substitution over to a working list, but it doesn't set the return list to NULL.  When the second macro attribute is not found, the working list is free'd, yet the address is returned to the caller, who then tries to free the list a second time.  The fix is to set the list to be returned to NULL when the memory is handed over to the working list.

Comment 1 Nathan Kinder 2009-07-30 22:31:36 UTC
Created attachment 355736 [details]
Patch

Comment 2 Nathan Kinder 2009-07-30 22:34:32 UTC
Created attachment 355737 [details]
Test LDIF

Import this LDIF to reproduce the bug.  To trigger the crash, perform the following operation as the "uid=admin,ou=people,o=test,dc=example,dc=com" user:

dn: uid=user,ou=people,o=test,dc=example,dc=com
changetype: modify
add: sn
sn: foo

Comment 3 Nathan Kinder 2009-07-30 22:37:31 UTC
Created attachment 355739 [details]
Test LDIF

Comment 4 Nathan Kinder 2009-07-31 00:09:33 UTC
Created attachment 355753 [details]
Revised patch

This fix addresses some issues pointed out by Noriko.

The list "a" was being set to NULL when we found an attribute match for a macro, but this is no longer necessary now that we reset "a" to NULL when the memory is handed off to the working_list (which covers both the cases of finding/not finding the attribute).

We were also accessing element 0 of list "a" right after handing the memory off to the working_list, but we weren't checking if "a" was NULL first.  I don't believe that "a" could be NULL at this point, but it's safest to check first in case there is some corner case we're not considering.

Comment 5 Nathan Kinder 2009-07-31 01:09:39 UTC
Pushed patch from comment#4 to master.  Thanks to Noriko for her review!

Comment 6 Jenny Galipeau 2010-06-07 18:24:11 UTC
1. initialized database and add attached ldif
2. performed modification as described in comment 2

RESULT:
No crash -

ldapmodify -x -h jgalipea-rhel4.idm.lab.bos.redhat.com -p 389 -D "uid=admin,ou=people,o=test,dc=example,dc=com" -w Secret12 -f mod.ldif 
modifying entry "uid=user,ou=people,o=test,dc=example,dc=com"
ldap_modify: Insufficient access (50)
	additional info: Insufficient 'write' privilege to the 'sn' attribute of entry 'uid=user,ou=people,o=test,dc=example,dc=com'.

Is this the expected result?

thanks

Comment 7 Nathan Kinder 2010-06-18 20:59:56 UTC
(In reply to comment #6)
> 1. initialized database and add attached ldif
> 2. performed modification as described in comment 2
> 
> RESULT:
> No crash -
> 
> ldapmodify -x -h jgalipea-rhel4.idm.lab.bos.redhat.com -p 389 -D
> "uid=admin,ou=people,o=test,dc=example,dc=com" -w Secret12 -f mod.ldif 
> modifying entry "uid=user,ou=people,o=test,dc=example,dc=com"
> ldap_modify: Insufficient access (50)
>  additional info: Insufficient 'write' privilege to the 'sn' attribute of entry
> 'uid=user,ou=people,o=test,dc=example,dc=com'.
> 
> Is this the expected result?
> 
> thanks    

Yes, this is fine.  The only thing we are looking for here is no crash.


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