Bug 458666 - Memory leaks in check_trivial_words, check_pw_storagescheme_value
Summary: Memory leaks in check_trivial_words, check_pw_storagescheme_value
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: 389
Classification: Retired
Component: Security - Password Policy
Version: 1.1.1
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rich Megginson
QA Contact: Chandrasekar Kannan
URL:
Whiteboard: comment#5.review+nhosoi
Depends On:
Blocks: 249650 FDS112 453229 CVE-2008-3283
TreeView+ depends on / blocked
 
Reported: 2008-08-11 14:06 UTC by Rich Megginson
Modified: 2015-01-04 23:33 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-08-27 20:39:13 UTC
Embargoed:


Attachments (Terms of Use)
diffs (1.26 KB, patch)
2008-08-11 14:07 UTC, Rich Megginson
no flags Details | Diff
new diffs (1.84 KB, patch)
2008-08-11 23:27 UTC, Rich Megginson
no flags Details | Diff
cvs commit log - DS8.0 (163 bytes, text/plain)
2008-08-12 22:28 UTC, Rich Megginson
no flags Details
cvs commit log - HEAD (157 bytes, text/plain)
2008-08-27 21:08 UTC, Rich Megginson
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2008:0602 0 normal SHIPPED_LIVE Moderate: redhat-ds-base and redhat-ds-admin security and bug fix update 2008-08-27 20:38:30 UTC

Description Rich Megginson 2008-08-11 14:06:23 UTC
The first leak happens when password policy is active and trivial words checking is being used, and the password is being modified.  When getting the list of attribute from the existing entry in the modify case, the function slapi_attr_get_valueset is used - this function makes a duplicate of the valueset and overwrites the valueset argument - vs must be freed first before calling this function.
The second leak happens when the password storage scheme is changed.  The function check_pw_storagescheme_value uses pw_name2scheme to check the given scheme - this function allocates a struct pw_scheme * which must be freed with free_pw_scheme.

Comment 1 Rich Megginson 2008-08-11 14:07:44 UTC
Created attachment 313965 [details]
diffs

Comment 2 Rich Megginson 2008-08-11 16:41:14 UTC
This leak can be triggered by an anonymous user.  The check_trivial_words check is done before access control is applied.  The way to mitigate this issue is to disable trivial words checking - if that's not possible, then password syntax checking must be disabled to avoid this leak.

Comment 3 Rich Megginson 2008-08-11 16:43:47 UTC
The check_pw_storagescheme_value leak can only be triggered by an authenticated, authorized user, changing the password storage setting in the fine grained password policy configuration.

Comment 4 Noriko Hosoi 2008-08-11 23:15:02 UTC
Your fixes look good.

Anoter idea for the first leak, "line 1289 vs = slapi_valueset_new();" may be called after line 1302 only when vs is NULL.  Then, we could save one extra malloc and free (although it's a tiny save...)

Comment 5 Rich Megginson 2008-08-11 23:27:00 UTC
Created attachment 314016 [details]
new diffs

Comment 6 Rich Megginson 2008-08-11 23:27:26 UTC
(In reply to comment #4)
> Your fixes look good.
> 
> Anoter idea for the first leak, "line 1289 vs = slapi_valueset_new();" may be
> called after line 1302 only when vs is NULL.  Then, we could save one extra
> malloc and free (although it's a tiny save...)

Thanks.  Patch updated.

Comment 7 Rich Megginson 2008-08-12 22:28:34 UTC
Created attachment 314147 [details]
cvs commit log - DS8.0

Reviewed by: nkinder, nhosoi (Thanks!)
Fix Description: The first leak happens when password policy is active and trivial words
checking is being used, and the password is being modified.  When getting the
list of attribute from the existing entry in the modify case, the function
slapi_attr_get_valueset is used - this function makes a duplicate of the
valueset and overwrites the valueset argument.  The fix is to move the allocation of vs until after the call to slapi_attr_get_valueset, and only allocate it if it is non NULL.
The second leak happens when the password storage scheme is changed.  The
function check_pw_storagescheme_value uses pw_name2scheme to check the given
scheme - this function allocates a struct pw_scheme * which must be freed with
free_pw_scheme.
Platforms tested: RHEL5, Fedora 8
Flag Day: no
Doc impact: no
QA impact: already covered by acceptance tests
New Tests integrated into TET: none

Comment 9 Jenny Severance 2008-08-19 20:29:10 UTC
How can QE verify this?  What to look for in the valgrind output?

Comment 10 Rich Megginson 2008-08-19 20:36:58 UTC
(In reply to comment #9)
> How can QE verify this?  What to look for in the valgrind output?

Look for a memory leak in check_trivial_words() and in check_pw_storagescheme_value()

Comment 11 Jenny Severance 2008-08-21 17:47:25 UTC
verified 8.0 RHEL4-32, RHEL4-64, RHEL5-32, RHEL5-64

Comment 14 errata-xmlrpc 2008-08-27 20:39:13 UTC
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2008-0602.html

Comment 15 Rich Megginson 2008-08-27 21:08:55 UTC
Created attachment 315147 [details]
cvs commit log - HEAD


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