Skip to content
This repository was archived by the owner on May 14, 2020. It is now read-only.

Conversation

@airween
Copy link
Contributor

@airween airween commented Feb 20, 2020

Fixes #1693.

@airween airween requested review from dune73 and fgsch February 20, 2020 10:31
SecRule &REQUEST_HEADERS:Transfer-Encoding "@eq 0" \
"setvar:'tx.anomaly_score_pl1=+%{tx.notice_anomaly_score}'"

SecRule REQUEST_METHOD "@streq POST" "chain"
Copy link
Contributor

@fgsch fgsch Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with doing this change now but I'd prefer if changes unrelated to what this PR is fixing are done (and documented) separately in case people wants to understand why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right - but I think that the replacing of @rx ^POST$ by @streq POST seems "just" a code cosmetics :).

Let's see other opinions.

@fgsch
Copy link
Contributor

fgsch commented Feb 26, 2020

I've been thinking about this some more and it only covers HTTP versions < 2.

I wonder if we should have another rule specific for HTTP/2 that only checks the Content-Length but it's not clear to me yet whether HTTP/2 allows to send data without the Content-Length header.

@fgsch
Copy link
Contributor

fgsch commented Feb 26, 2020

Minor tweaks aside this looks good to me.

@mirkodziadzka-avi
Copy link

I wonder if we should have another rule specific for HTTP/2 that only checks the Content-Length but it's not clear to me yet whether HTTP/2 allows to send data without the Content-Length header.

The curl request from #1693 creates such an HTTP/2 request without Content-Length header.

@fgsch
Copy link
Contributor

fgsch commented Feb 26, 2020

The curl request from #1693 creates such an HTTP/2 request without Content-Length header.

Can you share the output from that curl instance because in my tests C-L is present?

@mirkodziadzka-avi
Copy link

mirkodziadzka-avi commented Feb 26, 2020

The curl request from #1693 creates such an HTTP/2 request without Content-Length header.

Can you share the output from that curl instance because in my tests C-L is present?

$ curl -kvvv  --http2 -H "Accept: text/html" -H "Content-Type: application/json" -H "Transfer-Encoding: chunked" -d '{}' https://10.151.32.67/uploader.php
*   Trying 10.151.32.67...
* TCP_NODELAY set
* Connected to 10.151.32.67 (10.151.32.67) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-ECDSA-AES128-GCM-SHA256
* ALPN, server accepted to use h2
* Server certificate:
*  subject: CN=System Default EC Cert
*  start date: Feb 11 18:22:38 2020 GMT
*  expire date: Feb  8 18:22:38 2030 GMT
*  issuer: CN=System Default EC Cert
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x140fca0)
> POST /uploader.php HTTP/2
> Host: 10.151.32.67
> User-Agent: curl/7.58.0
> Accept: text/html
> Content-Type: application/json
> 
* Connection state changed (MAX_CONCURRENT_STREAMS updated)!
* We are completely uploaded and fine
< HTTP/2 200 
< content-type: text/html
< server: nginx/1.4.6 (Ubuntu)
< date: Wed, 26 Feb 2020 12:08:18 GMT
< x-powered-by: PHP/5.5.9-1ubuntu4.29
< 
* Connection #0 to host 10.151.32.67 left intact
Request Body of length 2 is uploaded at uploads/noname
$  curl -V
curl 7.58.0 (x86_64-pc-linux-gnu) libcurl/7.58.0 OpenSSL/1.1.0h zlib/1.2.8 nghttp2/1.31.1
Release-Date: 2018-01-24
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy 

@fgsch
Copy link
Contributor

fgsch commented Feb 26, 2020

Thanks. This seems to be because the T-E header is present. Removing it shows that curl sends a C-L header. I suspect what's happening is that because T-E is present, C-L is not added, and in turn T-E is removed because this is HTTP/2.

For HTTP/2 purposes, C-L is probably redundant but added there for compatibility/readability.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Protocol enforcement 920180 does have problems with HTTP/2

3 participants