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):
Steps to Reproduce:
Found during code review
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.
Created attachment 99149 [details]
Patch to fix all the problems listed above.
Please apply before fedora core 2 final.
This needs code review.
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.
>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 !
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.
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.
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).
Should be fixed in libuser-0.51.10-1, when it hits rawhide.