We were doing some code reviews on the new Win7 SDK samples the other day and one of the code reviewers noticed that the code used wcslen to compute the length of a string.
He pointed out that the SDL Banned API page calls out strlen/wcslen as being banned APIs:
For critical functions, such as those accepting anonymous Internet connections, strlen must also be replaced: Table 19. Banned string length functions and replacements Banned APIs StrSafe Replacement Safe CRT Replacement strlen, wcslen, _mbslen, _mbstrlen, StrLen, lstrlen String*Length strnlen_s
For critical functions, such as those accepting anonymous Internet connections, strlen must also be replaced:
Table 19. Banned string length functions and replacements
I was quite surprised to see this, since I’m not aware of any issues where the use of strlen/wcslen could cause security bugs.
I asked Michael Howard about this and his response was that Table 19 has a typo – the word “server” is missing in the text, it should be “For critical server functions, such as those accepting anonymous Internet connections, strlen must also be replaced”.
Adding that one word makes all the difference. And it makes sense – if you’re a server and accepting anonymous data over the internet, an attacker could cause you to crash by issuing a non null terminated string that was long enough – banning the API forces the developer to think about the length of the string.
Somewhat OT, but I also think that the table is poorly formatted – the “For critical…” text should be AFTER the table title – the way the text is written, it appears to be a part of the previous section instead of being attached as explanatory text on Table 19 (but that’s just the editor in me).
Apparently in SDL v5.0 (which hasn’t yet shipped) the *len functions are removed from the banned API list entirely.
It makes perfect sense that mbslen is banned. Too easy to misuse number of MB characters as the string length. And if there is a malformed multi-byte character, who knows if mbslen won't barf. You can hope.
strlen can turn bad, if you happen to use strncpy which doesn't always nul-terminate the buffer.
Good point Alex - mbslen IS one of those icky APIs. But the others aren't.
And strlen isn't the problem with the strlen/strncpy usage - it's strncpy which is correctly on the list of banned APIs.
Is there any plan to add a safe version of MultiByteToWideChar?
I appreciate and try to use the "safe" functions and it seems like this is a key API that's missed the treatment. (Maybe because it's in Win32 rather than the CRT.)
As per the docs, it's very difficult to call safely. Very commonly used API, too, especially when dealing with internet traffic and binary data file formats (two scenarios where the safe APIs tend to be very important).
(Of the top of my head, it's got issues with unterminated inputs, termination of inputs longer than your buffer, inputs that get truncated with half a surrogate at the end of the buffer (docs suggest the API deals with that on Vista but might mean something else), different API null-term behaviour depending on whether the input is null-terminated and/or whether you specify a length and/or whether the length includes the null. May be remembering incorrectly for some of them but that sort of thing. I had to write a little test app to see how the API behaved in some of those cases as it's not all documented.)
I recently wrote code to convert UTF8 tags from music files into UTF16. (Even ignoring malicious input, there are so many badly written music taggers out there producing dodgy tags that you've got to be careful.) Writing & testing a wrapper function which, I hope, deals with all the cases was fiddly & time consuming and makes me wonder if I thought of everything.
Its nice to see your blog alive again Larry!! its been a while since you posted.. W7 Beta/RC audio must have uncovered some nasty bug in audio sub system keeping you away from bloging..
OT, but ...
Larry, welcome back! You've been missed. I'm sure you're busy with Windows 7 but I'm looking forward to more posts. You're one of a select few Microsoft blogs I read regularly.
OT, but the table itself is fixed width, causing it to be truncated. There is seriously no reason for it to be fixed width in this case.
Make fit to the width of the browser so that the contents aren't truncated.