StrictHttpFirewall: Validate headers and parameters

[candrews]

Improvements to StrictHttpFirewall:

• Headers names values should never contain ISO control characters such as NUL, CR, and LF.
• Similarly, parameter names should never contain ISO control characters either. Parameter values can (imagine a multiline text input - that can and should submit CR/LF in its value)

[candrews]

@jzheaux do you have any feedback on this PR?

I look forward to working with you to (hopefully) getting this merged.

[jzheaux]

Thanks for adding these checks, @candrews, and sorry for the delay - I was just discussing this one internally with a teammate.

I'm a little concerned about the implementation in that it has to check each character of each header name and value and each parameter name. That's a lot of data to churn through, especially considering that the implementation imposes no constraints.

Thinking about Tomcat, a malicious user could send a 2MB request to the server and this class would have to parse through all of it. I ran a quick JMH benchmark on my machine to compare the existing StrictHttpFirewall to the proposed one and found that a malicious payload with the request body and headers maxed out changed the throughput for getFirewalledRequest from ~140000 ops/sec to ~70 ops/sec using the firewall's default settings.

I wonder if changing the API to check a defined set of headers and parameters would be easier to manage.

[candrews]

I ran a quick JMH benchmark on my machine to compare the existing StrictHttpFirewall to the proposed one and found that a malicious payload with the request body and headers maxed out changed the throughput for getFirewalledRequest from ~140000 ops/sec to ~70 ops/sec using the firewall's default settings.

That's definitely not good - can you please provide the JMH benchmark you used so I can run it? I'd like to use it against some other implementation ideas and see how they do.

I wonder if changing the API to check a defined set of headers and parameters would be easier to manage.

We could do that... but I'd prefer not to except as a last resort.

[jzheaux]

Certainly, @candrews!

I've changed the benchmark just now to have a larger max header size, which has changed some of the numbers. Here are my latest results:

Benchmark                                                  (which)   Mode  Cnt        Score        Error  Units
Gh8644StrictHttpFirewallTests.checkingAllChars           largeBody  thrpt   20       95.969 ±      5.164  ops/s
Gh8644StrictHttpFirewallTests.checkingAllChars         largeHeader  thrpt   20    28446.828 ±   1671.452  ops/s
Gh8644StrictHttpFirewallTests.checkingAllChars  largeBodyAndHeader  thrpt   20       65.613 ±     10.537  ops/s
Gh8644StrictHttpFirewallTests.checkingNoChars            largeBody  thrpt   20  3188685.750 ± 175936.646  ops/s
Gh8644StrictHttpFirewallTests.checkingNoChars          largeHeader  thrpt   20    75575.645 ±   3751.235  ops/s
Gh8644StrictHttpFirewallTests.checkingNoChars   largeBodyAndHeader  thrpt   20    77647.231 ±   1306.146  ops/s

https://github.com/jzheaux/spring-security-gh-8644 contains the code.

[candrews]

@jzheaux I believe that the "Reject the NULL character in paths in StrictHttpFirewall" portion isn't impactful to performance, so I split that out into a separate PR: #8703 Hopefully that's non-controversial and can be merged.

I'll continue to work on the headers and parameters changes in this pull request.

[candrews]

I made some modifications to the test to check different approaches, here's my test project: https://github.com/candrews/spring-security-gh-8644

Some results:

Benchmark                                                            (which)   Mode  Cnt        Score        Error  Units
Gh8644StrictHttpFirewallTests.gh8644FirewallUsingAllMatch          largeBody  thrpt   20      315.361 ±     35.599  ops/s
Gh8644StrictHttpFirewallTests.gh8644FirewallUsingCharacterMethods  largeBody  thrpt   20       94.010 ±     58.490  ops/s
Gh8644StrictHttpFirewallTests.gh8644FirewallUsingNoOp              largeBody  thrpt   20  2850933.449 ± 180344.603  ops/s
Gh8644StrictHttpFirewallTests.gh8644FirewallUsingRegex             largeBody  thrpt   20      447.353 ±     30.281  ops/s
Gh8644StrictHttpFirewallTests.gh8644FirewallUsingType              largeBody  thrpt   20      166.019 ±     67.272  ops/s
Gh8644StrictHttpFirewallTests.strictHttpFirewall                   largeBody  thrpt   20  3366402.238 ±  30657.744  ops/s

