unintended cookie attribute injections

[maltek]

Hi,

we've identified some unintended problematic code wrt to cookies, e.g. in https://github.com/OWASP/Benchmark/blob/b38d197949f775b3c165029bda9dc6bd890265fb/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest01826.java#L37-L42

Going to a URL like https://localhost:8443/benchmark/crypto-02/BenchmarkTest01826;SameSite=None;?BenchmarkTest01826=SafeText injects SameSite=None into the cookie header. Much worse things are not possible since the URL can't contain newlines.

Now Tomcat is nice enough to mitigate this throwing an exception An invalid path [/benchmark/crypto-02/BenchmarkTest01826;foobar] was specified for this cookie. However, as far as I can tell there's no guarantees from the servlet specs that this happens and indeed when I run the benchmark with e.g. JBoss Wildfly, I get back a response with a bad cookie header. So in my opinion Tomcat's great security feature doesn't mean this isn't an application bug static tools should be flagging.

The fix would be to use getContextPath() which comes from configuration unlike attacker-controlled getRequestURI().

---

The call to setDomain() would be similarly problematic, except setting a bad Host header requires so much control over the HTTP user agent that I don't see scenarios where this would be exploitable in practice. So we're taking that as a challenge for our SAST to filter out :)

[davewichers]

Interesting. Is this 'bug/vulnerability' causing your SAST tool to report False Positives or False Negatives, per expected Benchmark results? If so, can you provide a few example test cases where there this occurs? And, given the existing code, do you feel that your tool is correct, and the Benchmark expected results are wrong? And your proposed fix will fix the Benchmark for everyone?

[maltek]

Is this 'bug/vulnerability' causing your SAST tool to report False Positives or False Negatives, per expected Benchmark results?

Yes, per the expected results the reported findings are considered false negatives.

If so, can you provide a few example test cases where there this occurs?

The same code snippet is repeated in some 200+ test cases. I linked to BenchmarkTest01826.java above, besides a few tests here and there it's also in all tests in the ranges 53-118, 942-1014, 1822-1894.

I'm happy to send a PR if it's agreed that this should be changed.

And, given the existing code, do you feel that your tool is correct, and the Benchmark expected results are wrong?

Yes, I feel they are true positives.

And your proposed fix will fix the Benchmark for everyone?

Yes, I'd say so. I don't see how this fix would negatively affect any tools.

[davewichers]

So, to your point, these aren't True Positives when using Tomcat, but they ARE True Positives when running on other containers that don't behave the same as Tomcat, such as WildFly?

[maltek]

So, to your point, these aren't True Positives when using Tomcat, but they ARE True Positives when running on other containers that don't behave the same as Tomcat, such as WildFly?

Yes. Static tools of course have a hard time knowing the container used at runtime.

Philosphically, I'd argue that it's ALWAYS an application bug. With SOME containers (Tomcat) this bug manifests as a no-impact exception / internal server error, and with SOME others (WildFly) it manifests as a vulnerability.