Bug 120168
| Summary: | CAN-2004-2392 libuser serious programming bugs | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Steve Grubb <linux_4ever> | ||||||
| Component: | libuser | Assignee: | Miloslav Trmač <mitr> | ||||||
| Status: | CLOSED RAWHIDE | QA Contact: | |||||||
| Severity: | high | Docs Contact: | |||||||
| Priority: | medium | ||||||||
| Version: | rawhide | CC: | cochranb, mjc, rh-bugzilla, wtogami | ||||||
| Target Milestone: | --- | ||||||||
| Target Release: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | |||||||||
| Fixed In Version: | 0.51.10-1 | Doc Type: | Bug Fix | ||||||
| Doc Text: | Story Points: | --- | |||||||
| Clone Of: | Environment: | ||||||||
| Last Closed: | 2004-09-02 22:22:11 UTC | Type: | --- | ||||||
| Regression: | --- | Mount Type: | --- | ||||||
| Documentation: | --- | CRM: | |||||||
| Verified Versions: | Category: | --- | |||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||
| Embargoed: | |||||||||
| Bug Depends On: | |||||||||
| Bug Blocks: | 114961, 123268 | ||||||||
| Attachments: |
|
||||||||
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. |
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.