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.