Repository navigation
Http -Parameter Value Length / Validation issue #865
Description
Activity
Okay, maybe I'm just having an off day or am extra tired, or maybe @RobertMolenda just wasn't clear what the 'bug' is here and what we are being asked to do correct the situation.
However, looking at the SecurityWrapperRequest.java file, line 455, I do see an oddity in the getParameterValues method:
String cleanValue = ESAPI.validator().getValidInput("HTTP parameter value: " + value, value, "HTTPParameterValue", sc.getIntProp("HttpUtilities.URILENGTH"), true);
If the regex for value fails against the regex check as given by the Validator.HTTPParameterValue property, if will fail in one way, but if the total length for value exceeds HttpUtilities.URILENGTH, it will fail differently. I guess one could argue that that inconsistency is wrong, but I don't see how it is insecure. Just perhaps a bit confusing.
The problem is that we don't have a separate property for the maximum length of a separate parameter value. We could fix this by adding one and passing that in for the maxlength. So this issue is we're incorrectly comparing the overall URI length with the length of a specific single named parameter value. And if that parameter came from a POST rather than a GET, using HttpUtilities.URILENGTH makes zero sense because POST parameters are not really part of the URI and thus not included in the URI length.
The problem with fixing this and changing it to do something that is more intuitive means potentially (I could say likely) break backward compatibility. And since both of these 2 properties
- HttpUtilities.URILENGTH
- Validator.HTTPParameterValue
and thus can be tweaked as needed, I am somewhat reluctant to change the Validator.HTTPParameterValue regex to match the HttpUtilities.URLLENGTH as the max length of an HTTP parameter value. Alternately, in theory at least, one ought to be able to extend SecurityWrapperRequest and just override that one getParameterValues method.
I guess at this point, I need to here more convincing evidence about why this should be changed. I personally view more as confusing than wrong. And it certainly is not insecure in my mind. (I might consider it wrong if HttpUtilities.URLLENGTH were set to 1000 or less.)
In closing, I'm a bit puzzled about @RobertMolenda's comment of:
note granted - changing the MAXURI setting as well as reconfiguration of the HTTPParameterValue Regex statement is my current work-around, however the IT Security Team isn't that happy having a maximum URL Length that is massive.
I mean, what in the world did you change these to parameters too that your IT Security Team isn't happy with a "massive" URL length. If you look at the StackOverflow reference in the comment just above HttpUtilities.URLLENGTH, it notes that most browsers accept 8000 octets as per the RFC. Plus there are other (and IMHO, better) ways to enforce a maximum URL length. I now that Tomcat allows you to configure that and I expect that other JavaEE / Jakarta servlet engines / application servers allow that as well. So, I'm really not getting their alarm at all.
Anyway, please give me some better rationale as to why this should be fixed. Thus far, unless @xeno6696 or @jeremiahjstacey convince me otherwise, I am leaning towards closing this and marking it as "wontfix".
-kevin
P.S.- It's days like this when I think that ESAPI should have been named TMDP (Too Many Damned Properties). Sigh.I completely agree that there's some uncovered tech debt here..
I have a hard time determining what the security issue could be with default settings. Obviously there's four cases to consider because of there being two settings, but agreed, all I see here are unexpected instances of whichever value is shorter causing a rejection which would cause someone to debug and determine that the value is set in two places after some hunting.
And then yes, I can see there being some head-scratching as it involves the idea that there's a context switch between a GET parameter value and a POST parameter value.
I don't necessarily think that backwards compatibility should be honored if this behavior causes a security issue, as the only way I can see this being a problem is if an ESAPI user alters the defaults. By default the regex has a length and the method call itself requires a length even if you're passing in your own value.
The scarier part is that there aren't as many unit tests for this part of the code, so the chances of unforseen breaks for clients would definitely be something to consider.
If we do decide to do something here, I'll volunteer, but I don't believe this is urgent.
Well - honestly - "Bug" isn't a great indication - as well the code doesn't explode. - nor - is there a potential security issue
With that being said - as xeno6696 pointed out - this is more 'technical debt' that could be addressed in R3 which you guys are working on.
My dog-chasing-squirrel-thinking about this is that the URILENGTH should be utilized to check URL Lengths and not "other things that are not URL related" - and that the HTTPParameter-Length/Value Checks have currently two methods of checking
The "length" setting and/or the length presented in Regex.
That and/or* allows for a configuration to blow hole in foot (been there done that) - in where
Configure length for X and Regex validates for length Y and depending on that - then your foot hurts or not.
So in a perfect "configuration safe world" - the Regex expression would be checked for a 'length configuration' and if so then you can (squirrel mode) - validate against a "length configuration" OR use one or the other... (trust me in saying I write a bunch of validator-rules to ensure the configurator-person doesn't hurt themselves)
Certainly the architecture of the application I inherited "has room for improvement" - and a 15K header-parameter-value isn't optimal in my mind either - but changing architecture is a good few (many ??) releases out...
So to summarize - in looking at "Configuration Variable Names" - one makes assumptions (bad) - in where URILength does just that - and not be utilized for 'other checks' not related to URI
Best
RobertI keep circling back on this one to being a "UI" fail with the user, in this case being the programmer. We should probably put in the javadoc and in the properties under the length regexes that there is a potential competition for length, and that we should call out which one resolves first. (i.e., I think the length check happens before it's passed to the regex.)
I've always thought that the length check in the actual API should default to the regex in question since as a programmer, I'm more likely to maintain that at the regex level than necessarily think about that at the Java code level.
Of course, in a perfect world, we break apart ESAPI into various components, which we've discussed several times in the past.
I agree to @xeno6696's suggestion here that we ought to clarify this in the Javadoc. I think the reason that the some the regexes such as the one @RobertMolenda mentioned (i.e., Validator.HTTPParameterValue) is perhaps because someone thought that the regex might be used directly via Pattern / Matcher instead of one of the Validator.getValidInput methods, all of which take a maxLength parameter (which, as Matt mentioned, I believe it checked first), but that's just speculation on my part. I suppose I could figure out the actual reason if it weren't so late and I actually spent time trying to through the git history of Validator and validation.properties. But at this point, it doesn't much matter how we got here, but what can we do. I don't want to make any code changes (other than maybe some tweaks to log and/or exception messages) and risk breaking someone's code though, so I think Javadoc changes and perhaps adding a wiki page to describe additiona surprises is the best we can offer here. I agree with Robert, that anything more drastic should probably be deferred until ESAPI 3. (Of course, maybe we ought to drop an issue there to remind us of that.) I suspect there's a huge amount of technical debt that we want to review, but little thinks like this is easy to forget.
Describe the bug
Currently utilizing release 2.5.5.0 - looked at 2.6.0.0 and code is the same - so here it comes:
There are two observed issues with http parameter validation regarding "Length" checks
Observation 1: function:
SecurityWrapperRequest.public String[] getParameterValues(String name) {...}
Utilizes the setting:
HttpUtilities.URILENGTH
to validate the length of the value
Recommend adding a new parameter to differentiate "this vs that" so to say
Observation 2: Setting
The default setting for regex on httpParameterValue is:
Validator.HTTPParameterValue=^[-\p{L}\p{N}./+=_ !$*?@]{0,1000}$
This setting limits the value to 1000 bytes
To Reproduce
Create an http parameter > URILENGTH and send through cleansing
Expected behavior
Provide different length-settings between httpParameterValues versus URILENGTH
or
Remove the length check - and allow the regex to control validation
Reasoning:
they need to be validated differently
URL Length - prevent spamming an application
HttpParameter Length - prevent hacking / crashing an application
note granted - changing the MAXURI setting as well as reconfiguration of the HTTPParameterValue Regex statement is my current work-around, however the IT Security Team isn't that happy having a maximum URL Length that is massive.