[BusyBox 0000805]: Unterminated buffer in mdev.c

bugs at busybox.net bugs at busybox.net
Sun May 7 23:59:32 UTC 2006


The following issue has been CLOSED 
====================================================================== 
http://busybox.net/bugs/view.php?id=805 
====================================================================== 
Reported By:                Ken Milmore
Assigned To:                BusyBox
====================================================================== 
Project:                    BusyBox
Issue ID:                   805
Category:                   Other
Reproducibility:            have not tried
Severity:                   minor
Priority:                   normal
Status:                     closed
Resolution:                 open
Fixed in Version:           
====================================================================== 
Date Submitted:             03-28-2006 16:04 PST
Last Modified:              05-07-2006 16:59 PDT
====================================================================== 
Summary:                    Unterminated buffer in mdev.c
Description: 
The buffer used to read the major:minor numbers from sysfs is not correctly
terminated.
======================================================================
Relationships       ID      Summary
----------------------------------------------------------------------
duplicate of        0000651 mdev -s fails to work.
====================================================================== 

---------------------------------------------------------------------- 
 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. 

---------------------------------------------------------------------- 
 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 */ 

---------------------------------------------------------------------- 
 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? 

---------------------------------------------------------------------- 
 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. 

---------------------------------------------------------------------- 
 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                          
======================================================================




More information about the busybox-cvs mailing list