Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ParameterLimitValve to enforce request parameter limits for specific URLs #753

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dsoumis
Copy link
Contributor

@dsoumis dsoumis commented Sep 12, 2024

This is an effort of introducing Parameter Limit Valve to allow limiting the number of parameters in HTTP requests, but explicitly allowing more parameters for specific URLs. (The idea raised by this email)

It's worth to be noted that if the Parameter Limit Valve is configured, it operates independently of the Connector's maxParameterCount attribute. The Connector's maxParameterCount sets a global limit, while the Parameter Limit Valve offers additional flexibility by allowing different limits for specific URLs. However, if the maxParameterCount defined in the Connector is lower, it effectively overrides the valve by preventing large requests from ever reaching it.


For manual testing one can add something like the following in context.xml

Valve className="org.apache.catalina.valves.ParameterLimitValve"
           maxGlobalParams="4"
           urlPatternLimits="/api/.*=2,/admin/.*=1,/my/special/url1=3" />

and run some relevant test cases:

curl -X POST http://localhost:8080/api/resource -d "param1=val1&param2=val2" PASS
curl -X POST http://localhost:8080/api/resource -d "param1=val1&param2=val2&param3=val3" FAIL
curl -X POST http://localhost:8080/admin/settings -d "param1=val1" PASS
curl -X POST http://localhost:8080/admin/settings -d "param1=val1&param2=val2" FAIL
curl -X POST http://localhost:8080/my/special/url1 -d "param1=val1&param2=val2&param3=val3" PASS
curl -X POST http://localhost:8080/my/special/url1 -d "param1=val1&param2=val2&param3=val3&param4=val4" FAIL
curl -X POST http://localhost:8080/random -d "param1=val1&param2=val2&param3=val3&param4=val4" PASS
curl -X POST http://localhost:8080/random -d "param1=val1&param2=val2&param3=val3&param4=val4&param5=val5" FAIL

- Introduced ParameterLimitValve to allow limiting the number of parameters in HTTP requests.
- Added configuration options for global parameter limits and specific limits for URL patterns.
- Requests exceeding the configured limits are rejected with an HTTP 400 Bad Request error.
- Includes unit tests (TestParameterLimitValve) to ensure full test coverage for the valve's functionality.
- Added documentation for the ParameterLimitValve, detailing its attributes, usage, and configuration.
@markt-asf
Copy link
Contributor

This approach could be implemented as a Filter. If we were going to do this, I think something that enforces the limit at the point the parameters are parsed - rather than after - is the way to go. It would mean some refactoring of Request.doParseParameters() and I haven't thought it all the way through but it looks doable. Maybe add a maxParameterCount field to the request that is reset from the current connector value on recycle but the Valve could then change it. The added bonus is that if the app doesn't trigger parameter parsing, the parameters won't get parsed.

@ChristopherSchultz
Copy link
Contributor

While I haven't actually looked at the code (for either request.parseParameters or this Valve), I thought the point of maxParameterCount was that the request would actually fail to be fully-parsed and recovery was possible neither via Valve nor Filter. The parameter count limit is there to protect Tomcat from a DoS caused by hash collisions (right?).

Allowing a Valve/Filter to recover from this means that that DoS protection is effectively gone, even if the request is ultimately rejected with a 4xx response. The damage has already been done.

@dsoumis
Copy link
Contributor Author

dsoumis commented Sep 16, 2024

I will refactor the implementation as follows:

  1. The maxParameterCount limit will be enforced directly within Request.doParseParameters(). If the Valve is set and the number of parameters exceeds the configured limit, the request will fail immediately, preventing parameter parsing operations from taking place.
  2. The ParameterLimitValve will only adjust the maxParameterCount on the Request object before parameter parsing occurs. The actual enforcement of the limit will remain in the parsing stage, ensuring that no recovery happens post-parsing, and thus preserving the intended DoS protection.

Please let me know if this sounds like a reasonable solution, or if there’s anything else to consider that I am missing.

@markt-asf
Copy link
Contributor

The parameter count limit is there to protect Tomcat from a DoS caused by hash collisions (right?).

Hash collisions was why the 10k limit was put in place - CVE-2012-0022. That was chosen as the largest round number that was low enough to avoid the hash collision issue.

The further reduction to 1k was on the basis that very few apps need the limit that high and there is a memory cost to handling parameters. The aim was to reduce the minimum amount of RAM Tomcat needed to have and still be able to handle default maximum concurrent requests with default maximum parameters each.

Like all the Tomcat defaults, it is a trade-off.

…t the point when parameters are parsed - rather than after.

- Introduced a new maxParameterCount field in Request.java class
- Removed configuration option for global parameter limit, as it is replaced by the default Connector's limit.
- Replaced unit tests with integration tests to ensure better coverage for the valve's functionality.
- Fixed documentation
@dsoumis
Copy link
Contributor Author

dsoumis commented Sep 27, 2024

I have refactored the valve and the request as discussed here.
There were 2 failing tests after the change to Request's constructor which introduced method accessing of Connector.
The change at TestPersistentManager.java is just a reordering of when the mock Connector object is activated.
Regarding TestSSLValve.java, I introduced a singleton implementation of Mockrequest giving more flexibility for Connector values.

mockRequest.setHeader(valve.getSslCipherUserKeySizeHeader(), "not-an-integer");

try {
valve.invoke(mockRequest, null);
} catch (NumberFormatException e) {
Assert.assertNull(mockRequest.getAttribute(Globals.KEY_SIZE_ATTR));
mockRequest.setHeader(valve.getSslCipherUserKeySizeHeader(), null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the random ordering of the execution of the tests, I had to reset the header at this point because other tests were throwing NumberFormatException.

@dsoumis dsoumis requested a review from markt-asf September 27, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants