[Boost-bugs] [Boost C++ Libraries] #5835: cregex.cpp, fileiter.cpp, posix_api.cpp using unsafe sprintf

Subject: [Boost-bugs] [Boost C++ Libraries] #5835: cregex.cpp, fileiter.cpp, posix_api.cpp using unsafe sprintf
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2011-08-30 05:25:23

#5835: cregex.cpp, fileiter.cpp, posix_api.cpp using unsafe sprintf
--------------------------------+-------------------------------------------
 Reporter:  noloader@…          |       Owner:         
     Type:  Bugs                |      Status:  new    
Milestone:  To Be Determined    |   Component:  None   
  Version:  Boost 1.47.0        |    Severity:  Problem
 Keywords:                      |  
--------------------------------+-------------------------------------------
 Folks performing a security audit on Boost will have to take a look at
 cregex.cpp, fileiter.cpp and posix.cpp since it is using unsafe function
 such as sprintf.

 The same folks will have to fail Boost because its ignoring return values
 from sprintf (MAX_PATH is easy to overflow in practice, especially when
 the attacker controls the input).

 cregex.cpp BuildFileList:
 {{{
 char buf[MAX_PATH];
 ...

 #if BOOST_WORKAROUND(BOOST_MSVC, >= 1400)
     (::sprintf_s)(buf, sizeof(buf), "%s%s%s", dstart.path(),
 directory_iterator::separator(), ptr);
 #else
     (std::sprintf)(buf, "%s%s%s", dstart.path(),
 directory_iterator::separator(), ptr);
 #endif
 }}}

 Ditto for fileiter.cpp _fi_attributes:

 {{{
 unsigned _fi_attributes(const char* root, const char* name)
 {
   char buf[MAX_PATH];
   if( ( (root[0] == *_fi_sep) || (root[0] == *_fi_sep_alt) ) && (root[1]
 == '\0') )
     (std::sprintf)(buf, "%s%s", root, name);
   else
     (std::sprintf)(buf, "%s%s%s", root, _fi_sep, name);
     DIR* d = opendir(buf);
     if(d)
     {
       closedir(d);
       return _fi_dir;
     }
   return 0;
 }
 }}}

 In general, I would expect an ostringstream to be a more appropriate
 choice for a c++ library (rather than sprintf and snprintf). At minimum,
 the original authors and/or Boost QA team should place comments in the
 code for assurance purposes.

 I can't really comment on iohb.c since I'm not familiar with the Harwell-
 Boeing File format. However, I expect that an attacker *will* manipulate
 the header, and I don't trust NIST's use of unchecked sprintf, sscanf, and
 friends under adverse conditions and a hostile environment. For example,
 in readHB_header, the authors continue to parse even if sscanf encounters
 an error:

 {{{
     if ( sscanf(line,"%i",&Totcrd) != 1) Totcrd = 0;
     if ( sscanf(line,"%*i%i",Ptrcrd) != 1) *Ptrcrd = 0;
     if ( sscanf(line,"%*i%*i%i",Indcrd) != 1) *Indcrd = 0;
     if ( sscanf(line,"%*i%*i%*i%i",Valcrd) != 1) *Valcrd = 0;
     if ( sscanf(line,"%*i%*i%*i%*i%i",Rhscrd) != 1) *Rhscrd = 0;
 }}}

 For iohb.c, I would reject the submission until the authors port it to c++
 (there are too many little problems lurking in the C code). At minimum,
 don't distribute iohb.c until its known to be safe (place the honus on the
 authors who claim its ready for production).

-- 
Ticket URL: <https://svn.boost.org/trac/boost/ticket/5835>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.