Boost C++ Libraries: Ticket #5835: cregex.cpp, fileiter.cpp, posix_api.cpp using unsafe sprintf https://svn.boost.org/trac10/ticket/5835 <p> 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. </p> <p> 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). </p> <p> cregex.cpp <a class="missing wiki">BuildFileList</a>: </p> <pre class="wiki">char buf[MAX_PATH]; ... #if BOOST_WORKAROUND(BOOST_MSVC, &gt;= 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 </pre><p> Ditto for fileiter.cpp _fi_attributes: </p> <pre class="wiki">unsigned _fi_attributes(const char* root, const char* name) { char buf[MAX_PATH]; if( ( (root[0] == *_fi_sep) || (root[0] == *_fi_sep_alt) ) &amp;&amp; (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; } </pre><p> 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. </p> <p> 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: </p> <pre class="wiki"> if ( sscanf(line,"%i",&amp;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; </pre><p> 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). </p> en-us Boost C++ Libraries /htdocs/site/boost.png https://svn.boost.org/trac10/ticket/5835 Trac 1.4.3 Jeffrey Walton <noloader@…> Wed, 31 Aug 2011 00:41:56 GMT component changed; cc, owner set https://svn.boost.org/trac10/ticket/5835#comment:1 https://svn.boost.org/trac10/ticket/5835#comment:1 <ul> <li><strong>cc</strong> <span class="trac-author">noloader@…</span> added </li> <li><strong>owner</strong> set to <span class="trac-author">John Maddock</span> </li> <li><strong>component</strong> <span class="trac-field-old">None</span> → <span class="trac-field-new">regex</span> </li> </ul> Ticket John Maddock Mon, 10 Oct 2011 15:46:09 GMT <link>https://svn.boost.org/trac10/ticket/5835#comment:2 </link> <guid isPermaLink="false">https://svn.boost.org/trac10/ticket/5835#comment:2</guid> <description> <p> (In <a class="changeset" href="https://svn.boost.org/trac10/changeset/74897" title="Improve sprintf usage. Stop passing UDT's through (...) even in meta ...">[74897]</a>) Improve sprintf usage. Stop passing UDT's through (...) even in meta programs. Fixes <a class="closed ticket" href="https://svn.boost.org/trac10/ticket/5958" title="#5958: Bugs: Passing an argument of non-POD class type boost::match_results through ... (closed: fixed)">#5958</a>. Refs <a class="closed ticket" href="https://svn.boost.org/trac10/ticket/5835" title="#5835: Bugs: cregex.cpp, fileiter.cpp, posix_api.cpp using unsafe sprintf (closed: wontfix)">#5835</a>. </p> </description> <category>Ticket</category> </item> <item> <dc:creator>John Maddock</dc:creator> <pubDate>Mon, 10 Oct 2011 15:47:20 GMT</pubDate> <title>owner, component changed https://svn.boost.org/trac10/ticket/5835#comment:3 https://svn.boost.org/trac10/ticket/5835#comment:3 <ul> <li><strong>owner</strong> changed from <span class="trac-author">John Maddock</span> to <span class="trac-author">Andrew Sutton</span> </li> <li><strong>component</strong> <span class="trac-field-old">regex</span> → <span class="trac-field-new">graph</span> </li> </ul> <p> Fixed in Boost.Regex, reassigning to the Graph lib. </p> Ticket Jeremiah Willcock Sat, 03 Mar 2012 19:03:58 GMT status changed; resolution set https://svn.boost.org/trac10/ticket/5835#comment:4 https://svn.boost.org/trac10/ticket/5835#comment:4 <ul> <li><strong>status</strong> <span class="trac-field-old">new</span> → <span class="trac-field-new">closed</span> </li> <li><strong>resolution</strong> → <span class="trac-field-new">wontfix</span> </li> </ul> <p> This code (<code>iohb.[ch]</code>) is in an example that is not even compiled by default, and the example (<code>minimum_degree_ordering.cpp</code>) never calls the functions in <code>iohb.c</code> that use <code>sprintf</code>. Thus, I see no reason to change anything unless there are problems in the Harwell-Boeing reader code. </p> Ticket noloader@… Sat, 03 Mar 2012 19:19:20 GMT <link>https://svn.boost.org/trac10/ticket/5835#comment:5 </link> <guid isPermaLink="false">https://svn.boost.org/trac10/ticket/5835#comment:5</guid> <description> <blockquote class="citation"> <p> set to wontfix </p> </blockquote> <p> +1 for the freetards </p> <p> Provide broken code, waste a persons time who has to audit the broken code, and then claim its not even used. WTF? </p> </description> <category>Ticket</category> </item> </channel> </rss>