Ignore illegal response header #2439

kuei0221 · 2020-10-21T11:17:03Z

Description

Continue #2137 and solve bugs find in #2413 .

This PR will add the following feature:

Ignore header whose key contains an illegal character (i.e non-printable ASCII, DQUOTE, (),/:;<=>?@[]{} )
Ignore header whose key is Status or start with rack.
Ignore header whose value contains a character below 037 after split if possible.

Code change:

Add constant to handle illegal character and banned key of header.
Add methods illegal_header_key? and illegal_header_value? to check the header.
Method possible_header_injection? should be replaced with the above new feature. (please double-check this)
Either key or value is illegal, the header pair will be ignored.

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
I have added appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

st0012 · 2020-10-21T11:49:28Z

lib/puma/const.rb

@@ -239,5 +239,11 @@ module Const
    # Mininum interval to checks worker health
    WORKER_CHECK_INTERVAL = 5

+    # Illegal character in the key or value of response header
+    ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{Regexp.escape('"(),/:;<=>?@[]{}')}]/.freeze


maybe Regexp.escape('"(),/:;<=>?@[]{}') can be extracted to improve readibility?

ILLEGAL_HEADER_CHAR_REGEX = Regexp.escape('"(),/:;<=>?@[]{}').freeze ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{ILLEGAL_HEADER_CHAR_REGEX}]/.freeze

I agree, maybe add another constant for it 👍

nateberkopec · 2020-10-21T15:11:25Z

Labeling perf because this is on a very hot path.

nateberkopec · 2020-10-21T16:59:10Z

This branch, hello.sh:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   296.70us   76.12us   1.55ms   75.43%
    Req/Sec     6.75k   457.28     7.05k    93.52%
  Latency Distribution
     50%  283.00us
     75%  329.00us
     90%  395.00us
     99%  527.00us
  404337 requests in 30.10s, 29.31MB read
Requests/sec:  13431.47
Transfer/sec:      0.97MB

This branch, many_long_headers.sh:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     7.19ms   20.56ms 130.39ms   91.12%
    Req/Sec     2.23k   446.54     4.26k    77.54%
  Latency Distribution
     50%  827.00us
     75%    1.02ms
     90%   17.51ms
     99%   99.90ms
  133212 requests in 30.12s, 7.52GB read
Requests/sec:   4423.24

nateberkopec · 2020-10-21T17:01:58Z

Master branch, hello.sh:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   293.30us   74.15us   1.73ms   74.40%
    Req/Sec     6.83k   424.55     7.11k    97.18%
  Latency Distribution
     50%  281.00us
     75%  325.00us
     90%  390.00us
     99%  516.00us
  408821 requests in 30.10s, 29.63MB read
Requests/sec:  13581.95
Transfer/sec:      0.98MB

Master branch, many_long_headers.sh:

Puma starting in single mode...
* Version 5.0.2 (ruby 2.7.2-p137), codename: Spoony Bard
* Min threads: 4, max threads: 4
* Environment: development
* Listening on http://0.0.0.0:9292
Use Ctrl-C to stop
Running 30s test @ http://localhost:9292
  2 threads and 4 connections
connection 0: 36713 requests completed
connection 1: 39794 requests completed
connection 0: 35901 requests completed
connection 1: 36155 requests completed
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     7.71ms   21.51ms 149.83ms   90.49%
    Req/Sec     2.49k   608.78     5.05k    73.50%
  Latency Distribution
     50%  737.00us
     75%    0.92ms
     90%   25.14ms
     99%  100.40ms
  148563 requests in 30.04s, 8.38GB read
Requests/sec:   4946.24
Transfer/sec:    285.76MB

nateberkopec · 2020-10-21T17:02:29Z

Maybe a ~10% reduction in the headers benchmark throughput, but that could be measurement error.

nateberkopec · 2020-10-21T17:02:47Z

I want to come back to this PR again just to a final security/correctness review, but it looks fine to me.

kuei0221 · 2020-10-22T10:40:32Z

Thanks for checking! I have also run the same script on my machine, and I find the reduction is around 20%.

master branch, hello.sh:

❯ benchmarks/wrk/hello.sh

Puma starting in single mode...
* Version 5.0.2 (ruby 2.7.0-p0), codename: Spoony Bard
* Min threads: 4, max threads: 4
* Environment: development
* Listening on http://0.0.0.0:9292
Use Ctrl-C to stop
Running 30s test @ http://localhost:9292
  2 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   364.71us  118.23us   2.77ms   77.83%
    Req/Sec     5.48k   566.17     6.42k    58.97%
  Latency Distribution
     50%  339.00us
     75%  416.00us
     90%  510.00us
     99%  761.00us
  328166 requests in 30.10s, 23.79MB read
Requests/sec:  10902.75
Transfer/sec:    809.19KB
- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2020-10-22 18:25:00 +0800 ===
- Goodbye!

master branch, many_long_headers.sh

❯ benchmarks/wrk/many_long_headers.sh
Puma starting in single mode...
* Version 5.0.2 (ruby 2.7.0-p0), codename: Spoony Bard
* Min threads: 4, max threads: 4
* Environment: development
* Listening on http://0.0.0.0:9292
Use Ctrl-C to stop
Running 30s test @ http://localhost:9292
  2 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.03ms  340.45us   5.48ms   74.34%
    Req/Sec     1.95k   213.74     3.01k    78.87%
  Latency Distribution
     50%    0.97ms
     75%    1.20ms
     90%    1.46ms
     99%    2.07ms
  116664 requests in 30.10s, 6.58GB read
Requests/sec:   3875.83
Transfer/sec:    223.93MB
- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2020-10-22 18:25:40 +0800 ===
- Goodbye!

