Onderwerp: Re: local buffer overflow in htpasswd for apache 1.3.31 not fixed

* Larry Cashdollar <lwc@vapid.ath.cx> wrote:

> This was posted on the full-disclosure list sept 16 2004 by
> Luiz Fernando.
>=20
> http://archives.neohapsis.com/archiv...4-09/0547.html
>=20
> The nessus check for this vulnerability recommends upgrading to
> Apache version 1.3.32:
>=20
> http://cgi.nessus.org/plugins/dump.php3?id=3D14771
>=20
> But in Apache 1.3.33:
>=20
> lachoy# grep strcpy /install/src/apache_1.3.33/src/support/htpasswd.c
> strcpy(record, user);
> strcpy(pwfilename, argv[i]);
> strcpy(user, argv[i + 1]);
> strcpy(password, argv[i + 2]);
> strcpy(scratch, line);
>=20
> It is still vulnerable.

If you start htpasswd from a shell and you exploit some kind of buffer
overflow, you get ... a shell?. Wow!

Whoever runs htpasswd setuid is darn silly. The httpd developers, the docs =
and
commonsense tell, that the program is just not designed for such an intenti=
on.

Or in other words, take any program that's not designed for being run setuid
and you'll find a "vulnerability" (e.g. with malloc debug environment
variables or the like).

Sorry, I can't see a security vulnerability here.

nd
--=20
> [...] wei=DF jemand zuf=E4llig, was der Tag DIV ausgeschrieben bedeutet?
DIVerses. Benannt nach all dem unstrukturierten Zeug, was die Leute da
so reinpacken und dann absolut positionieren ...
-- Florian Hartig und Lars Kasper in dciwam

Hello,

I can confirm the buffer overflow in htpasswd of apache 1.3.33, for which
Luiz Fernando has written a PoC. ...

On Fri, 29 Oct 2004, Larry Cashdollar wrote:
> This was posted on the full-disclosure list sept 16 2004 by
> Luiz Fernando.
>
> http://archives.neohapsis.com/archiv...4-09/0547.html
[...]
> It is still vulnerable.

But Larrys patch "fixes" a lot of peaces of code, which aren't
vulnerable in my oppinion. A closer look shows, that the calls to strcpy
are protected by if-statements, which prevent a exploitation. Its just one
place where a closing brace ('}') is at the wrong position. ;-)

So, instead of this ...

> 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;
> }
>
> @@ -410,14 +410,14 @@
> fprintf(stderr, "%s: filename too long\n", argv[0]);
> return ERR_OVERFLOW;
> }
> - strcpy(pwfilename, argv[i]);
> + strncpy(pwfilename, argv[i], MAX_STRING_LEN-1);
> if (strlen(argv[i + 1]) > (sizeof(user) - 1)) {
> fprintf(stderr, "%s: username too long (>%lu)\n", argv[0],
> (unsigned long)(sizeof(user) - 1));
> return ERR_OVERFLOW;
> }
> }
> - strcpy(user, argv[i + 1]);
> + strncpy(user, argv[i + 1],MAX_STRING_LEN-1);
> if ((arg = strchr(user, ':')) != NULL) {
> fprintf(stderr, "%s: username contains illegal character
> '%c'\n",
> argv[0], *arg);
> @@ -429,7 +429,7 @@
> (unsigned long)(sizeof(password) - 1));
> return ERR_OVERFLOW;
> }
> - strcpy(password, argv[i + 2]);
> + strncpy(password, argv[i + 2],MAX_STRING_LEN - 1 );
> }
>
> #ifdef WIN32
> @@ -553,7 +553,7 @@
> putline(ftemp, line);
> continue;
> }
> - strcpy(scratch, line);
> + strncpy(scratch, line,MAX_STRING_LEN -1);
> /*
> * See if this is our user.
> */

.... I suggest the following shorter one, which will give us also a correct
error message instead of eventually filling htpasswd with "short" entries:

| --- src/support/htpasswd.c.orig Fri Feb 20 23:02:24 2004
| +++ src/support/htpasswd.c Fri Oct 29 21:13:36 2004
| @@ -411,11 +411,11 @@
| return ERR_OVERFLOW;
| }
| strcpy(pwfilename, argv[i]);
|- if (strlen(argv[i + 1]) > (sizeof(user) - 1)) {
|- fprintf(stderr, "%s: username too long (>%lu)\n", argv[0],
|- (unsigned long)(sizeof(user) - 1));
|- return ERR_OVERFLOW;
|- }
|+ }
|+ if (strlen(argv[i + 1]) > (sizeof(user) - 1)) {
|+ fprintf(stderr, "%s: username too long (>%lu)\n", argv[0],
|+ (unsigned long)(sizeof(user) - 1));
|+ return ERR_OVERFLOW;
| }
| strcpy(user, argv[i + 1]);
| if ((arg = strchr(user, ':')) != NULL) {

This bug exists in 1.3.31, 1.3.32 and 1.3.33. I didn't test other
versions. As I don't find a entry in the bug database, I reported that bug
to the apache httpd people. The Bug ID is #31975.

Yours, Michi.
----------------------------------------------------------------------------
Michael Engert michi@engert.org
80337 München