Bug 585905 - ACL with targattrfilters error crashes the server
Summary: ACL with targattrfilters error crashes the server
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Security - Access Control (ACL)
Version: 1.2.6
Hardware: x86_64
OS: Linux
low
medium
Target Milestone: ---
Assignee: Noriko Hosoi
QA Contact: Viktor Ashirov
URL:
Whiteboard:
Depends On:
Blocks: 434914 389_1.2.6 639035
TreeView+ depends on / blocked
 
Reported: 2010-04-26 11:34 UTC by Andrey Ivanov
Modified: 2015-12-07 16:55 UTC (History)
3 users (show)

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


Attachments (Terms of Use)
git patch file (master) (3.35 KB, patch)
2010-04-27 01:03 UTC, Noriko Hosoi
nkinder: review+
Details | Diff
git patch file (master) (3.43 KB, patch)
2010-04-27 05:52 UTC, Noriko Hosoi
nkinder: review+
Details | Diff

Description Andrey Ivanov 2010-04-26 11:34:29 UTC
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

Adding a simple ACL with (targattrfilters="smth") as target crashes the server

Reproducible: Always

Steps to Reproduce:
1. Install 389-ds-base-1.2.6-0.3.a3.el5 from rpm
2. setup-ds-admin.pl with express installation options
3. try to add to any entry the following acl:

aci: (targattrfilters="description")(version 3.0;acl "Hehe"; allow(read,search,compare) (userdn="ldap:///anyone"); )

Actual Results:  
The server crashes

Expected Results:  
The server should not crash, some sensible error message should be displayed. The ACL syntax validation procedure should be more robust...

Platform: CentOS 5.4 x86_64
The debuginfo for 389-ds-base-1.2.6-0.3.a3.el5 is not available from epel-testing-debuginfo, so the stack trace is not very informative.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x4684b940 (LWP 26788)]
0x00002b7a1c00e3f2 in acl_parse () from /usr/lib64/dirsrv/plugins/libacl-plugin.so
(gdb) bt full
#0  0x00002b7a1c00e3f2 in acl_parse () from /usr/lib64/dirsrv/plugins/libacl-plugin.so
No symbol table info available.
#1  0x00002b7a1c00f010 in acl_verify_syntax () from /usr/lib64/dirsrv/plugins/libacl-plugin.so
No symbol table info available.
#2  0x00002b7a1bfff9f9 in acl_check_mods () from /usr/lib64/dirsrv/plugins/libacl-plugin.so
No symbol table info available.
#3  0x00002b7a166289bd in plugin_call_acl_mods_access () from /usr/lib64/dirsrv/libslapd.so.0
No symbol table info available.
#4  0x00002b7a1df7559d in ldbm_back_modify () from /usr/lib64/dirsrv/plugins/libback-ldbm.so
No symbol table info available.
#5  0x00002b7a1661a07a in ?? () from /usr/lib64/dirsrv/libslapd.so.0
No symbol table info available.
#6  0x00002b7a1661b0f9 in do_modify () from /usr/lib64/dirsrv/libslapd.so.0
No symbol table info available.
#7  0x0000000000412eba in sasl_map_config_add ()
No symbol table info available.
#8  0x00002b7a1743a67d in ?? () from /usr/lib64/libnspr4.so
No symbol table info available.
#9  0x0000003862206617 in start_thread () from /lib64/libpthread.so.0
No symbol table info available.
#10 0x00000038612d3c2d in clone () from /lib64/libc.so.6
No symbol table info available.

Comment 1 Noriko Hosoi 2010-04-27 01:03:01 UTC
Created attachment 409325 [details]
git patch file (master)

Bug Description:
targattrfilters takes this format of value:
 (targattrfilters="add=attr1:F1 && attr2:F2... &&
  attrn:Fn,del=attr1:F1 && attr2:F2 ... && attrn:Fn")
The ACL plugin code had blindly expected the value contains
the operator "add" or "del" and '=' to concatenate the
attribute and filter pair.  The plugin should have checked
the possibility that the value does not follow the format.

Fix Description:
If '=' is not included in the targattrfilters value, the
ACL parser returns ACL_SYNTAX_ERR.

File: ldap/servers/plugins/acl/aclparse.c

Comment 2 Rich Megginson 2010-04-27 01:43:28 UTC
Comment on attachment 409325 [details]
git patch file (master)

first diff section - lines 291- - should be NULL == tempstr

Also, do the gotos or returns potentially cause any memory leaks?

Comment 3 Rich Megginson 2010-04-27 01:49:18 UTC
Comment on attachment 409325 [details]
git patch file (master)

sorry - I meant tmpstr not tempstr

Comment 4 Noriko Hosoi 2010-04-27 05:44:30 UTC
(In reply to comment #2)
> (From update of attachment 409325 [details])
> first diff section - lines 291- - should be NULL == tempstr
> 
> Also, do the gotos or returns potentially cause any memory leaks?    

(In reply to comment #3)
> (From update of attachment 409325 [details])
> sorry - I meant tmpstr not tempstr    

Oops.  Thanks, Rich!  I fixed it.  I went through the gotos and returns.  I don't think I introduced any "new" leaks.  I tested the server with valgrind for the test case that Andrey provided.  It does not cause memory leak.

Comment 5 Noriko Hosoi 2010-04-27 05:52:36 UTC
Created attachment 409357 [details]
git patch file (master)

Fixed the bug Rich found out.  (Thanks a lot!)

Comment 6 Noriko Hosoi 2010-04-27 17:36:43 UTC
Reviewed by Rich and Nathan (Thank you!!).

Pushed to master.

$ git merge work
Updating 3155d9c..b65b3c9
Fast forward
 ldap/servers/plugins/acl/aclparse.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

$ git push
Counting objects: 13, done.
Delta compression using 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 1.08 KiB, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
   3155d9c..b65b3c9  master -> master

Finally, special thanks to Andrey for providing us this good test case!

Comment 8 Amita Sharma 2011-09-28 09:02:25 UTC
[root@snmaptest ~]# rpm -qa | grep 389
389-adminutil-1.1.14-1.el6.x86_64
389-adminutil-devel-1.1.14-1.el6.x86_64
389-ds-base-libs-1.2.9.11-1.el6.x86_64
389-ds-console-1.2.6-1.el6.noarch
389-ds-base-debuginfo-1.2.8.2-1.el6_1.3.x86_64
389-console-1.1.7-1.el6.noarch
389-admin-console-1.1.8-1.el6.noarch
389-ds-console-doc-1.2.6-1.el6.noarch
389-ds-1.2.1-2.el6.noarch
389-ds-base-devel-1.2.9.11-1.el6.x86_64
389-ds-base-1.2.9.11-1.el6.x86_64
389-admin-1.1.23-2.el6.x86_64
389-admin-console-doc-1.1.8-1.el6.noarch

[root@snmaptest ~]# ldapmodify -h localhost -p 389 -D "cn=Directory Manager" -w Secret123 << EOF
> dn: dc=example,dc=com
> changetype: modify
> replace: aci
> aci: (targattrfilters="description")(version 3.0;acl "Hehe"; allow(read,search,compare) (userdn="ldap:///cn=Userami,dc=test,dc=com"); )
> EOF
modifying entry "dc=example,dc=com"
ldap_modify: Invalid syntax (21)
	additional info: ACL Syntax Error(-5):(targattrfilters=\22description\22)(version 3.0;acl \22Hehe\22; allow(read,search,compare) (userdn=\22ldap:///cn=Userami,dc=test,dc=com\22); )

hence VERIFIED.


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