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 http://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 http://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 (22)

  1. Senkwe says:

    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.

  2. Michael Howard says:

    You are absolutely correct – this is not an Apache Group fix, and I updated the text to reflect this.

  3. Mike Dimmick says:

    It almost looks as though that user has blindly searched for strcpy in the sources and replaced with strncpy – missing the point that strcpy *can* be safe *if-and-only-if* you’ve already checked the length of the string you’re going to copy and you’re sure it will fit. The line just above the diff context reads:

    if ((strlen(user) + 1 + strlen(cpw)) > (rlen – 1)) {

    where record and rlen are arguments to the function.

    Mind you, if you’ve done that, you might as well call memcpy – since you already know the length, there’s no need to check for the null terminator, requiring only a counted copy.

    This reinforces my general feeling about open source. If this is the quality of the ‘many eyes’ looking at the code, how can you say that the quality will be in any way improved over commercial development – or even equal it? This user knows just enough to be dangerous.

    Even more ironic is that this code appears to be part of a user tool (also named htpasswd) for generating .htpasswd files. It could generate erroneous files, but is unlikely to be risky unless it’s configured to be setuid on a UNIX system, in which case a local user could elevate their privileges. That’s specifically advised against in a comment at the top of the source file.

  4. - says:

    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)

  5. SRD says:

    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.

  6. Saurabh Jain says:

    Well, that’s why VS 2005 CRT has secure versions of these functions. Checkout strncat_s at http://msdn2.microsoft.com/library/w6w3kbaf.aspx.

  7. Smeg says:

    I can back up what I say. Not only gateways, devices too.

  8. Smeg says:

    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.

  9. Smeg says:

    Bad documentation means bad implementation. WAP is a prime example of this (not to mention old MSDN docs :D)

  10. 厚重之刀 says:

    It’s very hard to void some bugs.

  11. Jack Mayhoff says:

    2 words, Static analysis

  12. anonymous says:

    you loath it or you loathe it? 🙂

    Nice article.

  13. Michael Howard says:

    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 😉

  14. Michael Howard says:

    >>2 words, Static analysis

    One word, Education.

  15. Jack Mayhoff says:

    Ahh because yanky doodle education is the best in the world, who would have thunk it, how much did yours cost?

  16. Michael Howard says:

    Right now, few schools teaching secure design and coding, so the slack must be picked up by industry. Simple as that, really.

  17. Michael Howard says:

    >>2 words, Static analysis

    One word, Education.

    Education shouldnt be capitalised after a comma. So much for education on you.

  18. James says:

    > 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…

  19. Essive says:

    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.

  20. A couple of people have asked about the relationship between /GS, SAL and ASLR in Windows Vista. Here’s…

  21. Microsoft Security Expert Michael Howard provides a very technical explanation of the security strategies…