unintended cookie attribute injections #139

Activity

1. davewichers commented on Mar 29, 2021

   

   davewichers

   on Mar 29, 2021

   Contributor

   More actions

   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?
2. maltek commented on Mar 29, 2021

   

   maltek

   on Mar 29, 2021

   Author

   More actions

   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.
3. davewichers commented on Mar 29, 2021

   

   davewichers

   on Mar 29, 2021

   Contributor

   More actions

   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?
4. maltek commented on Mar 29, 2021

   

   maltek

   on Mar 29, 2021

   Author

   More actions

   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.
5. TheAuditorTool commented on Apr 13, 2026

   

   TheAuditorTool

   on Apr 13, 2026

   Contributor

   More actions

   @davewichers
   This can be fixed but requires touching over 700 testcode files in javabenchmark... Let me know if you want me to fix it.

   Phase: Investigation of GitHub Issue #139 -- Cookie Attribute Injection via getRequestURI()
   Objective: Verify the reported vulnerability, determine exact scope, and document the fix
   Status: COMPLETE (analysis only -- no files modified per standing orders)

   ---

   1. Verification Phase

   Initial Hypotheses vs Reality

   ┌────────────────────────────────────────────────────────────────────┬──────────────┬─────────────────────────────────────────────────────────────────────────────────────────┐
   │ Hypothesis │ Verification │ Evidence │
   ├────────────────────────────────────────────────────────────────────┼──────────────┼─────────────────────────────────────────────────────────────────────────────────────────┤
   │ getRequestURI() flows to cookie.setPath() creating injection │ CONFIRMED │ BenchmarkTest01826.java:41 -- userCookie.setPath(request.getRequestURI()) │
   ├────────────────────────────────────────────────────────────────────┼──────────────┼─────────────────────────────────────────────────────────────────────────────────────────┤
   │ Issue is limited to ranges 53-118, 942-1014, 1822-1894 as reported │ REFUTED │ Actual scope is 776 occurrences across 724 files, far exceeding the ~200 reported │
   ├────────────────────────────────────────────────────────────────────┼──────────────┼─────────────────────────────────────────────────────────────────────────────────────────┤
   │ Only one code pattern involved │ REFUTED │ 3 distinct patterns found (see below) │
   ├────────────────────────────────────────────────────────────────────┼──────────────┼─────────────────────────────────────────────────────────────────────────────────────────┤
   │ Other uses of getRequestURI() exist outside setPath() │ REFUTED │ Every single getRequestURI() call in testcode is inside a .setPath() -- zero other uses │
   ├────────────────────────────────────────────────────────────────────┼──────────────┼─────────────────────────────────────────────────────────────────────────────────────────┤
   │ Helper/framework files outside testcode are affected │ REFUTED │ grep of src/main/java/org/owasp/benchmark/ excluding testcode/ returned zero matches │
   └────────────────────────────────────────────────────────────────────┴──────────────┴─────────────────────────────────────────────────────────────────────────────────────────┘

   Discrepancies Found

   • The original reporter estimated ~200 test cases. Actual count is 724 files with 776 lines (some files have two occurrences -- one in doGet, one in doPost).

   ---

   2. Root Cause Analysis

   Surface Symptom: URL like /benchmark/crypto-02/BenchmarkTest01826;SameSite=None;?param=val injects SameSite=None into the Set-Cookie response header via path parameter pollution.

   Problem Chain:

   1. request.getRequestURI() returns the raw, attacker-controlled URI including path parameters (;SameSite=None)
   2. This value is passed directly to cookie.setPath() with no sanitization
   3. The cookie path becomes /benchmark/crypto-02/BenchmarkTest01826;SameSite=None
   4. The servlet container serializes this into Set-Cookie: ...;Path=/benchmark/crypto-02/BenchmarkTest01826;SameSite=None
   5. The semicolon-delimited value is parsed by the browser as a separate cookie attribute

   Why This Happened: The original test generator used getRequestURI() as a convenience to scope the cookie to "just this servlet." The intent was correct (narrow cookie path), but the
   method choice is wrong -- getRequestURI() is user-controlled input, getContextPath() is server configuration.

   Container Variance: Tomcat mitigates by throwing IllegalArgumentException on invalid path characters. WildFly and other containers do not. SAST tools correctly cannot assume the runtime
   container.

   ---

   3. Scope Analysis

   3 distinct patterns found across all 724 files:

   Pattern 1: userCookie in doGet (line 41)

   Location: 66 files (ranges ~0053-0118, ~0942-1014, ~1822-1894, and scattered)

   userCookie.setPath(request.getRequestURI());

   Pattern 2: rememberMe in doPost (varies, line ~80-111)

   Location: ~700 files

   rememberMe.setPath(request.getRequestURI()); // i.e., set path to JUST this servlet

   Pattern 3: cookie in doPost (varies, line ~73-96)

   Location: small subset

   cookie.setPath(request.getRequestURI()); // i.e., set path to JUST this servlet

   Proposed Fix (not applied -- test files are read-only)

   Before (all 3 patterns):
   userCookie.setPath(request.getRequestURI());
   rememberMe.setPath(request.getRequestURI());
   cookie.setPath(request.getRequestURI());

   After:
   userCookie.setPath(request.getContextPath());
   rememberMe.setPath(request.getContextPath());
   cookie.setPath(request.getContextPath());

   Why getContextPath(): Returns the server-configured context root (e.g., /benchmark), which is not attacker-controlled. This preserves the intent (scope cookie to this application)
   without the injection vector.

   ---

   4. Edge Cases Considered

   • setDomain(new URL(request.getRequestURL().toString()).getHost()): The original reporter noted this is similarly tainted but requires Host header control, making exploitation
     impractical. Agreed -- not actionable.
   • Expected results impact: Fixing the code eliminates the unintended vulnerability. SAST tools will stop flagging the cookie path injection, resolving the false-positive/true-positive
     classification dispute. The intended test category (crypto, XSS, SQLi, etc.) is unaffected.
   • No behavioral change for legitimate requests: getContextPath() returns /benchmark vs getRequestURI() returning /benchmark/crypto-02/BenchmarkTest01826. Cookie path scope narrows
     slightly, which is irrelevant for benchmark test execution.

   ---

   5. Post-Investigation Audit

   Files modified: None (read-only constraint on test files)

   Verification method: grep -rn across entire src/main/java/org/owasp/benchmark/ tree

   Counts verified:

   • 724 files contain getRequestURI()
   • 776 total line occurrences of .setPath(request.getRequestURI())
   • 0 occurrences of getRequestURI() outside .setPath() calls
   • 0 occurrences outside testcode/ directory

   ---

   6. Impact & Recommendation

   Immediate impact: 776 lines across 724 test files contain an unintended cookie attribute injection vulnerability that creates noise in SAST benchmark scoring.

   Recommended action: Single sed replacement in the BenchmarkJava test generator or across all generated files:
   find src/main/java/org/owasp/benchmark/testcode/ -name '*.java'
   -exec sed -i 's/request.getRequestURI()/request.getContextPath()/g' {} +

   Confidence: High -- the fix is a 1:1 API substitution with identical security semantics for the intended purpose and zero impact on test logic.
