You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2014/11/14 23:25:13 UTC
Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c
On Fri, Nov 14, 2014 at 1:18 PM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Fri Nov 14 18:18:15 2014
> New Revision: 1639717
>
> URL: http://svn.apache.org/r1639717
> Log:
> mod_authnz_fcgi: Fix a potential crash with response headers' size above
> 8K.
> (similar to r1638818 for mod_proxy_fcgi).
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1639717&r1=1639716&r2=1639717&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Nov 14 18:18:15 2014
> @@ -5,6 +5,9 @@ Changes with Apache 2.5.0
> mod_proxy_fcgi: Fix a potential crash with response headers' size
> above 8K.
> [Teguh <chain rop.io>, Yann Ylavic]
>
> + *) mod_authnz_fcgi: Fix a potential crash with response headers' size
> above 8K.
> + [Yann Ylavic]
> +
> *) mod_authnz_ldap: Resolve crashes with LDAP authz and non-LDAP authn
> since
> r1608202. [Eric Covener]
>
>
> Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1639717&r1=1639716&r2=1639717&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
> +++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Nov 14
> 18:18:15 2014
> @@ -1 +1 @@
> -2821
> +2822
>
> Modified: httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c?rev=1639717&r1=1639716&r2=1639717&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c Fri Nov 14 18:18:15
> 2014
> @@ -406,13 +406,12 @@ enum {
> *
> * Returns 0 if it can't find the end of the headers, and 1 if it found
> the
> * end of the headers. */
> -static int handle_headers(request_rec *r,
> - int *state,
> - char *readbuf)
> +static int handle_headers(request_rec *r, int *state,
> + char *readbuf, apr_size_t readlen)
> {
> const char *itr = readbuf;
>
> - while (*itr) {
> + while (readlen) {
> if (*itr == '\r') {
> switch (*state) {
> case HDR_STATE_GOT_CRLF:
> @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
> break;
> }
> }
> - else {
> + else if (*itr == '\t' || !apr_iscntrl(*itr)) {
> *state = HDR_STATE_READING_HEADERS;
> }
> + else {
> + return -1;
> + }
>
First, thanks for taking care of this issue.
I guess I don't understand the importance of this code which returns an
error on non-tab control characters. Why isn't the character checking the
same as in ap_scan_script_header_err_brigade_ex() since that is what will
finally parse the buffer?
Thanks!
> if (*state == HDR_STATE_DONE_WITH_HEADERS)
> break;
>
> + --readlen;
> ++itr;
> }
>
> @@ -555,7 +558,17 @@ static apr_status_t handle_response(cons
> APR_BRIGADE_INSERT_TAIL(ob, b);
>
> if (!seen_end_of_headers) {
> - int st = handle_headers(r, &header_state, readbuf);
> + int st = handle_headers(r, &header_state, readbuf,
> + readbuflen);
> +
> + if (st == -1) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> + APLOGNO(02821) "%s: error reading "
> + "headers from %s",
> + fn, conf->backend);
> + rv = APR_EINVAL;
> + break;
> + }
>
> if (st == 1) {
> int status;
> @@ -646,7 +659,7 @@ static apr_status_t handle_response(cons
> /*
> * Read/discard any trailing padding.
> */
> - if (plen) {
> + if (rv == APR_SUCCESS && plen) {
> rv = recv_data_full(conf, r, s, readbuf, plen);
> if (rv != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
>
>
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Nov 15, 2014 at 12:40 AM, Jeff Trawick <tr...@gmail.com> wrote:
> Summary: I don't think it is a big deal in practice but I think it can be
> confusing that the code that just needs to determine if the end has been
> found can discover errors that otherwise would be unnoticed.
I agree, the checks belong to ap_scan_script_header*().
Maybe we need (yet) another ap_scan_script_header_parse(), reentrant
(optionaly taking care of folding), and returning a relevant value for
the caller to know.
All cgi handlers (by the existing functions) would use it.
I will undo these change for now and let all the data reach
ap_scan_script_header_err_brigade_ex(), that's indeed better.
Likewise wrt r1638818.
Thanks for the review,
Yann.
Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 14, 2014 at 6:00 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Nov 14, 2014 at 11:25 PM, Jeff Trawick <tr...@gmail.com> wrote:
> > On Fri, Nov 14, 2014 at 1:18 PM, <yl...@apache.org> wrote:
> []
> >> -static int handle_headers(request_rec *r,
> >> - int *state,
> >> - char *readbuf)
> >> +static int handle_headers(request_rec *r, int *state,
> >> + char *readbuf, apr_size_t readlen)
> >> {
> >> const char *itr = readbuf;
> >>
> >> - while (*itr) {
> >> + while (readlen) {
> >> if (*itr == '\r') {
> >> switch (*state) {
> >> case HDR_STATE_GOT_CRLF:
> >> @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
> >> break;
> >> }
> >> }
> >> - else {
> >> + else if (*itr == '\t' || !apr_iscntrl(*itr)) {
> >> *state = HDR_STATE_READING_HEADERS;
> >> }
> >> + else {
> >> + return -1;
> >> + }
> >
> []
> > I guess I don't understand the importance of this code which returns an
> > error on non-tab control characters. Why isn't the character checking
> the
> > same as in ap_scan_script_header_err_brigade_ex() since that is what will
> > finally parse the buffer?
>
> Well, the previous code was stopping on '\0' (hopefully a '\n' was
> reached before), and I wanted to preserve this behaviour.
> While at it, I chose to also stop on any control char since they are
> not valid in HTTP headers (but '\t').
> getsfunc_BRIGADE() does not seem to care about those, and is not
> supposed to return anything but -1 (timeout) as error...
>
> Isn't handle_headers() handling HTTP headers?
> If not I'm afraid I overdid that fix.
>
handle_headers() is processing these headers just enough to tell when the
complete set of headers is in the brigade, at which point it will be
processed by ap_scan_script_header_err_brigade_ex(). I don't think
catching errors that ap_scan_* don't catch is a serious problem, but it
might cause confusion in some extremely odd cases such as programmers
comparing the code :) or having a "bad" script work with a module that only
uses ap_scan_* but fail with one of these).
Is the header syntax actually supported by ap_scan_script_header* HTTP or
HTTP-like? Here's an easy way out of that: It doesn't support folded
lines AFAICT, so it is HTTP-like ;) But seriously, it seems very plausible
that the CGI spec is intended to describe script headers as following the
HTTP header syntax. (It doesn't bother describing LWS.)
Summary: I don't think it is a big deal in practice but I think it can be
confusing that the code that just needs to determine if the end has been
found can discover errors that otherwise would be unnoticed.
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Nov 14, 2014 at 11:25 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 14, 2014 at 1:18 PM, <yl...@apache.org> wrote:
[]
>> -static int handle_headers(request_rec *r,
>> - int *state,
>> - char *readbuf)
>> +static int handle_headers(request_rec *r, int *state,
>> + char *readbuf, apr_size_t readlen)
>> {
>> const char *itr = readbuf;
>>
>> - while (*itr) {
>> + while (readlen) {
>> if (*itr == '\r') {
>> switch (*state) {
>> case HDR_STATE_GOT_CRLF:
>> @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
>> break;
>> }
>> }
>> - else {
>> + else if (*itr == '\t' || !apr_iscntrl(*itr)) {
>> *state = HDR_STATE_READING_HEADERS;
>> }
>> + else {
>> + return -1;
>> + }
>
[]
> I guess I don't understand the importance of this code which returns an
> error on non-tab control characters. Why isn't the character checking the
> same as in ap_scan_script_header_err_brigade_ex() since that is what will
> finally parse the buffer?
Well, the previous code was stopping on '\0' (hopefully a '\n' was
reached before), and I wanted to preserve this behaviour.
While at it, I chose to also stop on any control char since they are
not valid in HTTP headers (but '\t').
getsfunc_BRIGADE() does not seem to care about those, and is not
supposed to return anything but -1 (timeout) as error...
Isn't handle_headers() handling HTTP headers?
If not I'm afraid I overdid that fix.