Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code sample, the answer

What's wrong with this code sample, the answer

Rate This
  • Comments 13

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.

  • "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

  • I don't think any tester in the world could file that as a bug and get it to stick. It's a style problem.

    Under the covers StringCb*A is going to be exactly the same as StringCch*A with one extra line:

       size_t cchDest = cbDest / sizeof(char);

    Yes, strsafe is future-proofing against some time when sizeof(char) might not be 1. And yes, someone might refactor the code to handle Unicode strings. IMO, in the present code as is it's not a bug, though.

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

    I'm the opposite: I don't trust anyone, not even myself.  If the args were NULL, I'd return HR_EINVALIDARG or something.  I'd rather not crash at all.  Of course, the caller can still choke on that hr.

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

  • Chris, a VERY good point.  In fact, from the kernel, touching user mode memory without __try/__except is a security hole (and a really big one).

  • Drew, I've had testers that filed that bug and got it to stick, and I've filed that bug and gotten it to stick.

    Sure, if you're a couple of weeks/months from shipping, it won't stick for the current release, but it's absolutely going to stick in the future.

    And if I'm code reviewing stuff and I see things like that, I won't approve the checkin until it's fixed because the cost of fixing the issue (Cch->Cb) is so low and the future cost of not fixing it is so high.

  • 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@

  • Ok. Then I'm jealous. If early in the cycle your highest pri bug is some non-bug like this, I'm incredibly envious. Really. That would be amazingly cool!

    I point out nits like that in code review but I'd never file them as bugs. This strikes me as something that should be covered in dev coding guidelines if at all. I leave that to devs to sort out. No code bug there but if they have time to make code a little neater more power to the devs!

    I could rant about all of the different "taxes" on our devs that might have contributed to Vista slipping but I'll skip it. As a tester, my idea of quality is most informed by what will affect customers and I hate seeing time spent on anything without direct (measurable?) customer impact.

    Sorry - this is just my tester's eye view of things. I know you have more of a dev perspective.

    Hmm. Maybe I should fire up my own blog again and write there instead of sullying your comments. Again - I apologize. This isn't my soapbox. Thanks for letting me comment, though.

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

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

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

  • 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'

  • Richard, my point was simply that you should never call a Cch function with sizeof(x), because sizeof(x) only generates a Cch value when sizeof(*x)==1.  Instead they should either have used sizeof_array(x) or the Cb version of the function.  The sample was mixing metaphors.

Page 1 of 1 (13 items)