• gh8644FirewallUsingAllMatch iterates over all the characters, but returns true always. It's just there for comparison.
• gh8644FirewallUsingNoOp checks the parameters names and values and header names but always return true. It's just there for comparison.
• strictHttpFirewall is the current Spring Security implementation.
• gh8644FirewallUsingRegex, gh8644FirewallUsingCharacterMethods , and gh8644FirewallUsingType are potential approaches.

Analysis:

• strictHttpFirewall is the fastest. That makes sense because it doesn't even look at the parameter or header names or values at all.
• gh8644FirewallUsingNoOp is next fastest. This shows that there is a 15% cost to just having a predicate that can do checking.
• gh8644FirewallUsingRegex is the next fastest. Interestingly, it's faster than gh8644FirewallUsingAllMatch which is basically a fancy no-op. This test result shows how fast and well optimized java regex engine really is.

Therefore, I think the regex approach is the way to go, and I've updated this PR accordingly.

I agree that these numbers do look shocking, but I think it's important to keep in mind that this test is a microbenchmark. Any real world use case would not show a performance loss of close to this magnitude, as tasks such as processing data and do real work would dwarf the small amount of time added by this change. I believe this change does add value by making the firewall reject clearly invalid input that could cause (security) problems downstream, and that value outweighs its (in reality) small performance cost.

[jzheaux]

Thanks for this deeper investigation, @candrews.

I agree that these numbers do look shocking, but I think it's important to keep in mind that this test is a microbenchmark.

Agreed.

Any real world use case would not show a performance loss of close to this magnitude, as tasks such as processing data and do real work would dwarf the small amount of time added by this change ... reject invalid input that could cause (security) problems ...

I think more testing is necessary to determine the impact a change like this will have on a whole application. While we want to be secure, it would be much less secure if the firewall were so slow that applications remove it altogether in order to overcome a performance bottleneck.

In the meantime, an application can still achieve what you are advocating by creating a custom firewall class:

public class CodepointFirewall implements HttpFirewall {
    StrictHttpFirewall firewall = new StrictHttpFirewall();

    @Override
    public FirewalledRequest getFirewalledRequest(HttpServletRequest request) {
        // do codepoint checks
        return this.firewall.getFirewalledRequest(request);
    }
}

This shows that there is a 15% cost to just having a predicate that can do checking.

I wonder if inspecting the header and body are a different enough thing from inspecting the request line that they belong in a separate class.

Or perhaps there's a way to change the contract to Predicate<Iterable<String>>, thus making the iteration optional as well. I made an attempt at this. One reason I like it is that it makes it easier for an application reject a request if it has more than a certain number of headers or parameters.

[jzheaux]

@candrews I was chatting with Rob this morning about this ticket, and he shared an idea that might resolve the performance concern, which would be to defer these checks until the value is read by the application.

For example, the instance of FirewalledRequest could override getHeader, etc. to perform the predicate check. The nice thing about this is that the firewall is not arbitrarily iterating through the entire request, but is instead focused on the headers and parameters that the given request is actually reading.

What would you think of changing the PR to check headers and parameters lazily?

[candrews]

What would you think of changing the PR to check headers and parameters lazily?

That sounds like a good idea - I'll put doing so on my TODO list. Thank you for the feedback!

[candrews]

What would you think of changing the PR to check headers and parameters lazily?

I've done that. Parameter and header names and values are now lazily checked in FirewalledRequest. I'm eager to receive your feedback :)

Thanks again!

[jzheaux]

Thanks, @candrews, I think this looks a lot more doable. I've left some feedback inline.

[jzheaux]

Thanks for the updates, @candrews! I've left some additional feedback inline.

[jzheaux]

Awesome, @candrews, I'm glad we were able to find a path that satisfied both of us.

In preparation for merging, will you please squash your commits and format the final commit message?

[candrews]

I'm glad we were able to find a path that satisfied both of us.

I am as well! It's always a pleasure working the Spring developers. Thank you for patience in this process and for your continued efforts.

In preparation for merging, will you please squash your commits and format the final commit message?

If I had known that I'd be squashing to 1 commit, I wouldn't have carefully maintained 3 separate commits for each functional change 🤷 oh well - now I know better for next time!

I've updated this PR with the squashed commit and compliant commit message.

[jzheaux]

Ouch, @candrews! Sorry about that. :/ I'll try and communicate about commits better in the future.

[jzheaux]

@candrews, thank you again! This is now merged into master.