通过

What's wrong with this code sample, the answer

  • 项目

Yesterday, I posted a question about a security sample I ran into the other day.  I mentioned that the function made me cringe the instant I saw it.

Mike Dunn and Sys64378 danced around the root of what made the function cringeworthy, but Alan nailed it first.

The reason I cringed when looking at the code is that it worked by accident, not by design.

Here's the code again (C&P because it's short):

HRESULT Function(char *s1, char *s2) {
    char temp[32];
    HRESULT hr = StringCchCopy(temp,sizeof(temp),s1);
    if (FAILED(hr)) return hr;
    return StringCchCat(temp,sizeof(temp),s2);
}

The code as written is correct (yes, grue, Luciano and others pointed out that it actually doesn't do anything, but neither did the original code (this is supposed to be a secure version of an existing function).

However the problem in the code becomes blindingly obvious when you follow Sys64738's suggestion:

HRESULT Function(TCHAR *s1, TCHAR *s2) {
    TCHAR temp[32];
    HRESULT hr = StringCchCopy(temp,sizeof(temp),s1);
    if (FAILED(hr)) return hr;
    return StringCchCat(temp,sizeof(temp),s2);
}

The problem is that the function depends on the fact that temp[] is declared as an array of type "char".  Once you redeclare temp (and s1 and s2) as TCHAR's (which is a likely first step in a conversion to unicode), then it would INTRODUCE a security hole the instant that someone set UNICODE=1.

Mike Dunn's answer (use StringCchCopyA and StringCchCatA) would absolutely work in this situation (because when you enabled UNICODE, then you would get a compiler error).

But I think a far better solution would be:

HRESULT Function(char *s1, char *s2) {
    char temp[32];
    HRESULT hr = StringCbCopy(temp,sizeof(temp),s1);
    if (FAILED(hr)) return hr;
    return StringCbCat(temp,sizeof(temp),s2);
}

If you use the Cb version of the safe String functions then you're safe regardless of the type of temp.

 

Things that weren't wrong (IMHO):

Checking for null s1 and s2.  First off, this is sample code (and thus the checks would distract from the point being made), and secondly, I'm a firm believer that you don't check for correctness of input values - IMHO it's far better to crash when a caller violates your API contract than to try to resolve the issue.

Comments

  • Anonymous
    March 09, 2007
    "Checking for null s1 and s2." The StringXxxxEx versions do also that. Safer that safe. http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/resources/strings/stringreference/stringfunctions/stringcbcopyex.asp

  • Anonymous
    March 09, 2007
    The comment has been removed

  • Anonymous
    March 09, 2007
    The comment has been removed

  • Anonymous
    March 09, 2007
    <blockquote> IMHO it's far better to crash when a caller violates your API contract than to try to resolve the issue </blockquote> This depends on the environment. If you're in the kernel, you'd better not crash on an API violation if it was a simple thing to validate. Device driver developers don't always check coverage for all code paths, and better four extra bytes of binary than a BSOD. In userspace, it depends on how easy it is to run a debugger on the code in question.

  • Anonymous
    March 09, 2007
    The comment has been removed

  • Anonymous
    March 09, 2007
    The comment has been removed

  • Anonymous
    March 10, 2007
    Our in-house coding standard requires us to use a NUMCHARS macro instead of sizeof in this usage.  In fact, we have a NUMBYTES macro as well, and vanilla sizeof is not allowed (as it is a sign that the programming didn't think through whether they want CHARS or BYTES). ///d@

  • Anonymous
    March 10, 2007
    The comment has been removed

  • Anonymous
    March 11, 2007
    Why a bug ? Unless it is refactored, it's not a bug. If it gets refactored it should of course being rechecked in its entirety. If you consider this a bug, then it's also missing a critical section! You know some one could refactor temp to being static..

  • Anonymous
    March 11, 2007
    Wouldn't it be better to 'fix' this code by using the _countof macro instead of sizeof?

  • Anonymous
    March 30, 2007
    I agree with Larry, better to crash early (unless in kernel or a driver).

  • Anonymous
    May 14, 2007
    Hi, I dont see the issue.  If someone were to refactor the code to unicode they would HAVE to alter 'char temp[32]' to TCHAR because otherwise it won't compile ! HRESULT Function(TCHAR *s1, TCHAR *s2) {    char temp[32];    HRESULT hr = StringCchCopy(temp,sizeof(temp),s1);    if (FAILED(hr)) return hr;    return StringCchCat(temp,sizeof(temp),s2); } On my system this produces : error C2664: 'StringCchCopyW' : cannot convert parameter 1 from 'char [32]' to 'STRSAFE_LPWSTR'

  • Anonymous
    May 14, 2007
    The comment has been removed