You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ni...@apache.org on 2009/12/21 01:40:07 UTC
svn commit: r892678 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Author: niq
Date: Mon Dec 21 00:40:07 2009
New Revision: 892678
URL: http://svn.apache.org/viewvc?rev=892678&view=rev
Log:
Reject requests containing (invalid) NULL characters in request line
or request headers.
PR 43039
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/server/protocol.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=892678&r1=892677&r2=892678&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Dec 21 00:40:07 2009
@@ -19,6 +19,9 @@
a new request.
PR 47087 [Nick Kew]
+ *) Core: reject NULLs in request line or request headers.
+ PR 43039 [Nick Kew]
+
Changes with Apache 2.3.4
*) Replace AcceptMutex, LockFile, RewriteLock, SSLMutex, SSLStaplingMutex,
Modified: httpd/httpd/trunk/server/protocol.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=892678&r1=892677&r2=892678&view=diff
==============================================================================
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Mon Dec 21 00:40:07 2009
@@ -431,8 +431,13 @@
}
}
}
-
*read = bytes_handled;
+
+ /* PR#43039: We shouldn't accept NULL bytes within the line */
+ if (strlen(*s) < bytes_handled - 1) {
+ return APR_EINVAL;
+ }
+
return APR_SUCCESS;
}
@@ -609,6 +614,9 @@
else if (rv == APR_TIMEUP) {
r->status = HTTP_REQUEST_TIME_OUT;
}
+ else if (rv == APR_EINVAL) {
+ r->status = HTTP_BAD_REQUEST;
+ }
return 0;
}
} while ((len <= 0) && (++num_blank_lines < max_blank_lines));
@@ -897,9 +905,16 @@
/* Get the request... */
if (!read_request_line(r, tmp_bb)) {
- if (r->status == HTTP_REQUEST_URI_TOO_LARGE) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
- "request failed: URI too long (longer than %d)", r->server->limit_req_line);
+ if (r->status == HTTP_REQUEST_URI_TOO_LARGE
+ || r->status == HTTP_BAD_REQUEST) {
+ if (r->status == HTTP_BAD_REQUEST) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "request failed: invalid characters in URI");
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "request failed: URI too long (longer than %d)", r->server->limit_req_line);
+ }
ap_send_error_response(r, 0);
ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
ap_run_log_transaction(r);
RE: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Nicholas.Kew@Sun.COM On
> Behalf Of Nick Kew
> Sent: Montag, 21. Dezember 2009 10:36
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r892678 - in /httpd/httpd/trunk:
> CHANGES server/protocol.c
>
>
> On 21 Dec 2009, at 07:22, Ruediger Pluem wrote:
>
> > Why bytes_handled - 1 and not bytes_handled?
>
> Terminating null.
>
> (we know the last byte actually handled was LF because we got
> a line-end).
Right, but IMHO bytes_handled does not consider the terminating null,
but only the length of the real payload:
*last_char = '\0';
bytes_handled = last_char - *s;
If the string would be empty bytes_handled would be 0.
Regards
Rüdiger
Re: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Paul Querna <pa...@querna.org>.
On Mon, Dec 21, 2009 at 4:41 PM, Nick Kew <ni...@webthing.com> wrote:
> Paul Querna wrote:
>>
>> On Mon, Dec 21, 2009 at 2:39 AM, "Plüm, Rüdiger, VF-Group"
>> <ru...@vodafone.com> wrote:
>>>
>>> Please reconsider and fix.
>
> Done, thanks.
>
>> I am also slightly concerned about changing the behavoir of
>> ap_rgetline_core in regards to embedded NULL bytes, since this is not
>> just used by HTTP protocol handlers, it appears it could easily be
>> used by other protocols... at a minimum I think there should be a
>> comment in the header docs about this behavior change?
>
> From protocol.c
> /* Get a line of protocol input, including any continuation lines
> * caused by MIME folding (or broken clients) if fold != 0, and place it
> * in the buffer s, of size n bytes, without the ending newline.
>
> From http_protocol.h
> /**
> * Get the next line of input for the request
>
> Are you saying it might be used in a protocol with RFC822-family
> line-based headers but where NULL is allowed? Any examples?
>
> I agree with your point that I should have commented it.
> I'll do that if you're not going to veto the patch.
I'm happy enough with a comment in the header file mentioning the
embedded NULL byte behavior, which is kinda 'weird' because it passes
back a length size_t -- hence why I think it should be documented in
the interface documentation.
Re: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Nick Kew <ni...@webthing.com>.
Paul Querna wrote:
> On Mon, Dec 21, 2009 at 2:39 AM, "Plüm, Rüdiger, VF-Group"
> <ru...@vodafone.com> wrote:
>>
>> Please reconsider and fix.
Done, thanks.
> I am also slightly concerned about changing the behavoir of
> ap_rgetline_core in regards to embedded NULL bytes, since this is not
> just used by HTTP protocol handlers, it appears it could easily be
> used by other protocols... at a minimum I think there should be a
> comment in the header docs about this behavior change?
From protocol.c
/* Get a line of protocol input, including any continuation lines
* caused by MIME folding (or broken clients) if fold != 0, and place it
* in the buffer s, of size n bytes, without the ending newline.
From http_protocol.h
/**
* Get the next line of input for the request
Are you saying it might be used in a protocol with RFC822-family
line-based headers but where NULL is allowed? Any examples?
I agree with your point that I should have commented it.
I'll do that if you're not going to veto the patch.
--
Nick Kew
Re: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Paul Querna <pa...@querna.org>.
On Mon, Dec 21, 2009 at 2:39 AM, "Plüm, Rüdiger, VF-Group"
<ru...@vodafone.com> wrote:
>
>
>> -----Original Message-----
>> From: Nicholas.Kew@Sun.COM [mailto:Nicholas.Kew@Sun.COM] On
>> Behalf Of Nick Kew
>> Sent: Montag, 21. Dezember 2009 10:36
>> To: dev@httpd.apache.org
>> Subject: Re: svn commit: r892678 - in /httpd/httpd/trunk:
>> CHANGES server/protocol.c
>>
>>
>> On 21 Dec 2009, at 07:22, Ruediger Pluem wrote:
>>
>> > Why bytes_handled - 1 and not bytes_handled?
>
> Using bytes_handled - 1 breaks a lot of tests in the perl test framework.
> Using bytes_handled instead fixes this.
> Please reconsider and fix.
I am also slightly concerned about changing the behavoir of
ap_rgetline_core in regards to embedded NULL bytes, since this is not
just used by HTTP protocol handlers, it appears it could easily be
used by other protocols... at a minimum I think there should be a
comment in the header docs about this behavior change?
RE: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Nicholas.Kew@Sun.COM [mailto:Nicholas.Kew@Sun.COM] On
> Behalf Of Nick Kew
> Sent: Montag, 21. Dezember 2009 10:36
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r892678 - in /httpd/httpd/trunk:
> CHANGES server/protocol.c
>
>
> On 21 Dec 2009, at 07:22, Ruediger Pluem wrote:
>
> > Why bytes_handled - 1 and not bytes_handled?
Using bytes_handled - 1 breaks a lot of tests in the perl test framework.
Using bytes_handled instead fixes this.
Please reconsider and fix.
Regards
Rüdiger
Re: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Nick Kew <ni...@webthing.com>.
On 21 Dec 2009, at 07:22, Ruediger Pluem wrote:
> Why bytes_handled - 1 and not bytes_handled?
Terminating null.
(we know the last byte actually handled was LF because we got a line-end).
--
Nick Kew
Re: svn commit: r892678 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 21.12.2009 01:40, niq@apache.org wrote:
> Author: niq
> Date: Mon Dec 21 00:40:07 2009
> New Revision: 892678
>
> URL: http://svn.apache.org/viewvc?rev=892678&view=rev
> Log:
> Reject requests containing (invalid) NULL characters in request line
> or request headers.
> PR 43039
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=892678&r1=892677&r2=892678&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Mon Dec 21 00:40:07 2009
> @@ -431,8 +431,13 @@
> }
> }
> }
> -
> *read = bytes_handled;
> +
> + /* PR#43039: We shouldn't accept NULL bytes within the line */
> + if (strlen(*s) < bytes_handled - 1) {
Why bytes_handled - 1 and not bytes_handled?
> + return APR_EINVAL;
> + }
> +
> return APR_SUCCESS;
> }
>
Regards
Rüdiger