[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: [PATCH] httpd: Pass custom HTTP headers to CGI scripts
From: Alexander Vickberg <wickbergster () gmail ! com>
Date: 2019-03-29 7:13:06
Message-ID: CAAxSGfYnBH4=QMusALVY-GK4PcKnp9+FWzuxD0iThe-cyB10Yw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Hi,
Thank you for your valuable feedback Assaf.
Den tors 28 mars 2019 kl 18:33 skrev Assaf Gordon:
>
> Hello Alexander,
>
> On 2019-03-28 11:04 a.m., Alexander Vickberg wrote:
> > This patch creates a list of unmatched HTTP headers and sets up
> > environment variables before running the CGI script.
>
> I assume this is inspired by my (more limited) patch
> of passing "Content-encoding" header:
> http://lists.busybox.net/pipermail/busybox/2019-March/087141.html
> (or, just a very strange timing coincidence?).
>
> I like your patch and of course, if it is accepted,
> mine isn't needed.
However unlikely it might seem I didn't see your patch until after.
Besides I wanted to be able to use completely custom headers for
which there has to be a general approach implementation.
> two small comments:
>
> > @@ -417,6 +423,7 @@ struct globals {
> > IF_FEATURE_HTTPD_CGI(char *host;)
> > IF_FEATURE_HTTPD_CGI(char *http_accept;)
> > IF_FEATURE_HTTPD_CGI(char *http_accept_language;)
> > +IF_FEATURE_HTTPD_CGI(HTTP_Header *hdr_list;)
>
> Since your mechanism is now much more generic than
> the hard-coded CGI headers, perhaps they can
> be safely removed?
> i.e. host/http_accept/http_accept_language/cookie/referer .
>
> Seems like this could save some space.
Good point. I agree.
> > +HTTP_Header *cur = xzalloc(sizeof(HTTP_Header));
> > +char *after_colon = strchr(iobuf, ':');
> > +char *ch = iobuf;
> > +
> > +if (!after_colon)
> > + continue;
> > +
>
> I think the combination of "xzalloc" + "continue"
> opens the possibility of a resource leak -
> if a malicious client sends lots of HTTP header lines without
> a colon, there's no corresponding "free".
Another good catch. I also found another issue that if the requested URL is
not a CGI then the list will be populated but never freed. However I see
other
places which do allocs or strdups and not freeing them. I wonder if that
behavior is counting on that the fork handling the request will exit and
that
then the kernel will free it for us?
> regards,
> - assaf
/Alexander
[Attachment #5 (text/html)]
<div dir="ltr"><div dir="ltr"><div dir="ltr"><div \
dir="ltr"><div>Hi,</div><div><br></div><div>Thank you for your valuable feedback \
Assaf.</div><div dir="ltr"><br></div><div dir="ltr">Den tors 28 mars 2019 kl 18:33 \
skrev Assaf Gordon:<br>><br>> Hello Alexander,<br>><br>> On 2019-03-28 \
11:04 a.m., Alexander Vickberg wrote:<br>> > This patch creates a list of \
unmatched HTTP headers and sets up<br>> > environment variables before running \
the CGI script.<br>><br>> I assume this is inspired by my (more limited) \
patch<br>> of passing "Content-encoding" header:<br>> <a \
href="http://lists.busybox.net/pipermail/busybox/2019-March/087141.html">http://lists.busybox.net/pipermail/busybox/2019-March/087141.html</a><br>> \
(or, just a very strange timing coincidence?).<br>><br>> I like your patch and \
of course, if it is accepted,<br>> mine isn't needed.</div><div \
dir="ltr"><br><div>However unlikely it might seem I didn't see your patch until \
after.</div><div>Besides I wanted to be able to use completely custom headers \
for</div><div>which there has to be a general approach implementation.</div><br>> \
two small comments:<br>><br>> > @@ -417,6 +423,7 @@ struct globals {<br>> \
> IF_FEATURE_HTTPD_CGI(char *host;)<br>> > IF_FEATURE_HTTPD_CGI(char \
*http_accept;)<br>> > IF_FEATURE_HTTPD_CGI(char *http_accept_language;)<br>> \
> +IF_FEATURE_HTTPD_CGI(HTTP_Header *hdr_list;)<br>><br>> Since your \
mechanism is now much more generic than<br>> the hard-coded CGI headers, perhaps \
they can<br>> be safely removed?<br>> i.e. \
host/http_accept/http_accept_language/cookie/referer .<br>><br>> Seems like \
this could save some space.<br><br>Good point. I agree.<br><br>> > +HTTP_Header \
*cur = xzalloc(sizeof(HTTP_Header));<br>> > +char *after_colon = strchr(iobuf, \
':');<br>> > +char *ch = iobuf;<br>> > +<br>> > +if \
(!after_colon)<br>> > + continue;<br>> > +<br>><br>> I think \
the combination of "xzalloc" + "continue"<br>> opens the \
possibility of a resource leak -<br>> if a malicious client sends lots of HTTP \
header lines without<br>> a colon, there's no corresponding \
"free".<br><br><div dir="ltr">Another good catch. I also found another \
issue that if the requested URL is</div><div dir="ltr">not a CGI then the list will \
be populated but never freed. However I see other</div><div dir="ltr">places which do \
allocs or strdups and not freeing them. I wonder if that</div><div dir="ltr">behavior \
is counting on that the fork handling the request will exit and that</div><div \
dir="ltr">then the kernel will free it for us?</div><br>> regards,<br>> - \
assaf<br></div><div><br></div><div>/Alexander</div><div \
dir="ltr"><br></div></div></div></div></div>
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic