Re: svn commit: r1610674 - in /httpd/httpd/trunk: include/ap_mmn.h include/httpd.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c server/util.c
Hi,
spotted while looking at
https://raw.githubusercontent.com/icing/mod_h2/master/sandbox/httpd/patches/core-protocols.patch
which include it.
CJ
Le 15/07/2014 14:27, jor...@apache.org a écrit :
Author: jorton
Date: Tue Jul 15 12:27:00 2014
New Revision: 1610674
URL: http://svn.apache.org/r1610674
Log:
SECURITY (CVE-2014-0117): Fix a crash in mod_proxy. In a reverse
proxy configuration, a remote attacker could send a carefully crafted
request which could crash a server process, resulting in denial of
service.
Thanks to Marek Kroemeke working with HP's Zero Day Initiative for
reporting this issue.
* server/util.c (ap_parse_token_list_strict): New function.
* modules/proxy/proxy_util.c (find_conn_headers): Use it here.
* modules/proxy/mod_proxy_http.c (ap_proxy_http_process_response):
Send a 400 for a malformed Connection header.
Submitted by: Edward Lu, breser, covener
Modified:
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/include/httpd.h
httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
httpd/httpd/trunk/modules/proxy/proxy_util.c
httpd/httpd/trunk/server/util.c
Modified: httpd/httpd/trunk/server/util.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1610674&r1=1610673&r2=1610674&view=diff
==
--- httpd/httpd/trunk/server/util.c (original)
+++ httpd/httpd/trunk/server/util.c Tue Jul 15 12:27:00 2014
@@ -1452,6 +1452,95 @@ AP_DECLARE(int) ap_find_etag_weak(apr_po
return find_list_item(p, line, tok, AP_ETAG_WEAK);
}
+/* Grab a list of tokens of the format 1#token (from RFC7230) */
+AP_DECLARE(const char *) ap_parse_token_list_strict(apr_pool_t *p,
+const char *str_in,
+apr_array_header_t **tokens,
+int skip_invalid)
+{
+int in_leading_space = 1;
+int in_trailing_space = 0;
+int string_end = 0;
+const char *tok_begin;
+const char *cur;
+
+if (!str_in) {
+return NULL;
+}
+
+tok_begin = cur = str_in;
+
+while (!string_end) {
+const unsigned char c = (unsigned char)*cur;
+
+if (!TEST_CHAR(c, T_HTTP_TOKEN_STOP) && c != '\0') {
+/* Non-separator character; we are finished with leading
+ * whitespace. We must never have encountered any trailing
+ * whitespace before the delimiter (comma) */
+in_leading_space = 0;
+if (in_trailing_space) {
+return "Encountered illegal whitespace in token";
+}
+}
+else if (c == ' ' || c == '\t') {
+/* "Linear whitespace" only includes ASCII CRLF, space, and tab;
+ * we can't get a CRLF since headers are split on them already,
+ * so only look for a space or a tab */
+if (in_leading_space) {
+/* We're still in leading whitespace */
+++tok_begin;
+}
+else {
+/* We must be in trailing whitespace */
+++in_trailing_space;
+}
+}
+else if (c == ',' || c == '\0') {
+if (!in_leading_space) {
+/* If we're out of the leading space, we know we've read some
+ * characters of a token */
+if (*tokens == NULL) {
+*tokens = apr_array_make(p, 4, sizeof(char *));
+}
+APR_ARRAY_PUSH(*tokens, char *) =
+apr_pstrmemdup((*tokens)->pool, tok_begin,
+ (cur - tok_begin) - in_trailing_space);
+}
+/* We're allowed to have null elements, just don't add them to the
+ * array */
+
+tok_begin = cur + 1;
+in_leading_space = 1;
+in_trailing_space = 0;
+string_end = (c == '\0');
+}
+else {
+/* Encountered illegal separator char */
+if (skip_invalid) {
+/* Skip to the next separator */
+const char *temp;
+temp = ap_strchr_c(cur, ',');
+if(!temp) {
+temp = ap_strchr_c(cur, '\0');
+}
+
+/* Act like we haven't seen a token so we reset */
+cur = temp - 1;
If i understand correctly, if we find an invalid char and
'skip_invalid', we first look for the next comma and start searching for
new token from there.
If no comma is found before the trailing NULL, then nothing more can be
found and we end the processing.
So shouldn't this be:
cur = temp + 1;
(i.e. go to the character *following* the comma)
Not tested, but I think that if the invalid character is the one just
before the comma, then we loop forever.
Also if the:
temp = ap_strchr_c(cur, '\0');
above is supposed to reach
Re: [RFC] Enable OCSP Stapling by default in httpd trunk
Jeff Trawick wrote: > > >As of now there's still a veto on my suggestion of enabling stapling by >default in the httpd trunk config. Would that default need to be backported to 2.4.x? If it can be on by default for trunk/2.5.x, and off by default in earlier releases, this should surprise very few users. People upgrading from an older release could get a mild surprise, but at the same time if you upgrade from 2.4.x to 2.5.x then surprises aren't all that surprising. Overall I think the big question mark is around backport to 2.4.x rather than the change to httpd trunk. Tim -- Tim Bannister – is...@c8h10n4o2.org.uk
Re: [RFC] Enable OCSP Stapling by default in httpd trunk
On Mon, Jul 13, 2015 at 3:08 AM, Ruediger Pluem wrote: > > > On 07/11/2015 08:55 PM, William A Rowe Jr wrote: > > > If you are suggesting we shouldn't change the compiled-in default, I can > > agree, POLS (Principal Of Least Surprise). If you are suggesting the > default > > config shouldn't reflect the ability to efficiently handle OCSP by > stapling, here > > I think we would disagree. > > This something I can agree with: Leave the default compiled in to off and > in the configuration distributed by us have it > set to on. > > Regards > > Rüdiger > > As of now there's still a veto on my suggestion of enabling stapling by default in the httpd trunk config. Of Kaspar's justifications, I think this is the text that remains of his clarifying post on July 11, ignoring the license reference: "a) by default, an HTTP server is supposed to process *incoming* requests, not make accidental outgoing connections in addition (at least not unless it's explicitly instructed to do so);" (I sincerely hope I haven't misconstrued the situation or missed anything Kaspar intended.) Those of us in favor of enabling stapling by default for those using the default configs wouldn't have any trouble finding opposite justifications. For one, it is appropriate for the default config is there to enable practices which are reasonable in most situations, and OCSP Stapling is widely accepted as an appropriate feature for HTTP servers to enable. Also, strictly speaking, the default config does not have SSL enabled anyway, and after manually enabling it OCSP responses won't be fetched unless a certificate is configured which references an OCSP responder. While I find the "not make accidental outgoing connections" argument compelling (though perhaps with a different word than "accidental") the server already takes actions that cause outgoing connections to services not explicitly configured (DNS), and these occasionally cause problems. Looking up the values of the User and Group directives can possibly invoke one of multiple different network services depending on the system configuration. But I admit that's just fishing for a counter argument. Any suggestions for breaking the apparent impasse? Is there a principle at stake which could be followed consistently across disparate features in how the server behaves a) with compiled in defaults and minimal config, or b) with default/recommended config? If enabling stapling were more closely tied in the configuration language to configuring a certificate, which with "SSLUseStapling On" is the user action that makes mod_ssl talk to a responder, would that help the end user? (Controlling stapling parameters on a per-certificate basis is valuable anyway since you can have multiple certificates per vhost, possibly with different responders.) Thanks, Jeff -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: HTTP_MISDIRECTED_REQUEST
On Fri, Aug 28, 2015 at 9:26 AM, Stefan Eissing wrote: > If this works, one could think about introducing some kind of "equivalence > number" to speed up the checking, since in certain HTTP/2 setups there might > be a good percentage of requests requiting this verification. Long term we need to block these name-based renegotiations because we'll be at TLS1.3. I don't know how to ween people off, but making up an H2 requirement might be one way to ease people into it.
Re: regression in r1698239
Works fine here. Thanks! > Am 28.08.2015 um 15:14 schrieb Eric Covener : > > Thanks -- back in 1698334 with loops that actually move. > > On Fri, Aug 28, 2015 at 8:55 AM, Eric Covener wrote: >> On Fri, Aug 28, 2015 at 8:54 AM, Stefan Eissing >> wrote: >>> Change 1698239 brings t/apache/pr17629.t into an endless loop now. I built >>> on r1698322 in trunk (OSX + Ubuntu). >>> >>> I revert the change now. A certain "covener" might want to look into this. >>> ;-) >>> >>> Cheers, >> >> >> Thanks, go ahead and revert. > > > > -- > Eric Covener > cove...@gmail.com bytes GmbH Hafenweg 16, 48155 Münster, Germany Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
Re: HTTP_MISDIRECTED_REQUEST
As long as we are waiting for the *real* experts to chime in, I took a first
stab
refactoring the code a tiny bit and adding an additional check against the
selected
server_rec.
If the r->hostname does not match the SNI servername, the r->hostname is used to
check if it matches the sslconn->server. That catches the case where a vhost has
ServerAlias or even wildcard and the client somehow got the notion to use the
TLS
connection for such hosts as well. I think this is risk free as the same
server_rec
should be selected on a new connection with r->hostname in SNI.
(It could be that the match is not the only one and that a vhost in the list
*before* the
current one would be selected. Hmmm...)
Where I want to go with this the following test:
if (r->hostname does not match SNI) {
server_rec *rs = ssl_find_vhost_for(r->hostname);
if (sslconn->server != rs) {
SSLSrvConfigRec *conncfg = mySrvFromConn(c);
SSLSrvConfigRec *reqcfg = mySrvConfig(r->server);
if (!compatibleSslParams(conncfg, reqcfg)) {
return HTTP_MISDIRECTED_REQUEST;
}
}
}
/* all fine, process request */
Where "compatibleSslParams" needs to be defined. I think it needs to match at
least
SSLSrvConfigRec
- enabled
- cipher_server_pref
- insecure_reneg
- server
- pks (complete match of everything)
- protocol
- cert_chain
- stapling_*
- srp???
- ocsp_*
- ssl_ctx_*???
- strict_sni_vhost_check
- fips
- compression
- session_tickets???
If this works, one could think about introducing some kind of "equivalence
number" to speed up the checking, since in certain HTTP/2 setups there might be
a good percentage of requests requiting this verification.
Cheers,
Stefan
> Am 28.08.2015 um 10:54 schrieb Ruediger Pluem :
>
>
>
> On 08/28/2015 10:35 AM, Stefan Eissing wrote:
>>
>>> Am 28.08.2015 um 10:32 schrieb Ruediger Pluem :
>>> On 08/28/2015 09:32 AM, Stefan Eissing wrote:
> Am 28.08.2015 um 03:37 schrieb Roy T. Fielding :
>> +if (r->connection->keepalives > 0) {
>> +return HTTP_MISDIRECTED_REQUEST;
>> +}
>>return HTTP_BAD_REQUEST;
>>}
>>}
>>
> IIRC, it is applicable to HTTP/1.1 as well. Think misdirected requests
> containing
> an absolute request URI that points to some other server. I don't think
> the conditional
> is needed at all -- just return HTTP_MISDIRECTED_REQUEST.
Thanks for clarifying this.
> Hmm, I wonder how this impacts Google's desire to allow multiple hosts to
> reuse
> the same SPDY connection ... was that dropped for h2?
It wasn't. Our implementation currently just goes the easy way. It needs
to check that server/vhost from request and SNI indeed use the same
certificate and if not, maybe even if altnames/wildcards match. But I am
not sure that is a good idea.
>>>
>>> The issue is a little bit more complex. You need to ensure that the
>>> server/vhost from the request is using the same SSL
>>> parameters as the SNI host like protocols, ciphers, etc. Otherwise you
>>> would need to renegotiate. And as far as I
>>> remember some parameters are not renegotiable. See comments above this code.
>>
>> Hmm, I see. Since you know this more intimate than me: is checking the
>> mod_ssl config of both for equality of certain members the way to solve
>> this? It should either have the individual settings or the merged ones from
>> the base server, right?
>
> Interesting approach. I hope our SSL experts will chime in :-).
> And yes the configs should have the individual settings or the merged ones
> from the base server which could be the
> default values.
>
> Regards
>
> Rüdiger
bytes GmbH
Hafenweg 16, 48155 Münster, Germany
Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
Re: regression in r1698239
Thanks -- back in 1698334 with loops that actually move. On Fri, Aug 28, 2015 at 8:55 AM, Eric Covener wrote: > On Fri, Aug 28, 2015 at 8:54 AM, Stefan Eissing > wrote: >> Change 1698239 brings t/apache/pr17629.t into an endless loop now. I built >> on r1698322 in trunk (OSX + Ubuntu). >> >> I revert the change now. A certain "covener" might want to look into this. >> ;-) >> >> Cheers, > > > Thanks, go ahead and revert. -- Eric Covener cove...@gmail.com
Re: regression in r1698239
On Fri, Aug 28, 2015 at 8:54 AM, Stefan Eissing wrote: > Change 1698239 brings t/apache/pr17629.t into an endless loop now. I built on > r1698322 in trunk (OSX + Ubuntu). > > I revert the change now. A certain "covener" might want to look into this. ;-) > > Cheers, Thanks, go ahead and revert.
regression in r1698239
Change 1698239 brings t/apache/pr17629.t into an endless loop now. I built on r1698322 in trunk (OSX + Ubuntu). I revert the change now. A certain "covener" might want to look into this. ;-) Cheers, Stefan bytes GmbH Hafenweg 16, 48155 Münster, Germany Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
Re: HTTP_MISDIRECTED_REQUEST
On 08/28/2015 10:35 AM, Stefan Eissing wrote:
>
>> Am 28.08.2015 um 10:32 schrieb Ruediger Pluem :
>> On 08/28/2015 09:32 AM, Stefan Eissing wrote:
>>>
Am 28.08.2015 um 03:37 schrieb Roy T. Fielding :
> +if (r->connection->keepalives > 0) {
> +return HTTP_MISDIRECTED_REQUEST;
> +}
> return HTTP_BAD_REQUEST;
> }
> }
>
IIRC, it is applicable to HTTP/1.1 as well. Think misdirected requests
containing
an absolute request URI that points to some other server. I don't think
the conditional
is needed at all -- just return HTTP_MISDIRECTED_REQUEST.
>>>
>>> Thanks for clarifying this.
>>>
Hmm, I wonder how this impacts Google's desire to allow multiple hosts to
reuse
the same SPDY connection ... was that dropped for h2?
>>>
>>> It wasn't. Our implementation currently just goes the easy way. It needs to
>>> check that server/vhost from request and SNI indeed use the same
>>> certificate and if not, maybe even if altnames/wildcards match. But I am
>>> not sure that is a good idea.
>>
>> The issue is a little bit more complex. You need to ensure that the
>> server/vhost from the request is using the same SSL
>> parameters as the SNI host like protocols, ciphers, etc. Otherwise you would
>> need to renegotiate. And as far as I
>> remember some parameters are not renegotiable. See comments above this code.
>
> Hmm, I see. Since you know this more intimate than me: is checking the
> mod_ssl config of both for equality of certain members the way to solve this?
> It should either have the individual settings or the merged ones from the
> base server, right?
Interesting approach. I hope our SSL experts will chime in :-).
And yes the configs should have the individual settings or the merged ones from
the base server which could be the
default values.
Regards
Rüdiger
>
>
>
Re: HTTP_MISDIRECTED_REQUEST
> Am 28.08.2015 um 10:32 schrieb Ruediger Pluem :
> On 08/28/2015 09:32 AM, Stefan Eissing wrote:
>>
>>> Am 28.08.2015 um 03:37 schrieb Roy T. Fielding :
+if (r->connection->keepalives > 0) {
+return HTTP_MISDIRECTED_REQUEST;
+}
return HTTP_BAD_REQUEST;
}
}
>>> IIRC, it is applicable to HTTP/1.1 as well. Think misdirected requests
>>> containing
>>> an absolute request URI that points to some other server. I don't think
>>> the conditional
>>> is needed at all -- just return HTTP_MISDIRECTED_REQUEST.
>>
>> Thanks for clarifying this.
>>
>>> Hmm, I wonder how this impacts Google's desire to allow multiple hosts to
>>> reuse
>>> the same SPDY connection ... was that dropped for h2?
>>
>> It wasn't. Our implementation currently just goes the easy way. It needs to
>> check that server/vhost from request and SNI indeed use the same certificate
>> and if not, maybe even if altnames/wildcards match. But I am not sure that
>> is a good idea.
>
> The issue is a little bit more complex. You need to ensure that the
> server/vhost from the request is using the same SSL
> parameters as the SNI host like protocols, ciphers, etc. Otherwise you would
> need to renegotiate. And as far as I
> remember some parameters are not renegotiable. See comments above this code.
Hmm, I see. Since you know this more intimate than me: is checking the mod_ssl
config of both for equality of certain members the way to solve this? It should
either have the individual settings or the merged ones from the base server,
right?
//Stefan
bytes GmbH
Hafenweg 16, 48155 Münster, Germany
Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
Re: HTTP_MISDIRECTED_REQUEST
On 08/28/2015 09:32 AM, Stefan Eissing wrote:
>
>> Am 28.08.2015 um 03:37 schrieb Roy T. Fielding :
>>> +if (r->connection->keepalives > 0) {
>>> +return HTTP_MISDIRECTED_REQUEST;
>>> +}
>>> return HTTP_BAD_REQUEST;
>>> }
>>> }
>>>
>> IIRC, it is applicable to HTTP/1.1 as well. Think misdirected requests
>> containing
>> an absolute request URI that points to some other server. I don't think the
>> conditional
>> is needed at all -- just return HTTP_MISDIRECTED_REQUEST.
>
> Thanks for clarifying this.
>
>> Hmm, I wonder how this impacts Google's desire to allow multiple hosts to
>> reuse
>> the same SPDY connection ... was that dropped for h2?
>
> It wasn't. Our implementation currently just goes the easy way. It needs to
> check that server/vhost from request and SNI indeed use the same certificate
> and if not, maybe even if altnames/wildcards match. But I am not sure that is
> a good idea.
The issue is a little bit more complex. You need to ensure that the
server/vhost from the request is using the same SSL
parameters as the SNI host like protocols, ciphers, etc. Otherwise you would
need to renegotiate. And as far as I
remember some parameters are not renegotiable. See comments above this code.
Regards
Rüdiger
Re: HTTP_MISDIRECTED_REQUEST
> Am 28.08.2015 um 03:37 schrieb Roy T. Fielding :
>> +if (r->connection->keepalives > 0) {
>> +return HTTP_MISDIRECTED_REQUEST;
>> +}
>> return HTTP_BAD_REQUEST;
>> }
>> }
>>
> IIRC, it is applicable to HTTP/1.1 as well. Think misdirected requests
> containing
> an absolute request URI that points to some other server. I don't think the
> conditional
> is needed at all -- just return HTTP_MISDIRECTED_REQUEST.
Thanks for clarifying this.
> Hmm, I wonder how this impacts Google's desire to allow multiple hosts to
> reuse
> the same SPDY connection ... was that dropped for h2?
It wasn't. Our implementation currently just goes the easy way. It needs to
check that server/vhost from request and SNI indeed use the same certificate
and if not, maybe even if altnames/wildcards match. But I am not sure that is a
good idea.
Maybe I get time today to look into this...
//Stefan
bytes GmbH
Hafenweg 16, 48155 Münster, Germany
Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
