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/15 13:57:00 UTC
Re: svn commit: r1639814 - /httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
On Fri, Nov 14, 2014 at 7:37 PM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Sat Nov 15 00:37:13 2014
> New Revision: 1639814
>
> URL: http://svn.apache.org/r1639814
> Log:
> mod_authnz_fcgi: follow up to r1639717.
> Let ap_scan_script_header*() validate the headers.
>
> Modified:
> httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
>
> 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=1639814&r1=1639813&r2=1639814&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c Sat Nov 15 00:37:13
> 2014
> @@ -442,11 +442,8 @@ static int handle_headers(request_rec *r
> break;
> }
> }
> - else if (*itr == '\t' || !apr_iscntrl(*itr)) {
> - *state = HDR_STATE_READING_HEADERS;
> - }
> else {
> - return -1;
> + *state = HDR_STATE_READING_HEADERS;
> }
>
> if (*state == HDR_STATE_DONE_WITH_HEADERS)
>
>
>
I was looking at the diffs for 2.4 and noticed some vestigial code from the
first revision; please check the attached patch to see if you agree with
some additional removals.
Also, my understanding is that
* some of the code in your first revision of both modules catches potential
errors that should have been caught before, so that's an additional issue
that could be mentioned in CHANGES.
* the one CVE should apply to both modules, and the CHANGES entry can be
grouped together. (It could in fact be the same affected application,
which supports both authentication&|authorization and response generation,
using the two modules)
Agreed?
Thanks!
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1639814 - /httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Jeff,
I just post a note here so that you don't miss a comment I made in a
reply (dev@) to commit r1640036.
Thanks,
Yann.
On Sun, Nov 16, 2014 at 11:43 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sun, Nov 16, 2014 at 10:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Sat, Nov 15, 2014 at 1:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>
>>> I was looking at the diffs for 2.4 and noticed some vestigial code from the
>>> first revision; please check the attached patch to see if you agree with
>>> some additional removals.
>>
>> Agreed, I should have reverted the patch and restarted from scratch.
>> To ease review now, I'd better revert the whole and re-commit once for
>> both *fcgi modules, and propose this one for the CVE.
>
> Done in r1640034 (revert), and r1640036/r1640037 (commit/proposal).
>
>>
>>> Also, my understanding is that
>>>
>>> * some of the code in your first revision of both modules catches potential
>>> errors that should have been caught before, so that's an additional issue
>>> that could be mentioned in CHANGES.
>>
>> You are talking about the loop-breakage after the switch() which now
>> catches inner errors (not reverted by your patch), right?
>> I'll propose this change separately (from the CVE commit) then.
>
> Done in r1640040+r1640042 (commits), and r1640045 (proposal).
>
> Thanks again for the review.
Re: svn commit: r1639814 - /httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, Nov 16, 2014 at 10:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Nov 15, 2014 at 1:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>
>> I was looking at the diffs for 2.4 and noticed some vestigial code from the
>> first revision; please check the attached patch to see if you agree with
>> some additional removals.
>
> Agreed, I should have reverted the patch and restarted from scratch.
> To ease review now, I'd better revert the whole and re-commit once for
> both *fcgi modules, and propose this one for the CVE.
Done in r1640034 (revert), and r1640036/r1640037 (commit/proposal).
>
>> Also, my understanding is that
>>
>> * some of the code in your first revision of both modules catches potential
>> errors that should have been caught before, so that's an additional issue
>> that could be mentioned in CHANGES.
>
> You are talking about the loop-breakage after the switch() which now
> catches inner errors (not reverted by your patch), right?
> I'll propose this change separately (from the CVE commit) then.
Done in r1640040+r1640042 (commits), and r1640045 (proposal).
Thanks again for the review.
Re: svn commit: r1639814 - /httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Nov 15, 2014 at 1:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> I was looking at the diffs for 2.4 and noticed some vestigial code from the
> first revision; please check the attached patch to see if you agree with
> some additional removals.
Agreed, I should have reverted the patch and restarted from scratch.
To ease review now, I'd better revert the whole and re-commit once for
both *fcgi modules, and propose this one for the CVE.
> Also, my understanding is that
>
> * some of the code in your first revision of both modules catches potential
> errors that should have been caught before, so that's an additional issue
> that could be mentioned in CHANGES.
You are talking about the loop-breakage after the switch() which now
catches inner errors (not reverted by your patch), right?
I'll propose this change separately (from the CVE commit) then.
> * the one CVE should apply to both modules, and the CHANGES entry can be
> grouped together. (It could in fact be the same affected application, which
> supports both authentication&|authorization and response generation, using
> the two modules)
>
> Agreed?
Yes, clearly.
Regarding HTTP conformance (iscntl() and parsing), everything is
already there in fact, I just didn't look far enough in the chain
(ap_scan_script_header_err_core_ex and finally ap_http_header_filter).
We are just missing LimitResponseFieldSize now, for all proxy modules...
Regards,
Yann.