Improve cvssv3 validation#12440
Conversation
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
3c962cd to
b0dda81
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request reveals multiple security vulnerabilities across different files, including potential denial of service risks from CVSS parsing, command injection in a unittest script, information disclosure through logging, and inconsistent vulnerability scoring that could misrepresent security severity.
|
| Vulnerability | Potential Malicious Input Processing |
|---|---|
| Description | Multiple parsers (Aqua, npm audit, JFrog Xray) introduce parsing of external, untrusted CVSS vector data using parse_cvss_data. While the function is likely designed to be robust, processing external input without comprehensive validation could lead to unexpected behavior or resource consumption issues if the input is maliciously crafted. |
django-DefectDojo/dojo/tools/npm_audit_7_plus/parser.py
Lines 3 to 9 in 1a522af
⚠️ CVSS Score Inconsistency in dojo/tools/qualys/parser.py
| Vulnerability | CVSS Score Inconsistency |
|---|---|
| Description | In the Qualys parser, the code unconditionally overwrites the CVSSv3 score derived from the vector with a potentially different CVSS_value. This could lead to incorrect severity representation if the scores from different sources (v2 vs v3) are mixed, potentially misrepresenting the true vulnerability severity. |
django-DefectDojo/dojo/tools/qualys/parser.py
Lines 8 to 14 in 1a522af
⚠️ Potential Denial of Service via CVSS Parsing in dojo/utils.py
| Vulnerability | Potential Denial of Service via CVSS Parsing |
|---|---|
| Description | The parse_cvss_data function in dojo/utils.py passes cvss_vector_string directly to cvss.parser.parse_cvss_from_text without input validation. A maliciously crafted CVSS vector string could potentially cause excessive resource consumption during parsing, leading to a denial of service condition. |
django-DefectDojo/dojo/utils.py
Lines 15 to 32 in 1a522af
⚠️ Information Disclosure via Logging in dojo/validators.py
| Vulnerability | Information Disclosure via Logging |
|---|---|
| Description | The tag_validator and cvss3_validator functions log user-controlled input directly, which could expose sensitive details or aid attacker reconnaissance. Specifically, logger.error("cvss3_validator called with value: %s", value) and the error message construction in tag_validator embed unvalidated user input in log messages and error responses. |
django-DefectDojo/dojo/validators.py
Lines 1 to 53 in 1a522af
⚠️ Command Injection Risk in run-unittest.sh
| Vulnerability | Command Injection Risk |
|---|---|
| Description | The run-unittest.sh script executes a shell command using the user-controlled $TEST_CASE variable without proper sanitization. An attacker could manipulate the TEST_CASE input to inject arbitrary shell commands, potentially leading to remote code execution. The command docker compose exec uwsgi bash -c "python manage.py test $TEST_CASE -v2 --keepdb" directly interpolates user input into a shell command without escaping. |
django-DefectDojo/run-unittest.sh
Lines 51 to 57 in 1a522af
All finding details can be found in the DryRun Security Dashboard.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
4389265 to
5ab1c71
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Maffooch
left a comment
There was a problem hiding this comment.
This is looking really good - only a couple questions/nits
| except Exception as ex: | ||
| logger.error("Can't compute cvssv3 score for finding id %i. Invalid cvssv3 vector found: '%s'. Exception: %s", self.id, self.cvssv3, ex) | ||
| logger.warning("Can't compute cvssv3 score for finding id %i. Invalid cvssv3 vector found: '%s'. Exception: %s.", self.id, self.cvssv3, ex) | ||
| # should we set self.cvssv3 to None here to avoid storing invalid vectors? it would also remove invalid vectors on existing findings... |
There was a problem hiding this comment.
I think we should remove invalid vectors, but only if the id is None before the finding is saved. That allows us to prevent the issue from spreading even further without implications for existing data
There was a problem hiding this comment.
addressed this, but maybe we should just throw a ValidationError? That is what will happen in the future if we implement better validation support for Findings in general via clean.
There was a problem hiding this comment.
actually, I think the validator will do this already, as its called before the save method is, right?
There was a problem hiding this comment.
I believe only when setting a CVSS3 vector through the UI or API, but not when a (re)import is done.
112adfe to
a2f1f98
Compare
|
I will resolve the conflicts after 2.47.3 is release and merged back into dev. There might be more conflicts by then and I don't want to burden the release process with merge conflicts by merging this PR to dev before 2.47.3. |
1a522af to
1bfcc81
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request contains multiple low-risk findings related to CVSS vector parsing, including potential denial of service risks, sensitive input logging, and information disclosure through error messages, though none of the issues are considered blocking or critical.
Potential DoS via CVSS Parsing in
|
| Vulnerability | Potential DoS via CVSS Parsing |
|---|---|
| Description | Multiple parsers introduce a utility function parse_cvss_data() which processes untrusted CVSS vector inputs. While the function appears to use a library for parsing, there's a potential risk of resource exhaustion if maliciously crafted CVSS vectors are processed. The parsing occurs in multiple tools like JFrog Xray, Sonatype, and others, increasing the attack surface. |
django-DefectDojo/dojo/tools/jfrog_xray_unified/parser.py
Lines 2 to 8 in 15fc291
Logging Sensitive Input in dojo/utils.py
| Vulnerability | Logging Sensitive Input |
|---|---|
| Description | The parse_cvss_data() function in utils.py logs the full CVSS vector string when parsing fails. In a debug or error logging context, this could potentially expose sensitive information if the CVSS vector contains unexpected data. The logging occurs at debug level, which mitigates some risk but doesn't eliminate it completely. |
django-DefectDojo/dojo/utils.py
Lines 15 to 32 in 15fc291
Validator Information Disclosure in dojo/validators.py
| Vulnerability | Validator Information Disclosure |
|---|---|
| Description | The validators.py contains error handling that logs and constructs error messages embedding user input. While the input is expected to be CVSS vectors, the direct inclusion of input in error messages could potentially leak implementation details or be used for reconnaissance. |
django-DefectDojo/dojo/validators.py
Lines 1 to 53 in 15fc291
All finding details can be found in the DryRun Security Dashboard.
Although it's not fully clear "what the problem is" reported in #8264 , we want to stop using the regex for validation and use the
cvsslibrary in a validator. This PR updates the validator on the model and updates the test cases to reflect the new behaviour. The new behaviour is better:Finding.save()is called, no validation is perfrmed by Django. So we have make sure the parsers only store valid vectors.I updated the parsers that did not yet follow the guidelines in
docs/content/en/open_source/contributing/how-to-write-a-parser.md. I considered:set_data_from_cvss_string(...)to the Finding modelFinding.save()to validate/parse the vector stringBut in the end I went for a helper method in
dojo.utilsto parse the CVSS vector. This way we have the parsing logic in one central place and the parsers can decide which of the 3 parsed fields they want to use on the model (vector,score,severity). The logic is also failsafe, invalid vectors result in empty cvss fields but won't cause the import to fail.Remaining questions:
Finding.save()that sets (overwrites) thecvssv3_scoreif a valid vector is set? (I think for now YES)Finding.save()encounters an invalid vector incvssv3. Should we clear the vector before saving? (I think for now NO)Risks:
Feedback welcome. I also think this is a nice preparation for when we want to support CVSS4 vectors.