Add Cache Poisoning Vulnerability#534
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #534 +/- ##
============================================
+ Coverage 52.01% 53.80% +1.78%
- Complexity 465 504 +39
============================================
Files 69 71 +2
Lines 2676 2801 +125
Branches 285 293 +8
============================================
+ Hits 1392 1507 +115
- Misses 1153 1159 +6
- Partials 131 135 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luks-santos
changed the title
…ature/cache-poisoning # Conflicts: # src/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.java # src/main/resources/i18n/messages.properties
luks-santos
changed the title
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
@luks-santos I must say this is amazing code change. Very thorough PR. I really liked the PR completeness. I have added small comments, can you please address?
|
@preetkaran20 Thanks a lot for the feedback! |
- Level 4 cached response now embeds derived email and last-login IP - Rewrite L3, L4 and L5 payload hints around the lab UI flow - Auto-clear attacker input after poison click (per-level)
…ature/cache-poisoning # Conflicts: # src/main/resources/i18n/messages.properties
|
I've pushed the changes addressing the feedback. Summary of what was implemented: Frontend / UX
Vulnerability depth
The PR description has been updated to reflect the final scope. Let me know if there's anything else you'd like me to adjust! |
| } | ||
|
|
||
| function initLevel() { | ||
| const level = vulnerabilityLevelSelected || "LEVEL_1"; |
There was a problem hiding this comment.
This is breaking in Modern UI. Instead of using this, use below function to get the selectedLevel:
getCurrentVulnerabilityLevel(), this is present in both legacy and modern ui.
In order to test with modern ui use: https://github.com/SasanLabs/VulnerableApp#testing-with-modern-ui
| }); | ||
|
|
||
| // Configure Signals | ||
| if (config.signals.includes("cacheControl")) |
There was a problem hiding this comment.
I feel like code became very complex after merging templates. It becomes very heavy on JS side. I think we should have different templates or group of templates for each level. Sorry, I felt easier initially but now it feels little difficult.
| if (xmlHttpRequest.status == 200 || xmlHttpRequest.status == 401) { | ||
| if (isJson) { | ||
| callBack(JSON.parse(xmlHttpRequest.responseText)); | ||
| callBack(JSON.parse(xmlHttpRequest.responseText), xmlHttpRequest); |
There was a problem hiding this comment.
Thank you for this change. Really appreciate this.
| CACHE_POISONING_LEVEL_1_XSS=Cross-Site Scripting (XSS) via Cache Poisoning. Since the banner parameter is reflected without sanitization and the cache only keys on the route, an attacker can store a malicious script in the cache that will execute for all subsequent users. | ||
| CACHE_POISONING_LEVEL_2_NAIVE_FILTER=This level attempts to fix the issue by stripping script tags from the input, but the cache key still only uses the route. Attackers can still poison the cache with misleading content or other HTML payloads that bypass the naive filter. | ||
| CACHE_POISONING_LEVEL_2_XSS=XSS via Cache Poisoning with Filter Bypass. The application strips obvious <script> tags, but does not sanitize other HTML attributes. Attackers can use event handlers like 'onerror' in tags like <img> to execute scripts and poison the cache. | ||
| CACHE_POISONING_LEVEL_3_JS_INJECTION=JavaScript Injection via Cache Poisoning. Since the application trusts the X-Forwarded-Host header to build asset URLs, an attacker can poison the cache to point to a malicious domain, causing all users to load and execute external scripts. |
There was a problem hiding this comment.
this seems to be not in sync with messages.properties. can you please fix it?
| return "https://" + StringEscapeUtils.escapeHtml4(host) + "/assets/cache-poisoning-demo.js"; | ||
| } | ||
|
|
||
| void clearCache() { |
There was a problem hiding this comment.
I think this should just clear the cache of that level, not other levels, but for now it is ok as this PR is having a lot of changes. we can take it as a newer issue.
| boolean resetCache, | ||
| HttpServletRequest request) { | ||
| if (resetCache) { | ||
| clearCache(); |
There was a problem hiding this comment.
resetCache flag is present in all the levels. I think we can have a newer function with spring annotations and expose just clearcache as an api call example:
@PostMapping("/clearCache")
public String clearCache() {
cache.clear();
return "Cache cleared";
}
then the URL will become something like /vulnerableapp/cachepoisioning/clearCache. you can create a newer function in vulnerableApp.js to return base url for the vulnerability and add clearCache to it.
In case you please to add a function to VulnerableApp.js, please add it to https://github.com/SasanLabs/VulnerableApp-facade/blob/main/facade-app/public/GlobalUtility.js as well
| htmlTemplate = "Common/CachePoisoning") | ||
| public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel1( | ||
| @RequestParam(value = BANNER_QUERY_PARAMETER, required = false) String banner, | ||
| @RequestParam( |
There was a problem hiding this comment.
I tried testing clearCache but it is not working. Seems like some bug. On clicking clear cache, if i again make a victim request, it is still returning me cached values.
| Pattern.compile("(?is)<\\s*/?\\s*script\\b[^>]*>"); | ||
| private static final Pattern JAVASCRIPT_SCHEME_PATTERN = Pattern.compile("(?i)javascript\\s*:"); | ||
|
|
||
| private final ConcurrentHashMap<String, CachedResponse> cache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
I think we need to make it static final, else it is not working as per expected.
| static final String EMPTY_CACHE_KEY = "-"; | ||
| static final String CACHE_STATUS_HIT = "HIT"; | ||
| static final String CACHE_STATUS_MISS = "MISS"; | ||
| static final String CACHE_CONTROL_PUBLIC = "public, max-age=60"; |
There was a problem hiding this comment.
I think it is causing a bit of difficulty to understand. this is a double edged sword.
I am thinking of adding extra levels where it is always fetches from the server i.e. no caching on browser layer for students and levels where browser side cache is there for scanners to find out that poisioning can be a candidate.
Any thoughts? shall we add multiple levels or add a flag within the level where users can choose browser level cache along with server level cache or just server cache?
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
Overall, @luks-santos this is really a great PR. I have added a few comments, I think once they are resolved, i think we can merge it quickly. Just a few thoughts on browser side caching issues which we need to work on.
3 participants
Summary
Closes #516.
This PR delivers the full level progression for the Cache Poisoning vulnerability, from the basic route-only cache key issue to the secure implementation, and adds the supporting frontend, hints, i18n, and automated tests
needed for the lab experience.
Implemented levels
bannerquery parameter is rendered unescaped (also exploitable as stored XSS via the shared cache) while the cache key uses only the route.bannerparameter does not fix the underlying issue because the cache key is still route-only — alternative unkeyed inputs keep the poisoning viable.X-Forwarded-Hostheader used to build absolute asset URLs.Cache-Control: private, no-store.