AppSec crashes when parsing integer HTTP headers (undefined method `tr' for nil:NilClass (NoMethodError))

[TomNaessens]

Hi all, 👋

We often have bots hit our systems with malformed HTTP headers. One of the malformed HTTP headers they apparently try (a lot), is a plain number (e.g. HTTP_2). This causes the following line to throw an "undefined method `tr' for nil:NilClass (NoMethodError)":

dd-trace-rb/lib/datadog/appsec/contrib/rack/gateway/request.rb

Line 44 in f867bd9

h[k.gsub(/^HTTP_/, '').downcase!.tr('_', '-')] = v if k =~ /^HTTP_/

k =~ /^HTTP_/ is matched, k.gsub(/^HTTP_/, '') results into "2", which becomes nil through downcase!:

[527]dev(main):0> "2".downcase!
=> nil

I'm not exactly sure what the right solution is, given that we're already working with a new object after the gsub, a regular downcase (without !) might work (although that will create another object):

[526]dev(main):0> "2".downcase
=> "2"

To reproduce, spin up a Rails app with dd trace, enable appsec, instrument Rack, and send a request with an integer as HTTP header:

 ~  curl http://localhost:3001 -H "2: 3"
Puma caught this error: undefined method `tr' for nil:NilClass (NoMethodError)

[lloeki]

Good catch! Thanks for the report.

I'm not exactly sure what the right solution is

Probably something like:

h[k.gsub(/^HTTP_/, '').tap(&:downcase!).tr('_', '-')] = v if k =~ /^HTTP_/ 

And to save another allocation:

h[k.gsub(/^HTTP_/, '').tap(&:downcase!).tap { |s| s.tr!('_', '-') }] = v if k =~ /^HTTP_/ 

Note: not using gsub! because we don't want to mutate the original key.

[TomNaessens]

I'm wanting to open a PR and write some tests for this, one last question: what's the expected behaviour when such a header is sent? With the .taps, "malformed" headers not containing text will end up in the hash as nil => value. Is that intended, or should they just not end up in the HeaderCollection at all?

[vpellan]

Hello ! We have pushed a fix that should be included in the next release ! As for the headers that does not contain any text, they should be filtered out by Rack, but if it does happen, we still do want to send them to the WAF.

[TonyCTHsu]

👋 @TomNaessens We've released a 2.3.0, give it a try!

[TomNaessens]

Awesome, thank you!