6. davewichers commented on Apr 13, 2026

   

   davewichers

   on Apr 13, 2026

   Contributor

   More actions

   @TheAuditorTool - If you can give me an example code fix for each of these situations, I can change the code in the codeblocks used to generate BenchmarkJava and regenerate fixes for all of these. Or, since you have access to the Java codeblocks, maybe you can generate a PR with code fixes for each of the relevant templates and/or codeblocks? Either is fine.
7. TheAuditorTool commented on Apr 13, 2026

   

   TheAuditorTool

   on Apr 13, 2026

   Contributor

   More actions

   @TheAuditorTool - If you can give me an example code fix for each of these situations, I can change the code in the codeblocks used to generate BenchmarkJava and regenerate fixes for all of these. Or, since you have access to the Java codeblocks, maybe you can generate a PR with code fixes for each of the relevant templates and/or codeblocks? Either is fine.

   Please review.
   Repo: BenchmarkCodeBlocks
   Branch: fix/cookie-path-injection-139
   Base: main
   Fixes: OWASP-Benchmark/BenchmarkJava#139
   Files changed: 21 (20 codeblock edits + 1 new regression test)
   Diff: +39 / -39 lines (codeblocks) + 57 lines (new script)

   ---

   Summary

   request.getRequestURI() passed to cookie.setPath() in 20 codeblock templates creates an unintended cookie attribute injection vulnerability. A URL like /benchmark/crypto-02/BenchmarkTest01826;SameSite=None causes SameSite=None to be injected into the Set-Cookie response header via path parameter pollution.

   Tomcat mitigates this by throwing IllegalArgumentException on invalid path characters, but other servlet containers (WildFly, Jetty) do not. SAST tools correctly flag this as a vulnerability, creating false positive noise in benchmark scoring for test cases that are not intended to test cookie path injection.

   Fix: Replace request.getRequestURI() with request.getContextPath() in all 20 affected codeblock files. getContextPath() returns the server-configured context root (/benchmark), which is not attacker-controlled.

   ---

   Root Cause

   The original test generator used getRequestURI() as a convenience to scope cookies to "just this servlet." The intent was correct (narrow cookie path), but the method choice is wrong:

   Method	Returns	Attacker-controlled?	Example
   getRequestURI()	Full request path including path parameters	YES (via ;param=value injection)	/benchmark/xss-01/BenchmarkTest01234;SameSite=None
   getContextPath()	Server-configured context root only	NO (from deployment config)	/benchmark

   The fix was originally proposed by @maltek in the issue and is the exact API substitution recommended.

   ---

   Changes

   Pattern 1: Cookie source setup (1 file)

   sources/RequestGetCookies_SetCookie.code:6 -- Injected into doGet() of all cookie-source test cases via Permute.java:1203-1228 when source filename contains "Cookies".

   Before:

   userCookie.setPath(request.getRequestURI());

   After:

   userCookie.setPath(request.getContextPath());

   Pattern 2: securecookie sinks (2 files)

   sinks/ResponseAddCookie-Nonsecure.code:20 and sinks/ResponseAddCookie-Secure.code:20 -- The actual vulnerability tested is setSecure(false) vs setSecure(true) (CWE-614). The cookie path is just infrastructure.

   Before:

   cookie.setPath(request.getRequestURI()); // i.e., set path to JUST this servlet
                                            // e.g., /benchmark/sql-01/BenchmarkTest01001

   After:

   cookie.setPath(request.getContextPath()); // i.e., set path to this application's context
                                             // e.g., /benchmark

   Pattern 3: weakrand sinks (17 files)

   sinks/MathRandom.code, sinks/Random{NextBytes,NextDouble,NextFloat,NextGaussian,NextInt,NextInt-i,NextLong}.code, sinks/SecureRandom{NextBytes,NextBytes2,NextDouble,NextDouble2,NextFloat,NextGaussian,NextInt,NextInt-i,NextLong}.code -- The actual vulnerability tested is weak vs strong random number generation (CWE-330). The cookie path is just infrastructure for the "remember me" demo.

   Before:

   rememberMe.setPath(request.getRequestURI()); // i.e., set path to JUST this servlet
                                                 // e.g., /benchmark/sql-01/BenchmarkTest01001

   After:

   rememberMe.setPath(request.getContextPath()); // i.e., set path to this application's context
                                                  // e.g., /benchmark

   New: Regression test script

   scripts/validate_no_uri_in_setpath.sh -- Reusable validation script that checks:

   1. No .code file contains setPath(request.getRequestURI())
   2. sources/RequestGetURI.code still exists with getRequestURI() (legitimate taint source not accidentally removed)
   3. All setPath() calls use request.getContextPath()

   ---

   What is NOT changed

   • sources/RequestGetURI.code -- Legitimate taint source definition for SAST testing. Its XML metadata declares <dangerous-source>false</dangerous-source>. Used in Benchmark_v1.3_base.yaml as a source. Changing this would break the benchmark.
   • setDomain() calls (5 files) -- Uses request.getRequestURL().getHost() which requires Host header control to exploit. Original reporter @maltek agreed this is not practically exploitable.
   • Python/dotNet codeblocks -- Grepped: zero matches for any equivalent pattern.
   • Helper/service classes -- No getRequestURI, setPath, or getContextPath outside testcode/.
   • Zero generated test files -- Codeblock-only fix. Regeneration is Dave's responsibility.

   ---

   Behavioral Impact Analysis

   Cookie path scope changes

   Before: cookie scoped to specific servlet path (e.g., /benchmark/xss-01/BenchmarkTest01234)
   After: cookie scoped to application context root (/benchmark)

   Why this doesn't break test execution

   1. Cookie source tests (doGet sets cookie, doPost reads it): Cookies are identified by NAME (BenchmarkTest01234), not by path. Broader path scope means the cookie is sent to more URLs, but the read logic filters by name. No cross-test interference.
   2. securecookie sinks (CWE-614): Vulnerability classification is setSecure(false) vs setSecure(true). Cookie path is irrelevant.
   3. weakrand sinks (CWE-330): Vulnerability classification is Math.random() vs SecureRandom. Cookie path is infrastructure.
   4. expectedresults CSV: Unchanged. Vuln classification is driven by dangerous-source, propagate-danger, and vulnerability flags in XML metadata, none of which reference cookie path.

   Servlet API contract

   • getContextPath() is defined in Servlet 3.1 spec (JSR 340, matching BenchmarkJava's web.xml schema)
   • BenchmarkJava deploys as benchmark.war (pom.xml:887 <finalName>benchmark</finalName>) so getContextPath() returns /benchmark
   • Return value is server-configured, not attacker-controlled

   ---

   YAML Config Verification

   All 19 affected sinks confirmed present in both generation configs:

   • Benchmark_v1.2.yaml (lines 94-155): all 19 sinks listed
   • Benchmark_v1.3_base.yaml (lines 108-168): all 19 sinks listed

   RequestGetCookies.code is listed as a source in both YAMLs, which triggers the RequestGetCookies_SetCookie.code injection via the cookie source indicator path (Permute.java:130, defaultgenconfig.yaml:64).

   ---

   Verification

   Automated test

   bash scripts/validate_no_uri_in_setpath.sh
   # PASS: No setPath(request.getRequestURI()) found in any codeblock
   # PASS: RequestGetURI.code exists and contains getRequestURI()
   # PASS: All setPath() calls use request.getContextPath()
   # ALL CHECKS PASSED

   Counts

   Metric	Before	After
   setPath(request.getRequestURI()) in codeblocks	20	0
   setPath(request.getContextPath()) in codeblocks	0	20
   Total getRequestURI remaining in codeblocks	23	3
   Remaining getRequestURI locations	20 in setPath + 3 in RequestGetURI.{code,xml}	3 in RequestGetURI.{code,xml} only
   Files changed	--	20 modified + 1 new
   Diff balance	--	+39 / -39 (codeblocks)

   Regeneration note

   After merging, regenerate BenchmarkJava with the updated codeblocks. Expected result:

   • 776 occurrences of getRequestURI in generated test files replaced with getContextPath
   • expectedresults-1.2.csv unchanged (deterministic with same randseed: 1984)
   • mvn compile succeeds