Buffer Overflow in Apache 1.3.xx fixed on Bugtraq - the evils of strncpy and strncat!
This just came in my inbox from Bugtraq, a buffer overrun processing Apache 1.3.x .htpasswd files.
"local buffer overflow in htpasswd for apache 1.3.31 not fixed in .33?" at https://www.securityfocus.com/archive/1/379842/2004-10-26/2004-11-01/0
What was interesting is the fix has a buffer overrun, can you spot it? Note, this is a proposed fix for a publicly known defect, and is not a fix from the Apache Group.
<root@bokchoy:~/tes/apache_1.3.33/src/support># diff -uN htpasswd.orig.c htpasswd.c
--- htpasswd.orig.c 2004-10-28 18:20:13.000000000 -0400
+++ htpasswd.c 2004-10-28 18:17:25.000000000 -0400
@@ -202,9 +202,9 @@
ap_cpystrn(record, "resultant record too long", (rlen - 1));
return ERR_OVERFLOW;
}
- strcpy(record, user);
+ strncpy(record, user,MAX_STRING_LEN - 1);
strcat(record, ":");
- strcat(record, cpw);
+ strncat(record, cpw,MAX_STRING_LEN - 1);
return 0;
}
One caveat, I don't have the Apache source, I'm working solely from this code diff, so I have no context to work with. For example, I don't know how big the "record" variable really is. And I'm assuming strncat is just "good ol' strncat" as defined by the ANSI C standard.
FWIW, I spotted this bug in about 2.5 seconds, we teach issues about common 'n-Functions' security defects as part of the mandatory "Security Basics" training.
Ok, have you spotted the bug yet?
The last call to strncat is wrong, the last arg is not the total buffer size, it's what's left in the buffer. If you want a hint about using strncat safely, look at the MSDN docs "Security note" section https://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclib/html/_crt_strncat.2c_.wcsncat.2c_._mbsncat.asp.
FWIW, I loathe strncpy and strncat, because they are hard to get right, and people think the code is safe simply because an 'n' function is used. Which clearly is not the case in this example.
Comments
- Anonymous
October 29, 2004
Wow :-). Anyway I don't think that was an official fix though, just one guy "patching" his own source. I'm pretty sure this fix wouldn't have made it into the release version. - Anonymous
October 29, 2004
You are absolutely correct - this is not an Apache Group fix, and I updated the text to reflect this. - Anonymous
October 29, 2004
The comment has been removed - Anonymous
October 29, 2004
Someone should forbid standard C text-parsing functions. Time has demonstrated that people fall on them again and again - no matter if it's open or comercial code. C is not a bad language ("too simple" yes, "bad" no) but it seems it's hard to find good C programmers (well, it's difficult to find good programmers no matter what language, that's why someone should write functions that encourage "good code", not like strcpy which only encourages bad code) - Anonymous
October 29, 2004
I presume the first strncpy is used because record is defined to be of size MAX_STRING_LEN. If that is the case then
strcat(record, ":") can also be a buffer overflow (if the string length of "user" is MAX_STRING_LEN or more). Of course, if record was defined as MAX_STRING_LEN + 1 you would be fine. But I share your hatred of these function. I personally prefer using STL and using the append method myself. - Anonymous
October 29, 2004
Well, that's why VS 2005 CRT has secure versions of these functions. Checkout strncat_s at http://msdn2.microsoft.com/library/w6w3kbaf.aspx. - Anonymous
October 30, 2004
I can back up what I say. Not only gateways, devices too. - Anonymous
October 30, 2004
Maybe its not the developer that was wrong, maybe it was his documents explaining it badly? If you want a serious buggy spec, read WAP. I bet I can crash ANY wap gateway within SECONDS ANYTIME ANYPLACE. IF you want proof, set up me a WAP GATEWAY and watch me crash it within microseconds. Want to know how? Hehehe. EVERY WAP GATEWAY IS VULNERABLE, why? BECAUSE THE SPECS ARE SO BADLY DESIGNED and WRITTEN. - Anonymous
October 30, 2004
Bad documentation means bad implementation. WAP is a prime example of this (not to mention old MSDN docs :D) - Anonymous
October 31, 2004
It's very hard to void some bugs. - Anonymous
October 31, 2004
2 words, Static analysis - Anonymous
November 01, 2004
you loath it or you loathe it? :-)
Nice article. - Anonymous
November 01, 2004
That's too funny, I have to be totally honest, i DID NOT KNOW there were two variants. And to be accurate, I LOATHE them :)
You learn something everyday ;) - Anonymous
November 01, 2004
>>2 words, Static analysis
One word, Education. - Anonymous
November 01, 2004
Ahh because yanky doodle education is the best in the world, who would have thunk it, how much did yours cost? - Anonymous
November 01, 2004
Right now, few schools teaching secure design and coding, so the slack must be picked up by industry. Simple as that, really. - Anonymous
November 01, 2004
>>2 words, Static analysis
One word, Education.
Education shouldnt be capitalised after a comma. So much for education on you. - Anonymous
November 02, 2004
> Right now, few schools teaching secure design and coding, so the slack must be picked up by industry.
Simple, sure, but experience has shown the industry is NOT doing a good enough job. The same types of bugs/exploits/problems show up over, and over, and over... - Anonymous
November 23, 2004
The art of programming has definitely declined in the last 10 years. Sloppy coding practices are now the norm - due to too many self-proclaimed programmers from the .com era and vastly inexperienced offshore developers.
When we lived in the C/C++ era the experience, tools and practices were reaching a very mature level by the early to mid-90s. - Anonymous
June 12, 2006
A couple of people have asked about the relationship between /GS, SAL and ASLR in Windows Vista. Here’s... - Anonymous
June 15, 2006
Microsoft Security Expert Michael Howard provides a very technical explanation of the security strategies... - Anonymous
May 29, 2009
The comment has been removed