Re: FYI brotli

2017-01-16 Thread Jacob Champion

On 01/16/2017 04:42 PM, Jacob Champion wrote:

Current guidance to avoid BREACH is still, AFAIK, to avoid situations
where third-party data is being sent in the same response as first-party
secrets. I don't think we have a way to know when this is happening


...though if the current response is coming from a static file on disk, 
that's probably a decent heuristic for the vast majority of users out 
there...


--Jacob


Re: FYI brotli

2017-01-16 Thread Jacob Champion

On 01/16/2017 04:06 PM, William A Rowe Jr wrote:

Before we push this at users.. is there a concern that brotoli
compression has similar dictionary or simply size based vulnerabilities
as deflate?


If you mean HTTP compression oracles (BREACH et al), then I would expect 
*any* compression technique to be vulnerable, brotli included.



If so, maybe we teach both to step out of the way when SSL encryption
filters are in place?


Current guidance to avoid BREACH is still, AFAIK, to avoid situations 
where third-party data is being sent in the same response as first-party 
secrets. I don't think we have a way to know when this is happening; 
it's up to the server admin to disable compression in dangerous 
situations. (If anyone knows of an update to this, please jump in.)


Disabling compression whenever TLS is enabled (assuming I've understood 
your suggestion correctly) is IMO too broad a stroke, and will 
de-optimize sites that have been correctly designed to avoid compression 
oracle attacks.


--Jacob


Re: FYI brotli

2017-01-16 Thread William A Rowe Jr
Before we push this at users.. is there a concern that brotoli compression
has similar dictionary or simply size based vulnerabilities as deflate?

If so, maybe we teach both to step out of the way when SSL encryption
filters are in place?

On Jan 16, 2017 10:14, "Jim Jagielski"  wrote:

> Just a head's up that I am working on the backport proposal/patch
> for brotli for 2.4.x...
>


Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

2017-01-16 Thread Daniel Ruggeri
For the most part, yes except the portions that make the header presence
optional (the HDR_MISSING case). Those were added as it came into the
code base to handle a use case I was working on. I've added some
comments inline since I won't have time to poke around myself for a
while yet.


For convenience, here's a link to the original code

https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c


On 1/16/2017 10:14 AM, Jim Jagielski wrote:
> Let me take a look... afaict, this is a copy of what
> was donated, which has been working for numerous people.
> But that doesn't mean it can't have bugs ;)
>
>> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem  wrote:
>>
>> Anyone?

Apologies for missing the original reply

