From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.4.2) Gecko/20040308 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 function. 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): libuser-0.51.7-7.1.1 How reproducible: Always 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.
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. Thanks again.