Mailing List Archive

On Tue, Sep 7, 2021 at 2:00 PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Mon, May 25, 2020 at 7:50 AM <rpluem@apache.org> wrote:
> >
> > Author: rpluem
> > Date: Mon May 25 05:50:12 2020
> > New Revision: 1878092
> >
> > URL: http://svn.apache.org/viewvc?rev=1878092&view=rev
> > Log:
> > Fix a NULL pointer dereference
> >
> > * server/scoreboard.c (ap_increment_counts): In certain cases like certain
> > invalid requests r->method might be NULL here. r->method_number defaults
> > to M_GET and hence is M_GET in these cases.
> >
> >
> > Modified:
> > httpd/httpd/trunk/server/scoreboard.c
> >
> > Modified: httpd/httpd/trunk/server/scoreboard.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1878092&r1=1878091&r2=1878092&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/server/scoreboard.c (original)
> > +++ httpd/httpd/trunk/server/scoreboard.c Mon May 25 05:50:12 2020
> > @@ -381,7 +381,7 @@ AP_DECLARE(void) ap_increment_counts(ap_
> > if (pfn_ap_logio_get_last_bytes != NULL) {
> > bytes = pfn_ap_logio_get_last_bytes(r->connection);
> > }
> > - else if (r->method_number == M_GET && r->method[0] == 'H') {
> > + else if (r->method_number == M_GET && r->method && r->method[0] == 'H') {
> > bytes = 0;
> > }
> > else {
>
> Sorry for the lateness..
> Maybe we could have an r->method_number == M_INVALID and r->method ==
> "" by default on failure, like with the attached patch?

Or even initialize r->{method,uri,unparsed_uri} to "-" for logging (v2
attached).
We can ap_die() from here, but the potential ap_internal_redirect()
from there would rewrite those.

On Tue, Sep 7, 2021 at 8:18 AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Tue, Sep 7, 2021 at 2:00 PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> >
> > On Mon, May 25, 2020 at 7:50 AM <rpluem@apache.org> wrote:
> > >
> > > Author: rpluem
> > > Date: Mon May 25 05:50:12 2020
> > > New Revision: 1878092
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1878092&view=rev
> > > Log:
> > > Fix a NULL pointer dereference
> > >
> > > * server/scoreboard.c (ap_increment_counts): In certain cases like certain
> > > invalid requests r->method might be NULL here. r->method_number defaults
> > > to M_GET and hence is M_GET in these cases.
> > >
> > >
> > > Modified:
> > > httpd/httpd/trunk/server/scoreboard.c
> > >
> > > Modified: httpd/httpd/trunk/server/scoreboard.c
> > > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1878092&r1=1878091&r2=1878092&view=diff
> > > ==============================================================================
> > > --- httpd/httpd/trunk/server/scoreboard.c (original)
> > > +++ httpd/httpd/trunk/server/scoreboard.c Mon May 25 05:50:12 2020
> > > @@ -381,7 +381,7 @@ AP_DECLARE(void) ap_increment_counts(ap_
> > > if (pfn_ap_logio_get_last_bytes != NULL) {
> > > bytes = pfn_ap_logio_get_last_bytes(r->connection);
> > > }
> > > - else if (r->method_number == M_GET && r->method[0] == 'H') {
> > > + else if (r->method_number == M_GET && r->method && r->method[0] == 'H') {
> > > bytes = 0;
> > > }
> > > else {
> >
> > Sorry for the lateness..
> > Maybe we could have an r->method_number == M_INVALID and r->method ==
> > "" by default on failure, like with the attached patch?
>
> Or even initialize r->{method,uri,unparsed_uri} to "-" for logging (v2
> attached).
> We can ap_die() from here, but the potential ap_internal_redirect()
> from there would rewrite those.

looks reasonable to me

--
Eric Covener
covener@gmail.com

On 9/7/21 2:00 PM, Yann Ylavic wrote:
> On Mon, May 25, 2020 at 7:50 AM <rpluem@apache.org> wrote:
>>
>> Author: rpluem
>> Date: Mon May 25 05:50:12 2020
>> New Revision: 1878092
>>
>> URL: http://svn.apache.org/viewvc?rev=1878092&view=rev
>> Log:
>> Fix a NULL pointer dereference
>>
>> * server/scoreboard.c (ap_increment_counts): In certain cases like certain
>> invalid requests r->method might be NULL here. r->method_number defaults
>> to M_GET and hence is M_GET in these cases.
>>
>>
>> Modified:
>> httpd/httpd/trunk/server/scoreboard.c
>>
>> Modified: httpd/httpd/trunk/server/scoreboard.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1878092&r1=1878091&r2=1878092&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/server/scoreboard.c (original)
>> +++ httpd/httpd/trunk/server/scoreboard.c Mon May 25 05:50:12 2020
>> @@ -381,7 +381,7 @@ AP_DECLARE(void) ap_increment_counts(ap_
>> if (pfn_ap_logio_get_last_bytes != NULL) {
>> bytes = pfn_ap_logio_get_last_bytes(r->connection);
>> }
>> - else if (r->method_number == M_GET && r->method[0] == 'H') {
>> + else if (r->method_number == M_GET && r->method && r->method[0] == 'H') {
>> bytes = 0;
>> }
>> else {
>
> Sorry for the lateness..
> Maybe we could have an r->method_number == M_INVALID and r->method ==
> "" by default on failure, like with the attached patch?


Instead of the above or on top?

Regards

Rüdiger

On 9/7/21 2:18 PM, Yann Ylavic wrote:
> Index: server/protocol.c
> ===================================================================
> --- server/protocol.c (revision 1893001)
> +++ server/protocol.c (working copy)
> @@ -716,6 +716,13 @@ static int read_request_line(request_rec *r, apr_b
> if (rv != APR_SUCCESS) {
> r->request_time = apr_time_now();
>
> + /* Fall through with an invalid (non NULL) request */
> + r->method = "-";

In line 1484 of server/protocol.c we check for r->method being NULL to determine and log that we had a malformed request line
(AH00566). Hence this line would need to be adjusted as well.

> + r->method_number = M_INVALID;
> + r->uri = r->unparsed_uri = apr_pstrdup(r->pool, "-");

I would leave this NULL as in the normal path we don't need it and thus we are wasting pool memory with each request.

> + r->proto_num = HTTP_VERSION(1,0);
> + r->protocol = "HTTP/1.0";
> +
> /* ap_rgetline returns APR_ENOSPC if it fills up the
> * buffer before finding the end-of-line. This is only going to
> * happen if it exceeds the configured limit for a request-line.
> @@ -732,8 +739,6 @@ static int read_request_line(request_rec *r, apr_b
> else if (APR_STATUS_IS_EINVAL(rv)) {
> r->status = HTTP_BAD_REQUEST;
> }
> - r->proto_num = HTTP_VERSION(1,0);
> - r->protocol = "HTTP/1.0";
> return 0;
> }
> } while ((len <= 0) && (--num_blank_lines >= 0));

Regards

Rüdiger

On Tue, Sep 7, 2021 at 3:05 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 9/7/21 2:18 PM, Yann Ylavic wrote:
> > Index: server/protocol.c
> > ===================================================================
> > --- server/protocol.c (revision 1893001)
> > +++ server/protocol.c (working copy)
> > @@ -716,6 +716,13 @@ static int read_request_line(request_rec *r, apr_b
> > if (rv != APR_SUCCESS) {
> > r->request_time = apr_time_now();
> >
> > + /* Fall through with an invalid (non NULL) request */
> > + r->method = "-";
>
> In line 1484 of server/protocol.c we check for r->method being NULL to determine and log that we had a malformed request line
> (AH00566). Hence this line would need to be adjusted as well.

Good catch, I will fix it.

>
> > + r->method_number = M_INVALID;
> > + r->uri = r->unparsed_uri = apr_pstrdup(r->pool, "-");
>
> I would leave this NULL as in the normal path we don't need it and thus we are wasting pool memory with each request.

In the "spirit" of not leaving NULLs on error for (eventually
third-party-)modules hooks/filters, this looks sensible to me. Also
r->pool is in its very youth here, probably not near the 8K boundary
so these two bytes are almost free..
Not a strong opinion though, it looks to me like r->[unparsed_]uri
could be accessed/logged from anywhere, and we call ap_die() in more
cases than we did two or three releases ago (since the error handling
changes in ap_read_request(), can't remember which version it was).


Thanks;
Yann.

On Tue, Sep 7, 2021 at 3:05 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 9/7/21 2:00 PM, Yann Ylavic wrote:
> > On Mon, May 25, 2020 at 7:50 AM <rpluem@apache.org> wrote:
> >>
> >> Modified: httpd/httpd/trunk/server/scoreboard.c
> >> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1878092&r1=1878091&r2=1878092&view=diff
> >> ==============================================================================
> >> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >> +++ httpd/httpd/trunk/server/scoreboard.c Mon May 25 05:50:12 2020
> >> @@ -381,7 +381,7 @@ AP_DECLARE(void) ap_increment_counts(ap_
> >> if (pfn_ap_logio_get_last_bytes != NULL) {
> >> bytes = pfn_ap_logio_get_last_bytes(r->connection);
> >> }
> >> - else if (r->method_number == M_GET && r->method[0] == 'H') {
> >> + else if (r->method_number == M_GET && r->method && r->method[0] == 'H') {
> >> bytes = 0;
> >> }
> >> else {
> >
> > Sorry for the lateness..
> > Maybe we could have an r->method_number == M_INVALID and r->method ==
> > "" by default on failure, like with the attached patch?
>
> Instead of the above or on top?

I'd say on top, the above can't hurt ;)

Regards;
Yann.

On 9/7/21 4:16 PM, Yann Ylavic wrote:
> On Tue, Sep 7, 2021 at 3:05 PM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>> On 9/7/21 2:00 PM, Yann Ylavic wrote:
>>> On Mon, May 25, 2020 at 7:50 AM <rpluem@apache.org> wrote:
>>>>
>>>> Modified: httpd/httpd/trunk/server/scoreboard.c
>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1878092&r1=1878091&r2=1878092&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/server/scoreboard.c (original)
>>>> +++ httpd/httpd/trunk/server/scoreboard.c Mon May 25 05:50:12 2020
>>>> @@ -381,7 +381,7 @@ AP_DECLARE(void) ap_increment_counts(ap_
>>>> if (pfn_ap_logio_get_last_bytes != NULL) {
>>>> bytes = pfn_ap_logio_get_last_bytes(r->connection);
>>>> }
>>>> - else if (r->method_number == M_GET && r->method[0] == 'H') {
>>>> + else if (r->method_number == M_GET && r->method && r->method[0] == 'H') {
>>>> bytes = 0;
>>>> }
>>>> else {
>>>
>>> Sorry for the lateness..
>>> Maybe we could have an r->method_number == M_INVALID and r->method ==
>>> "" by default on failure, like with the attached patch?
>>
>> Instead of the above or on top?
>
> I'd say on top, the above can't hurt ;)

+1

Regards

Rüdiger

On 9/7/21 4:04 PM, Yann Ylavic wrote:
> On Tue, Sep 7, 2021 at 3:05 PM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>> On 9/7/21 2:18 PM, Yann Ylavic wrote:
>>> Index: server/protocol.c
>>> ===================================================================
>>> --- server/protocol.c (revision 1893001)
>>> +++ server/protocol.c (working copy)
>>> @@ -716,6 +716,13 @@ static int read_request_line(request_rec *r, apr_b
>>> if (rv != APR_SUCCESS) {
>>> r->request_time = apr_time_now();
>>>
>>> + /* Fall through with an invalid (non NULL) request */
>>> + r->method = "-";
>>
>> In line 1484 of server/protocol.c we check for r->method being NULL to determine and log that we had a malformed request line
>> (AH00566). Hence this line would need to be adjusted as well.
>
> Good catch, I will fix it.
>
>>
>>> + r->method_number = M_INVALID;
>>> + r->uri = r->unparsed_uri = apr_pstrdup(r->pool, "-");
>>
>> I would leave this NULL as in the normal path we don't need it and thus we are wasting pool memory with each request.
>
> In the "spirit" of not leaving NULLs on error for (eventually
> third-party-)modules hooks/filters, this looks sensible to me. Also
> r->pool is in its very youth here, probably not near the 8K boundary
> so these two bytes are almost free..
> Not a strong opinion though, it looks to me like r->[unparsed_]uri
> could be accessed/logged from anywhere, and we call ap_die() in more
> cases than we did two or three releases ago (since the error handling
> changes in ap_read_request(), can't remember which version it was).

Another thought that came up with me in a similar fashion like AH00566: Are we sure that that r->uri == r->unparsed_uri == NULL is
not used somewhere as an error check? Furthermore if r->uri != NULL doesn't the code elsewhere assume that is does contain a valid
path starting with '/' ?


Regards

Rüdiger