busybox.mdev_devt_parse-fix.diff [^] (475 bytes) 04-11-06 11:00 
| Anonymous | Login | Signup for a new account | 11-10-2008 11:00 PST |
| Main | My View | View Issues | Change Log | Docs |
| Viewing Issue Simple Details [ Jump to Notes ] | [ View Advanced ] [ Issue History ] [ Print ] | ||||||||
| ID | Category | Severity | Reproducibility | Date Submitted | Last Update | ||||
| 0000805 | [BusyBox] Other | minor | have not tried | 03-28-06 16:04 | 05-07-06 16:59 | ||||
| Reporter | Ken Milmore | View Status | public | ||||||
| Assigned To | BusyBox | ||||||||
| Priority | normal | Resolution | won't fix | ||||||
| Status | closed | Product Version | svn | ||||||
| Summary | 0000805: Unterminated buffer in mdev.c | ||||||||
| Description | The buffer used to read the major:minor numbers from sysfs is not correctly terminated. | ||||||||
| Additional Information | See bug [BusyBox 0000651]. The patch I provided for that issue was not quite applied properly. The temp buffer should be explicitly terminated with a null byte after reading from the file on line 48 of mdev.c. This termination was in the original source and appears to have been mistakenly removed. | ||||||||
| Attached Files |
busybox.mdev_devt_parse-fix.diff [^] (475 bytes) 04-11-06 11:00 |
||||||||
|
|
|||||||||
Notes |
|
|
(0001260) bernhardf 04-11-06 11:00 |
Not sure if i understand you correct, but do you mean something like the attached, completely untested patchlet (busybox.mdev_devt_parse-fix.diff) ? Could you please check if putting the '\n' into the sscanf is smaller than the temp[--len]=0 which was there before? TIA. |
|
(0001266) Ken Milmore 04-11-06 16:08 |
What I had in mind was to reinstate the line of code which terminates the temp buffer with a NUL byte (see inline patch below). Runnning sscanf on an unterminated buffer is potentially dangerous. I do not recommend adding the newline character to the sscanf template string. As already discussed, the presence of the newline is a kernel "feature" that it might be best not to rely upon. --- mdev.c.orig 2006-04-11 23:59:11.000000000 +0100 +++ mdev.c 2006-04-12 00:01:17.000000000 +0100 @@ -48,6 +48,7 @@ len = read(fd, temp, PATH_MAX-1); close(fd); if (len < 1) goto end; + temp[len] = 0; /* Determine device name, type, major and minor */ |
|
(0001352) landley 05-07-06 14:45 |
It seems to work just fine without wasting the extra byte. In theory, any trailing garbage is ignored by sscanf anyway. Are you seeing an actual failure? |
|
(0001353) Ken Milmore 05-07-06 15:17 |
No, I've not seen an actual failure; it seems extremely unlikely that it could go wrong in practice. I only pointed this issue out because the buffer WAS correctly terminated in the original version of the code, and now it isn't. Such mistakes usually trip someone up sooner or later. |
|
(0001354) landley 05-07-06 16:59 |
It's not a mistake, it's the removal of an unnecessary byte from a program optimized for size. Any non-digit value should terminate an integer read, especially whitespace, adding an explicit dependency on a \n that just coincidentally happens to be there is not only unnecessary, but it makes the code brittle. Since we're about to close the file leaving unread input is fine, and if somebody can fake the contents of sysfs to inject malicious values we have bigger worries from a security standpoint. And I just checked to make darn sure, the scanf() standard doesn't require that we read all input if we're not interested in it: http://www.opengroup.org/onlinepubs/009695399/functions/scanf.html [^] |
|
Issue History |
|||
| Date Modified | Username | Field | Change |
| 03-28-06 16:04 | Ken Milmore | New Issue | |
| 03-28-06 16:04 | Ken Milmore | Status | new => assigned |
| 03-28-06 16:04 | Ken Milmore | Assigned To | => BusyBox |
| 04-11-06 11:00 | bernhardf | Note Added: 0001260 | |
| 04-11-06 11:00 | bernhardf | File Added: busybox.mdev_devt_parse-fix.diff | |
| 04-11-06 11:03 | bernhardf | Relationship added | duplicate of 0000651 |
| 04-11-06 16:08 | Ken Milmore | Note Added: 0001266 | |
| 05-07-06 14:45 | landley | Note Added: 0001352 | |
| 05-07-06 14:45 | landley | Status | assigned => feedback |
| 05-07-06 15:17 | Ken Milmore | Note Added: 0001353 | |
| 05-07-06 16:59 | landley | Status | feedback => closed |
| 05-07-06 16:59 | landley | Note Added: 0001354 | |
| 05-07-06 16:59 | landley | Resolution | open => won't fix |
| 05-07-06 16:59 | landley | Fixed in Version | => svn |
| Copyright © 2000 - 2006 Mantis Group |  |