>> Regards
>>
>> Rüdiger
>>
>> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>>>
>>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
 Author: druggeri
 Date: Fri Dec 30 14:20:48 2016
 New Revision: 1776575

 URL: http://svn.apache.org/viewvc?rev=1776575&view=rev
 Log:
 Merge new PROXY protocol code into mod_remoteip

 Modified:
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
httpd/httpd/trunk/modules/metadata/mod_remoteip.c

 ==
 --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
 +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
 2016
 @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
 return OK;
 }

 +static int remoteip_is_server_port(apr_port_t port)
 +{
 +ap_listen_rec *lr;
 +
 +for (lr = ap_listeners; lr; lr = lr->next) {
 +if (lr->bind_addr && lr->bind_addr->port == port) {
 +return 1;
 +}
 +}
 +
 +return 0;
 +}
 +
 +/*
 + * Human readable format:
 + * PROXY {TCP4|TCP6|UNKNOWN}   
  
 + */
 +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
 +  
 remoteip_conn_config_t *conn_conf,
 +  proxy_header 
 *hdr, apr_size_t len,
 +  apr_size_t 
 *hdr_len)
 +{
 +char *end, *word, *host, *valid_addr_chars, *saveptr;
 +char buf[sizeof(hdr->v1.line)];
 +apr_port_t port;
 +apr_status_t ret;
 +apr_int32_t family;
 +
 +#define GET_NEXT_WORD(field) \
 +word = apr_strtok(NULL, " ", &saveptr); \
 +if (!word) { \
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
 +  "RemoteIPProxyProtocol: no " field " found in 
 header '%s'", \
 +  hdr->v1.line); \
 +return HDR_ERROR; \
 +}
 +
 +end = memchr(hdr->v1.line, '\r', len - 1);
 +if (!end || end[1] != '\n') {
 +return HDR_NEED_MORE; /* partial or invalid header */
 +}
 +
 +*end = '\0';
 +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
 +
 +/* parse in separate buffer so have the original for error messages */
 +strcpy(buf, hdr->v1.line);
 +
 +apr_strtok(buf, " ", &saveptr);
 +
 +/* parse family */
 +GET_NEXT_WORD("family")
 +if (strcmp(word, "UNKNOWN") == 0) {
 +conn_conf->client_addr = c->client_addr;
 +conn_conf->client_ip = c->client_ip;
 +return HDR_DONE;
 +}
 +else if (strcmp(word, "TCP4") == 0) {
 +family = APR_INET;
 +valid_addr_chars = "0123456789.";
 +}
 +else if (strcmp(word, "TCP6") == 0) {
 +#if APR_HAVE_IPV6
 +family = APR_INET6;
 +valid_addr_chars = "0123456789abcdefABCDEF:";
 +#else
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
 +  "RemoteIPProxyProtocol: Unable to parse v6 address 
 - APR is not compiled with IPv6 support",
 +  word, hdr->v1.line);
 +return HDR_ERROR;
 +#endif
 +}
 +else {
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
 +  "RemoteIPProxyProtocol: unknown family '%s' in 
 header '%s'",
 +  word, hdr->v1.line);
 +return HDR_ERROR;
 +}
 +
 +/* parse client-addr */
 +GET_NEXT_WORD("client-address")
 +
 +if (strspn(word, valid_addr_chars) != strlen(word)) {
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
 +  "RemoteIPProxyProtocol: invalid client-address '%s' 
 found in "
 +  "header '%s'

Re: FYI brotli

2017-01-16 Thread Evgeny Kotkov
Jim Jagielski  writes:

> Functional patch avail... working on doccos.
>
> http://home.apache.org/~jim/patches/brotli-2.4.patch

Hi Jim,

Thank you for the backport patch.

There is, however, a potential problem with backporting mod_brotli, since
it relies on the Brotli library 1.0.0, which has not yet been released.
In other words, if the upstream changes the API or the library layout
or their pkg-config files after mod_brotli is shipped with httpd 2.4.x, it's
either going to stop building or working.

The open ticket about the new release is here:

  https://github.com/google/brotli/issues/483

My impression on this is that mod_brotli is only safe to backport after the
Brotli authors publish the 1.0.0 version of their library.  But perhaps I am
missing something?

(Apart from this, I think that Brotli did change the layout of their pkg-config
files in [https://github.com/google/brotli/commit/fe9f9a91], and it requires
an update in the filters/config.m4 file; I'll do that.)


Regards,
Evgeny Kotkov


Re: FCGI_ABORT behavior in mod-proxy-fcgi

2017-01-16 Thread Jacob Champion

On 01/11/2017 10:37 AM, Luca Toscano wrote:

I still haven't found any good/clear motivation to send the FCGI_ABORT
record (just before dropping the connection), but I am probably missing
some good point or my assumptions could be wrong. Any comment or
suggestion would be really welcome :)


My $0.02, copied from IRC for discussion on-list:

I don't think we should implement FCGI_ABORT until/unless we decide to 
implement FCGI multiplexing. I understand that the bug's OP is operating 
happily with an FCGI_ABORT patch, but until they respond to you to 
clarify what backend they're using, we don't know if their approach is 
correct even for their own use case.


I don't think we should send FCGI_ABORT and then immediately close the 
connection without waiting for the client to respond. In my mind, 
sending FCGI_ABORT is a contract: "we will accept and discard a 
reasonable number of messages for the current request ID while we wait 
for you to send FCGI_END_REQUEST".


I do think that closing the FCGI connection when the client closes 
theirs is valuable. Eric's suggestion on #httpd to hide this behind a 
directive is probably wise; we've broken enough FCGI backends recently...


--Jacob


Re: FYI brotli

2017-01-16 Thread Jim Jagielski
Functional patch avail... working on doccos.

http://home.apache.org/~jim/patches/brotli-2.4.patch

> On Jan 16, 2017, at 11:11 AM, Jim Jagielski  wrote:
> 
> Just a head's up that I am working on the backport proposal/patch
> for brotli for 2.4.x...



Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

2017-01-16 Thread Jim Jagielski
Let me take a look... afaict, this is a copy of what
was donated, which has been working for numerous people.
But that doesn't mean it can't have bugs ;)

> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem  wrote:
> 
> Anyone?
> 
> Regards
> 
> Rüdiger
> 
> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>> 
>> 
>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
>>> Author: druggeri
>>> Date: Fri Dec 30 14:20:48 2016
>>> New Revision: 1776575
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1776575&view=rev
>>> Log:
>>> Merge new PROXY protocol code into mod_remoteip
>>> 
>>> Modified:
>>>httpd/httpd/trunk/docs/log-message-tags/next-number
>>>httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>>>httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>> 
>> 
>>> ==
>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
>>> 2016
>> 
>>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>>> return OK;
>>> }
>>> 
>>> +static int remoteip_is_server_port(apr_port_t port)
>>> +{
>>> +ap_listen_rec *lr;
>>> +
>>> +for (lr = ap_listeners; lr; lr = lr->next) {
>>> +if (lr->bind_addr && lr->bind_addr->port == port) {
>>> +return 1;
>>> +}
>>> +}
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +/*
>>> + * Human readable format:
>>> + * PROXY {TCP4|TCP6|UNKNOWN}
>>> 
>>> + */
>>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>>> +  
>>> remoteip_conn_config_t *conn_conf,
>>> +  proxy_header 
>>> *hdr, apr_size_t len,
>>> +  apr_size_t 
>>> *hdr_len)
>>> +{
>>> +char *end, *word, *host, *valid_addr_chars, *saveptr;
>>> +char buf[sizeof(hdr->v1.line)];
>>> +apr_port_t port;
>>> +apr_status_t ret;
>>> +apr_int32_t family;
>>> +
>>> +#define GET_NEXT_WORD(field) \
>>> +word = apr_strtok(NULL, " ", &saveptr); \
>>> +if (!word) { \
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>>> +  "RemoteIPProxyProtocol: no " field " found in header 
>>> '%s'", \
>>> +  hdr->v1.line); \
>>> +return HDR_ERROR; \
>>> +}
>>> +
>>> +end = memchr(hdr->v1.line, '\r', len - 1);
>>> +if (!end || end[1] != '\n') {
>>> +return HDR_NEED_MORE; /* partial or invalid header */
>>> +}
>>> +
>>> +*end = '\0';
>>> +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>>> +
>>> +/* parse in separate buffer so have the original for error messages */
>>> +strcpy(buf, hdr->v1.line);
>>> +
>>> +apr_strtok(buf, " ", &saveptr);
>>> +
>>> +/* parse family */
>>> +GET_NEXT_WORD("family")
>>> +if (strcmp(word, "UNKNOWN") == 0) {
>>> +conn_conf->client_addr = c->client_addr;
>>> +conn_conf->client_ip = c->client_ip;
>>> +return HDR_DONE;
>>> +}
>>> +else if (strcmp(word, "TCP4") == 0) {
>>> +family = APR_INET;
>>> +valid_addr_chars = "0123456789.";
>>> +}
>>> +else if (strcmp(word, "TCP6") == 0) {
>>> +#if APR_HAVE_IPV6
>>> +family = APR_INET6;
>>> +valid_addr_chars = "0123456789abcdefABCDEF:";
>>> +#else
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>>> +  "RemoteIPProxyProtocol: Unable to parse v6 address - 
>>> APR is not compiled with IPv6 support",
>>> +  word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +#endif
>>> +}
>>> +else {
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>>> +  "RemoteIPProxyProtocol: unknown family '%s' in 
>>> header '%s'",
>>> +  word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +}
>>> +
>>> +/* parse client-addr */
>>> +GET_NEXT_WORD("client-address")
>>> +
>>> +if (strspn(word, valid_addr_chars) != strlen(word)) {
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>>> +  "RemoteIPProxyProtocol: invalid client-address '%s' 
>>> found in "
>>> +  "header '%s'", word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +}
>>> +
>>> +host = word;
>>> +
>>> +/* parse dest-addr */
>>> +GET_NEXT_WORD("destination-address")
>>> +
>>> +/* parse client-port */
>>> +GET_NEXT_WORD("client-port")
>>> +if (sscanf(word, "%hu", &port) != 1) {
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>>> +  "RemoteIPProxyProtocol: error parsing port '%s' in 
>>> header '%s'",
>>> +  word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +}
>>> +
>>> +/* parse dest-port */
>>> +/* GET_NEXT_WORD(

FYI brotli

2017-01-16 Thread Jim Jagielski
Just a head's up that I am working on the backport proposal/patch
for brotli for 2.4.x...


Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

2017-01-16 Thread Ruediger Pluem
Anyone?

Regards

Rüdiger

On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
> 
> 
> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
>> Author: druggeri
>> Date: Fri Dec 30 14:20:48 2016
>> New Revision: 1776575
>>
>> URL: http://svn.apache.org/viewvc?rev=1776575&view=rev
>> Log:
>> Merge new PROXY protocol code into mod_remoteip
>>
>> Modified:
>> httpd/httpd/trunk/docs/log-message-tags/next-number
>> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>
> 
>> ==
>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
>> 2016
> 
>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>>  return OK;
>>  }
>>  
>> +static int remoteip_is_server_port(apr_port_t port)
>> +{
>> +ap_listen_rec *lr;
>> +
>> +for (lr = ap_listeners; lr; lr = lr->next) {
>> +if (lr->bind_addr && lr->bind_addr->port == port) {
>> +return 1;
>> +}
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/*
>> + * Human readable format:
>> + * PROXY {TCP4|TCP6|UNKNOWN}
>> 
>> + */
>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>> +  
>> remoteip_conn_config_t *conn_conf,
>> +  proxy_header 
>> *hdr, apr_size_t len,
>> +  apr_size_t 
>> *hdr_len)
>> +{
>> +char *end, *word, *host, *valid_addr_chars, *saveptr;
>> +char buf[sizeof(hdr->v1.line)];
>> +apr_port_t port;
>> +apr_status_t ret;
>> +apr_int32_t family;
>> +
>> +#define GET_NEXT_WORD(field) \
>> +word = apr_strtok(NULL, " ", &saveptr); \
>> +if (!word) { \
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>> +  "RemoteIPProxyProtocol: no " field " found in header 
>> '%s'", \
>> +  hdr->v1.line); \
>> +return HDR_ERROR; \
>> +}
>> +
>> +end = memchr(hdr->v1.line, '\r', len - 1);
>> +if (!end || end[1] != '\n') {
>> +return HDR_NEED_MORE; /* partial or invalid header */
>> +}
>> +
>> +*end = '\0';
>> +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>> +
>> +/* parse in separate buffer so have the original for error messages */
>> +strcpy(buf, hdr->v1.line);
>> +
>> +apr_strtok(buf, " ", &saveptr);
>> +
>> +/* parse family */
>> +GET_NEXT_WORD("family")
>> +if (strcmp(word, "UNKNOWN") == 0) {
>> +conn_conf->client_addr = c->client_addr;
>> +conn_conf->client_ip = c->client_ip;
>> +return HDR_DONE;
>> +}
>> +else if (strcmp(word, "TCP4") == 0) {
>> +family = APR_INET;
>> +valid_addr_chars = "0123456789.";
>> +}
>> +else if (strcmp(word, "TCP6") == 0) {
>> +#if APR_HAVE_IPV6
>> +family = APR_INET6;
>> +valid_addr_chars = "0123456789abcdefABCDEF:";
>> +#else
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>> +  "RemoteIPProxyProtocol: Unable to parse v6 address - 
>> APR is not compiled with IPv6 support",
>> +  word, hdr->v1.line);
>> +return HDR_ERROR;
>> +#endif
>> +}
>> +else {
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>> +  "RemoteIPProxyProtocol: unknown family '%s' in header 
>> '%s'",
>> +  word, hdr->v1.line);
>> +return HDR_ERROR;
>> +}
>> +
>> +/* parse client-addr */
>> +GET_NEXT_WORD("client-address")
>> +
>> +if (strspn(word, valid_addr_chars) != strlen(word)) {
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>> +  "RemoteIPProxyProtocol: invalid client-address '%s' 
>> found in "
>> +  "header '%s'", word, hdr->v1.line);
>> +return HDR_ERROR;
>> +}
>> +
>> +host = word;
>> +
>> +/* parse dest-addr */
>> +GET_NEXT_WORD("destination-address")
>> +
>> +/* parse client-port */
>> +GET_NEXT_WORD("client-port")
>> +if (sscanf(word, "%hu", &port) != 1) {
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>> +  "RemoteIPProxyProtocol: error parsing port '%s' in 
>> header '%s'",
>> +  word, hdr->v1.line);
>> +return HDR_ERROR;
>> +}
>> +
>> +/* parse dest-port */
>> +/* GET_NEXT_WORD("destination-port") - no-op since we don't care about 
>> it */
>> +
>> +/* create a socketaddr from the info */
>> +ret = apr_sockaddr_info_get(&conn_conf->client_addr, host, family, 
>> port, 0,
>> +c->pool);
>> +if (ret != APR_SUCCESS) {
>> +conn_conf->client_addr = NULL;
>> +ap_log_cerror(APLOG_MAR

Re: RemoteIPProxyProtocol

2017-01-16 Thread Graham Leggett
On 15 Jan 2017, at 18:35, Daniel Ruggeri  wrote:

>> As we *sure* we want to call it RemoteIPProxyProtocol instead
>> of just "regular" ProxyProtocol ?
>> 
>> The latter just sounds and looks "more right" to me.
> 
> I still like RemoteIPProxyProtocol because I also like the idea of
> "namespacing" modules' directives.

+1.

Anything starting with Proxy* should be in a module called mod_proxy*, 
otherwise confusion will ensue.

Regards,
Graham
--