You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by sf...@apache.org on 2009/10/18 22:39:05 UTC
svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
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];
+ char l[3 * MAX_STRING_LEN];
char w[MAX_STRING_LEN];
char x[MAX_STRING_LEN];
int found;
Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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.
Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
Posted by Guenter Knauf <fu...@apache.org>.
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@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.
Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
Posted by Guenter Knauf <fu...@apache.org>.
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.
Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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@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@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.
Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
Posted by Guenter Knauf <fu...@apache.org>.
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@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@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.
Re: svn commit: r826520 - /httpd/httpd/trunk/support/htdigest.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/18/2009 10:39 PM, sf@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