Bug 120168 - CAN-2004-2392 libuser serious programming bugs
Summary: CAN-2004-2392 libuser serious programming bugs
Alias: None
Product: Fedora
Classification: Fedora
Component: libuser
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Miloslav Trmač
QA Contact:
Depends On:
Blocks: FC2Blocker FC3Target
TreeView+ depends on / blocked
Reported: 2004-04-06 16:08 UTC by Steve Grubb
Modified: 2007-11-30 22:10 UTC (History)
4 users (show)

Fixed In Version: 0.51.10-1
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2004-09-02 22:22:11 UTC

Attachments (Terms of Use)
Patch to fix all the problems listed above. (7.21 KB, patch)
2004-04-06 16:10 UTC, Steve Grubb
no flags Details | Diff
Current patch I'm testing (8.12 KB, patch)
2004-07-27 19:08 UTC, Steve Grubb
no flags Details | Diff

Description Steve Grubb 2004-04-06 16:08:48 UTC
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.4.2)

Description of problem:
During code review I could a number of serious programming bugs:

If there was a failure reading config data, the memory buffer was
taken to be valid.

On a read failure of /dev/urandom, buffer was taken to be valid. The
function could not indicate that a failure occurred or even retry if
errno == EINTR.

Upon a read failure, salt may be mostly 0's.

During unlock, a structure was being passed by value to a variadic

length returned by read was being caught in an unsigned variable which
would later cause write to output 4294967295 bytes of data.

flcose was being passed a NULL pointer in many places.

Version-Release number of selected component (if applicable):

How reproducible:

Steps to Reproduce:
Found during code review

Additional info:

I will attach a patch that fixes all of these issues. I did not review
prior versions to see if they are susceptible to all these problems.

Comment 1 Steve Grubb 2004-04-06 16:10:43 UTC
Created attachment 99149 [details]
Patch to fix all the problems listed above.

Please apply before fedora core 2 final.

Comment 2 Warren Togami 2004-04-11 03:59:26 UTC
This needs code review.

Comment 3 Nalin Dahyabhai 2004-04-13 03:32:28 UTC
I have to disagree with the first two hunks, which appear to be
cosmetic.  Hunk 3 accepted, good catch!  Hunk 4 likewise.  Hunks 6,7,8
applied, will need additional fixes to handle errors returned.  Good
catch on hunk 9.  Hunk 10, good fix.  Hunk 11 looks cosmetic.  Hunks
13,14,15,16,17 are good catches.

I'm not sure there's much point to checking for EROFS errors from
access(), as the logic is going to cause a failure when we try to
modify the files anyway.

Comment 4 Steve Grubb 2004-04-13 12:48:24 UTC
>I'm not sure there's much point to checking for EROFS errors from
>access(), as the logic is going to cause a failure when we try to
>modify the files anyway.

While this may be true...why bother to ask for the superuser password
when that will ultimately fail? My personal beliefs are to inform the
user at the earliest chance so they might be able to remount the
filesystem or otherwise perform corrective action instead of typing a
password and then seeing a failure. Its not as important as the other
bugs pointed out.

As for the first two chunks...while it seems cosmetic, it would be
good to fix so the next person doing a code review doesn't have to
report the same thing.

I did not check to see if these bugs exist in earlier versions of
Libuser. You might want to look into RHL 9 before it is no longer
maintained as well as the Enterprise versions & Fedora Core 1. The
read() return code being caught in an unsigned variable could cause
someone to have a bad day.

Thanks for giving this your attention !

Comment 5 Bob Cochran 2004-05-31 04:36:49 UTC
I applied the patch as part of rebuilding libuser (the Fedora Core 2
release version) against openldap-2.2.11. I'm not sure whether my
openldap packages will prove stable or what will happen with the
libuser packages. I'll let you know. 

Comment 6 Steve Grubb 2004-07-27 19:08:04 UTC
Created attachment 102236 [details]
Current patch I'm testing

This patch has the error handling Nalin suggested. It compiles cleanly. I've
been running this for 2 months with no known problems.

Comment 7 Miloslav Trmač 2004-09-02 19:00:50 UTC
Thanks for the patch.

I have applied it, after fixing a few leaks introduced
by or found while applying this patch, properly initializing
struct lu_errror with a message, and most importantly
handling the other lu_make_crypted() call in modules/ldap.c
(otherwise setting a password when /dev/urandom is not
available would set an empty password).

Comment 8 Miloslav Trmač 2004-09-02 22:22:11 UTC
Should be fixed in libuser-0.51.10-1, when it hits rawhide.
Thanks again.

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