Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#5835 closed Bugs (wontfix)

cregex.cpp, fileiter.cpp, posix_api.cpp using unsafe sprintf

Reported by: noloader@… Owned by: Andrew Sutton
Milestone: To Be Determined Component: graph
Version: Boost 1.47.0 Severity: Problem
Keywords: Cc: noloader@…

Description

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

Attachments (0)

Change History (5)

comment:1 Changed 6 years ago by Jeffrey Walton <noloader@…>

Cc: noloader@… added
Component: Noneregex
Owner: set to John Maddock

comment:2 Changed 6 years ago by John Maddock

(In [74897]) Improve sprintf usage. Stop passing UDT's through (...) even in meta programs. Fixes #5958. Refs #5835.

comment:3 Changed 6 years ago by John Maddock

Component: regexgraph
Owner: changed from John Maddock to Andrew Sutton

Fixed in Boost.Regex, reassigning to the Graph lib.

comment:4 Changed 6 years ago by Jeremiah Willcock

Resolution: wontfix
Status: newclosed

This code (iohb.[ch]) is in an example that is not even compiled by default, and the example (minimum_degree_ordering.cpp) never calls the functions in iohb.c that use sprintf. Thus, I see no reason to change anything unless there are problems in the Harwell-Boeing reader code.

comment:5 Changed 6 years ago by noloader@…

set to wontfix

+1 for the freetards

Provide broken code, waste a persons time who has to audit the broken code, and then claim its not even used. WTF?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Andrew Sutton.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.