this branch, hello.sh

❯ benchmarks/wrk/hello.sh
Puma starting in single mode...
* Version 5.0.2 (ruby 2.7.0-p0), codename: Spoony Bard
* Min threads: 4, max threads: 4
* Environment: development
* Listening on http://0.0.0.0:9292
Use Ctrl-C to stop
Running 30s test @ http://localhost:9292
  2 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   441.36us  148.59us   4.87ms   77.43%
    Req/Sec     4.53k   587.44     6.27k    67.83%
  Latency Distribution
     50%  408.00us
     75%  506.00us
     90%  628.00us
     99%    0.93ms
  270271 requests in 30.00s, 19.59MB read
Requests/sec:   9008.98
Transfer/sec:    668.64KB
- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2020-10-22 18:26:34 +0800 ===
- Goodbye!

this branch, many_long_headers.sh:

❯ benchmarks/wrk/many_long_headers.sh
Puma starting in single mode...
* Version 5.0.2 (ruby 2.7.0-p0), codename: Spoony Bard
* Min threads: 4, max threads: 4
* Environment: development
* Listening on http://0.0.0.0:9292
Use Ctrl-C to stop
Running 30s test @ http://localhost:9292
  2 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.38ms  509.31us   6.90ms   75.64%
    Req/Sec     1.46k   234.95     2.01k    69.83%
  Latency Distribution
     50%    1.29ms
     75%    1.61ms
     90%    2.00ms
     99%    3.10ms
  87417 requests in 30.01s, 4.93GB read
Requests/sec:   2913.31
Transfer/sec:    168.31MB
- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2020-10-22 18:27:14 +0800 ===
- Goodbye!

nateberkopec · 2020-10-22T20:05:51Z

OK, I thought about it and I think these changes are worth it.

Technically, I would say Puma is not responsible for making sure that the response object conforms to the RACK spec. Most of the time people should use Rack::Lint to ensure this conformance. However, in these cases re: headers, not conforming to the spec can be abused by malicious actors, as we've seen. I think this change proactively closes possible security holes or mistakes in apps, so I think it's worth it.

I'm going to delay merge until 5.1, because this may break people's (admittedly bad and non-conformant) apps.

nateberkopec · 2020-10-22T20:06:29Z

I would also say this closes #2137.

See puma#2495 and puma#2439

Fix Puma::Const::ILLEGAL_HEADER_KEY_REGEX overlap See puma#2495 and puma#2439 Update test_launcher.rb, use parenthesis with regex parameters

Fix Puma::Const::ILLEGAL_HEADER_KEY_REGEX overlap Add backslash as illegal character for header field-name See puma#2495 and puma#2439 Update test_launcher.rb, use parenthesis with regex parameters

Fix Puma::Const::ILLEGAL_HEADER_KEY_REGEX overlap Add backslash as illegal character for header field-name See #2495 and #2439 Update test_launcher.rb, use parenthesis with regex parameters

st0012 reviewed Oct 21, 2020

View reviewed changes

nateberkopec added bug waiting-for-review Waiting on review from anyone labels Oct 21, 2020

kuei0221 force-pushed the ignore_illegal_header branch from 079acc6 to e26656f Compare October 21, 2020 14:18

Ignore illegal response header

c53d561

kuei0221 force-pushed the ignore_illegal_header branch from e26656f to c53d561 Compare October 21, 2020 14:43

kuei0221 marked this pull request as ready for review October 21, 2020 15:02

nateberkopec added the perf label Oct 21, 2020

nateberkopec added this to the 5.0.3 milestone Oct 21, 2020

nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Oct 22, 2020

nateberkopec removed the waiting-for-changes Waiting on changes from the requestor label Oct 22, 2020

nateberkopec modified the milestones: 5.0.3, 5.1.0 Oct 22, 2020

nateberkopec added 2 commits October 26, 2020 16:00

Merge branch 'master' into ignore_illegal_header

c45db60

Update History.md

f971646

nateberkopec merged commit 31c72cf into puma:master Oct 26, 2020

MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Nov 27, 2020

Puma::Const::ILLEGAL_HEADER_KEY_REGEX overlap

b539869

See puma#2495 and puma#2439

MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Nov 27, 2020

Fix regex warnings in const.rb & test_launcher.rb

eae293b

Fix Puma::Const::ILLEGAL_HEADER_KEY_REGEX overlap See puma#2495 and puma#2439 Update test_launcher.rb, use parenthesis with regex parameters

MSP-Greg mentioned this pull request Nov 28, 2020

Bug: Fix regex warnings in const.rb & test_launcher.rb [changelog skip] #2498

Merged

8 tasks

micke mentioned this pull request Dec 10, 2020

Make illegal header matching more stringent #2510

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore illegal response header #2439

Ignore illegal response header #2439

kuei0221 commented Oct 21, 2020 •

edited

Loading

st0012 Oct 21, 2020

kuei0221 Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

kuei0221 commented Oct 22, 2020 •

edited

Loading

nateberkopec commented Oct 22, 2020

nateberkopec commented Oct 22, 2020

Ignore illegal response header #2439

Ignore illegal response header #2439

Conversation

kuei0221 commented Oct 21, 2020 • edited Loading

Description

Your checklist for this pull request

st0012 Oct 21, 2020

Choose a reason for hiding this comment

kuei0221 Oct 21, 2020

Choose a reason for hiding this comment

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

nateberkopec commented Oct 21, 2020

kuei0221 commented Oct 22, 2020 • edited Loading

nateberkopec commented Oct 22, 2020

nateberkopec commented Oct 22, 2020

kuei0221 commented Oct 21, 2020 •

edited

Loading

kuei0221 commented Oct 22, 2020 •

edited

Loading