Mailing List Archive: Apache: Dev

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c

Index | Next | Previous | View Threaded

rpluem at apache

Oct 18, 2009, 2:02 PM

Post #1 of 6 (180 views)
Permalink

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c

On 10/18/2009 10:39 PM, sf[at]apache.org wrote:
> Author: sf
> Date: Sun Oct 18 20:39:05 2009
> New Revision: 826520
>
> URL: http://svn.apache.org/viewvc?rev=826520&view=rev
> Log:
> Fix some more overflows spotted by Ruediger Pluem
>
> Modified:
> httpd/httpd/trunk/support/htdigest.c
>
> Modified: httpd/httpd/trunk/support/htdigest.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/support/htdigest.c?rev=826520&r1=826519&r2=826520&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/support/htdigest.c (original)
> +++ httpd/httpd/trunk/support/htdigest.c Sun Oct 18 20:39:05 2009
> @@ -124,7 +124,7 @@
> char *pw;
> apr_md5_ctx_t context;
> unsigned char digest[16];
> - char string[MAX_STRING_LEN];
> + char string[3 * MAX_STRING_LEN];
> char pwin[MAX_STRING_LEN];
> char pwv[MAX_STRING_LEN];
> unsigned int i;
> @@ -188,8 +188,8 @@
> char *dirname;
> char user[MAX_STRING_LEN];
> char realm[MAX_STRING_LEN];
> - char line[MAX_STRING_LEN];
> - char l[MAX_STRING_LEN];
> + char line[3 * MAX_STRING_LEN];

Why do you think that line should be also 3 * MAX_STRING_LEN?
I guess currently it can be MAX_STRING_LEN at max because of line
256:

while (!(get_line(line, MAX_STRING_LEN, f))) {

But maybe this should be changed to

while (!(get_line(line, 3 * MAX_STRING_LEN, f))) {

as a password line could be up to 2 * MAX_STRING_LEN + length of MD5 hash in hex + 1.

Regards

Rüdiger

fuankg at apache

Oct 18, 2009, 2:20 PM

Post #2 of 6 (168 views)
Permalink

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c [In reply to]

Hi,
Ruediger Pluem schrieb:
> Why do you think that line should be also 3 * MAX_STRING_LEN?
> I guess currently it can be MAX_STRING_LEN at max because of line
> 256:
>
> while (!(get_line(line, http://mail-archives.apache.org/mod_mbox/httpd-dev/200107.mbox/%3Csb52b03e.070[at]prv-mail20.provo.novell.com%3E f))) {
>
> But maybe this should be changed to
>
> while (!(get_line(line, 3 * MAX_STRING_LEN, f))) {
>
> as a password line could be up to 2 * MAX_STRING_LEN + length of MD5 hash in hex + 1.

another problem I see here is that MAX_STRING_LEN = 8192 bytes, that
means that already 6*8k are allocated from stack which is a problem at
least on NetWare, as already discussed here back in 2001:
http://mail-archives.apache.org/mod_mbox/httpd-dev/200107.mbox/%3Csb52b03e.070[at]prv-mail20.provo.novell.com%3E
I think for such things like username, password, realm we dont need to
expect more than 256 bytes, but even if we want to be super-save it
would be enough to reserve 512 bytes; so cant we introduce a new define
like:
#define SMALL_STRING_LEN 256
and use this instead within the auth modules for username, password, realm?
1,5k <-> 48k is a huge difference ...

Gün.

sf at sfritsch

Oct 18, 2009, 2:23 PM

Post #3 of 6 (167 views)
Permalink

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c [In reply to]

On Sunday 18 October 2009, Guenter Knauf wrote:
> Hi,
>
> Ruediger Pluem schrieb:
> > Why do you think that line should be also 3 * MAX_STRING_LEN?
> > I guess currently it can be MAX_STRING_LEN at max because of line
> > 256:
> >
> > while (!(get_line(line,
> > http://mail-archives.apache.org/mod_mbox/httpd-dev/200107.mbox/%3
> >Csb52b03e.070[at]prv-mail20.provo.novell.com%3E f))) {
> >
> > But maybe this should be changed to
> >
> > while (!(get_line(line, 3 * MAX_STRING_LEN, f))) {
> >
> > as a password line could be up to 2 * MAX_STRING_LEN + length of
> > MD5 hash in hex + 1.
>
> another problem I see here is that MAX_STRING_LEN = 8192 bytes,
> that means that already 6*8k are allocated from stack which is a
> problem at least on NetWare, as already discussed here back in
> 2001:
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200107.mbox/%3Cs
> b52b03e.070[at]prv-mail20.provo.novell.com%3E I think for such things
> like username, password, realm we dont need to expect more than
> 256 bytes, but even if we want to be super-save it would be enough
> to reserve 512 bytes; so cant we introduce a new define like:
> #define SMALL_STRING_LEN 256
> and use this instead within the auth modules for username,
> password, realm? 1,5k <-> 48k is a huge difference ...
>
> Gün.
>
digest.c already has

#define MAX_STRING_LEN 256

No problem there.

fuankg at apache

Oct 18, 2009, 2:26 PM

Post #4 of 6 (167 views)
Permalink

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c [In reply to]

Hi,
Guenter Knauf schrieb:
> another problem I see here is that MAX_STRING_LEN = 8192 bytes, that
> means that already 6*8k are allocated from stack which is a problem at
> least on NetWare, as already discussed here back in 2001:
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200107.mbox/%3Csb52b03e.070[at]prv-mail20.provo.novell.com%3E
> I think for such things like username, password, realm we dont need to
> expect more than 256 bytes, but even if we want to be super-save it
> would be enough to reserve 512 bytes; so cant we introduce a new define
> like:
> #define SMALL_STRING_LEN 256
> and use this instead within the auth modules for username, password, realm?
> 1,5k <-> 48k is a huge difference ...
just to carify: it was more that I thought I post about this when I saw
the MAX_STRING_LEN * X usage - here in this special case with htdigest.c
its most likely not a problem since its only a support program; however
I did a wuick search through sources, and found some other places in
auth modules (not looked yet further) where I expect this more critical.

Gün.

sf at sfritsch

Oct 18, 2009, 2:26 PM

Post #5 of 6 (167 views)
Permalink

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c [In reply to]

On Sunday 18 October 2009, Ruediger Pluem wrote:
> Why do you think that line should be also 3 * MAX_STRING_LEN?
> I guess currently it can be MAX_STRING_LEN at max because of line
> 256:
>
> while (!(get_line(line, MAX_STRING_LEN, f))) {
>
> But maybe this should be changed to
>
> while (!(get_line(line, 3 * MAX_STRING_LEN, f))) {
>
> as a password line could be up to 2 * MAX_STRING_LEN + length of
> MD5 hash in hex + 1.

I wanted htdigest to be able to read the files it writes and chose 3 *
MAX_STRING_LEN because it is larger than the max password line length.

But I missed the get_line call :-( . Thanks again.

fuankg at apache

Oct 18, 2009, 2:30 PM

Post #6 of 6 (167 views)
Permalink

Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c [In reply to]

Hi Stefan,
Stefan Fritsch schrieb:
> digest.c already has
>
> #define MAX_STRING_LEN 256
>
> No problem there.
ah, thanks for the pointer - I thought the setting was inherited from
the httpd.h define ...

Gün.

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com