DefaultHTTPUtility uses hard coded Header name/value lengths

[Criptak]

Generates an error when adding a header via DefaultHTTPUtilities:
org.owasp.esapi.errors.ValidationException: addHeader: Invalid input. The maximum length of 20 characters was exceeded.

esapi-java-legacy/src/main/java/org/owasp/esapi/reference/DefaultHTTPUtilities.java

Line 240 in 19e8341

String safeName = ESAPI.validator().getValidInput("addHeader", strippedName, "HTTPHeaderName", 20, false);

The DefaultHTTPUtility uses hard coded Header name/value lengths rather than reference the MaxHeaderNameSize and MaxHeaderValueSize set in properties. This should either be updated to reference the SecurityConfiguration similar to the SecurityWrapperResponse,

esapi-java-legacy/src/main/java/org/owasp/esapi/filters/SecurityWrapperResponse.java

Line 182 in 19e8341

safeName = ESAPI.validator().getValidInput("addHeader", strippedName, "HTTPHeaderName", sc.getIntProp("HttpUtilities.MaxHeaderNameSize"), false);

, or be refactored to use the SecurityWrapperResponse if there is duplication.

[xeno6696]

I genuinely thought I had found all the ways that was being set. Kinda crazy it took 4yrs for someone to bring up.

[kwwall]

@Criptak - Good catch.
@xeno6696 - My guess it has taken 4 years to discover because we don't have sufficient test cases for (at least) the DefaultHTTPUtilities class. In fact, all I see is src/test/java/org/owasp/esapi/reference/HTTPUtilitiesTest.java and there and nothing that really tests that version of DefaultHTTPUtilities.addHeader() at all. I think if we would have had adequate test cases we would have caught this way earlier, at least when these changes were made. Can I assign this to you, Matt?

[xeno6696]

Yeah, I'll try to take a deeper sweep to see if there's anything else I might have missed. And of course yes, I'll get them under test.

[xeno6696]

FWIW I thought I had done a grep for "addHeader" and so I feel I should have snagged this one.

[kwojcikowski]

Hi,
Could You consider also changing it in DefaultHTTPUtilities#addCookie(HttpServletResponse response, Cookie cookie)?
String cookieName = ESAPI.validator().getValidInput("cookie name", name, "HTTPCookieName", 50, false, errors); String cookieValue = ESAPI.validator().getValidInput("cookie value", value, "HTTPCookieValue", 5000, false, errors);

[xeno6696]

It'll be changed to be user-configurable. I will bump the default value to 1024--somewhat arbitrary but in my experience cookies larger than 500 chars are rare. I question cookie lengths of 5K as you're suggesting. That sounds like something weird is going on. But the limit will be configurable so if you want 5000 you can set it.

[kwwall]

I think there is a maximum length for all the cookies...something around 4K IIRC. So if you set something like 5000 per cookie, you're probably only asking for trouble. I suggest making it pick up the values from ESAPI's properties (Validator.HTTPCookieName, and Validator.HTTPCookieValue). You may also want to change the regex for Validator.HTTPCookieName to bump the max cookie length from 32 to 50.

[xeno6696]

Okay so @Criptak and @kwojcikowski --Just so we're clear, the current codebase says this:

192.        String cookieName = ESAPI.validator().getValidInput("cookie name", name, "HTTPCookieName", 50, false, errors);

193.        String cookieValue = ESAPI.validator().getValidInput("cookie value", value, "HTTPCookieValue", 5000, false, errors);

So, I'm thinking that if you update to use the most recent version of ESAPI, you are already covered.

@kwwall just so you know we've had a default max cookie value of 4096 going back at least 12yrs. (I didn't push further than that.) See line 41 in HTTPUtilities.java.

[kwwall]

I think (at least 12 years ago), 4K was the limitation for most browsers and was taken from RFC 2109. That # may have changed. The latest (draft) RFC for "HTTP State Management" is RFC 6265.

Also, I grepped for MAX_COOKIE_LEN and the only place we use that is when dealing with encrypted cookies in the DefaultHTTPUtilities.encryptStateInCookie(). Was a bit surprised that was the only place that 4K value seems to be reflected.

[xeno6696]

I found the same thing, I wasn't sure if you felt that should be parameterized or left compilation-only for crypto purposes?

[kwwall]

If we start using it more widely, it definitely should be parameterized. We should also look at the RFC to see if the value has increased. It likely has.

[kwojcikowski]

Hi @xeno6696,
I see that You have updated cookies header and value to be parameterized. Will there be any update on HTTPHeaderName length parametrization?

[xeno6696]

@kwojcikowski I'll get those propagated as well, there were a few spots where that got missed. Appreciate the assist.

[xeno6696]

Yeah, the HTTPUtilities class didn't have any unit tests, the only ones that exist now are ones that I built back when I made this initial parameterization adjustment. Clearly I felt rushed. I will generate another issue to ensure that the whole class is properly tested. Much of this had to be mocked, and so it'll also be good to review my old tests to ensure they're actually testing things. I'm always cautious with mocks.

[xeno6696]

I'm gonna take that back. It had some unit tests, using mock classes. (back before Mockito/PowerMock.) The tests feel muddy here. There's functionality getting tested in HTTPUtilitiesTest that should probably be back in the Validator tests. Not sure I want to tackle that as part of this issue however.

[xeno6696]

So mocking the addHeader from the HTTPUtilities test is a bit much in regards to mock setup, but the following unit test covers what we're after.

    @Test
    public void testHeaderLengthChecks(){
    	Validator v = ESAPI.validator();
    	SecurityConfiguration sc = ESAPI.securityConfiguration();
    	assertFalse(v.isValidInput("addHeader", TestUtils.generateStringOfLength(257), "HTTPHeaderName", sc.getIntProp("HttpUtilities.MaxHeaderNameSize"), false));
    	assertFalse(v.isValidInput("addHeader", TestUtils.generateStringOfLength(4097), "HTTPHeaderValue", sc.getIntProp("HttpUtilities.MaxHeaderValueSize"), false));
    }

[kwwall]

This seems to have been fixed via PR #663, which would have been first appeared in the ESAPI 2.3.0.0 release, so I am marking this as closed.

@Criptak - Add a comment to this issue if it is not fixed in ESAPI 2.3.0.0 and later and we'll re-open this.