Conversation
Maffooch
left a comment
There was a problem hiding this comment.
Very clean implementation!
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
🟡 Please give this pull request extra attention during review.This pull request introduces parser code that constructs LocationData.url(...) directly from unvalidated user input (affected_target) without URL allowlisting, DNS/IP checks, or protocol restrictions, which could enable server-side request forgery (SSRF) if those locations are later used to make network requests. The changes rely on downstream cleaning that logs or accepts invalid locations rather than blocking external/internal addresses, so attacker-controlled URLs may not be sufficiently constrained.
🟡 Potential Server-Side Request Forgery (SSRF) in
|
| Vulnerability | Potential Server-Side Request Forgery (SSRF) |
|---|---|
| Description | Parsers directly construct LocationData.url(url=affected_target) from input (affected_target) without any URL allowlist, DNS/IP validation, or protocol restriction in the visible changes. LocationData objects produced here are later converted/cleaned by LocationManager.make_abstract_locations / clean_unsaved_locations, but those functions only attempt to convert LocationData to AbstractLocation (URL.from_location_data) and call .clean(), which will log/accept invalid locations rather than block external network targets. There is no evidence in the provided patch that attacker-controlled URLs are validated against an allowlist or that resolved IPs are checked for internal/private ranges before being associated and potentially used by other application components. Because user-controlled URL strings flow into LocationData.url(...) with no shown validation, this can enable SSRF if later handling causes server-side requests using those locations. |
django-DefectDojo/dojo/tools/api_cobalt/parser.py
Lines 151 to 154 in 152005b
All finding details can be found in the DryRun Security Dashboard.
This PR updates how parser/importer interaction works for locations in V3. Rather than having parsers return lists of concrete model types, they return LocationData objects, a dataclass that can be changed separate from models. This also allows us to start tracking other data that might be useful in the future that is not tied directly to a model class; this PR introduces dependency info gathering to that end. Parsers have been updated accordingly.
A few other changes are present to locations-related models. The
hashfield on URL has been renamed toidentity_hashand some fields have been added to Location(Finding|Product)Reference models to allow us to track data pertinent to the association between a Location and Finding/Product.