Bug 514824

Summary: Multi-macro ACIs can cause double free if macro attribute does not exist
Product: [Retired] 389 Reporter: Nathan Kinder <nkinder>
Component: Security - Access Control (ACL)Assignee: Nathan Kinder <nkinder>
Status: CLOSED CURRENTRELEASE QA Contact: Viktor Ashirov <vashirov>
Severity: medium Docs Contact:
Priority: low    
Version: 1.2.1CC: jgalipea, 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 16:43:53 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: 434914, 517385    
Attachments:
Description Flags
Patch
none
Test LDIF
none
Test LDIF
none
Revised patch none

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 Severance 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.