-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Improve cvssv3 validation #12440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
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
import logging | |
from dojo.models import Finding | |
from dojo.utils import parse_cvss_data | |
logger = logging.getLogger(__name__) | |
⚠️ 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
from dojo.models import Endpoint, Finding | |
from dojo.tools.qualys import csv_parser | |
from dojo.utils import parse_cvss_data | |
logger = logging.getLogger(__name__) | |
⚠️ 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
import bleach | |
import crum | |
import cvss.parser | |
import hyperlink | |
import vobject | |
from asteval import Interpreter | |
from cryptography.hazmat.backends import default_backend | |
from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes | |
from cvss.cvss3 import CVSS3 | |
from dateutil.parser import parse | |
from dateutil.relativedelta import MO, SU, relativedelta | |
from django.conf import settings | |
from django.contrib import messages | |
from django.contrib.auth.signals import user_logged_in, user_logged_out, user_login_failed | |
from django.core.paginator import Paginator | |
from django.db.models import Case, Count, IntegerField, Q, Sum, Value, When | |
from django.db.models.query import QuerySet |
⚠️ 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
import logging | |
import re | |
from collections.abc import Callable | |
import cvss.parser | |
from cvss import CVSS2, CVSS3, CVSS4 | |
from django.core.exceptions import ValidationError | |
logger = logging.getLogger(__name__) | |
def tag_validator(value: str | list[str], exception_class: Callable = ValidationError) -> None: | |
TAG_PATTERN = re.compile(r'[ ,\'"]') | |
error_messages = [] | |
if isinstance(value, list): | |
error_messages.extend(f"Invalid tag: '{tag}'. Tags should not contain spaces, commas, or quotes." for tag in value if TAG_PATTERN.search(tag)) | |
elif isinstance(value, str): | |
if TAG_PATTERN.search(value): | |
error_messages.append(f"Invalid tag: '{value}'. Tags should not contain spaces, commas, or quotes.") | |
else: | |
error_messages.append(f"Value must be a string or list of strings: {value} - {type(value)}.") | |
if error_messages: | |
logger.debug(f"Tag validation failed: {error_messages}") | |
raise exception_class(error_messages) | |
def cvss3_validator(value: str | list[str], exception_class: Callable = ValidationError) -> None: | |
logger.error("cvss3_validator called with value: %s", value) | |
cvss_vectors = cvss.parser.parse_cvss_from_text(value) | |
if len(cvss_vectors) > 0: | |
vector_obj = cvss_vectors[0] | |
if isinstance(vector_obj, CVSS3): | |
# all is good | |
return | |
if isinstance(vector_obj, CVSS4): | |
# CVSS4 is not supported yet by the parse_cvss_from_text function, but let's prepare for it anyway: https://github.com/RedHatProductSecurity/cvss/issues/53 | |
msg = "Unsupported CVSS(4) version detected." | |
raise exception_class(msg) | |
if isinstance(vector_obj, CVSS2): | |
msg = "Unsupported CVSS(2) version detected." | |
raise exception_class(msg) | |
msg = "Unsupported CVSS version detected." | |
raise exception_class(msg) | |
# Explicitly raise an error if no CVSS vectors are found, | |
# to avoid 'NoneType' errors during severity processing later. | |
msg = "No valid CVSS vectors found by cvss.parse_cvss_from_text()" | |
raise exception_class(msg) |
⚠️ 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
fi | |
echo "Running docker compose unit tests with test case $TEST_CASE ..." | |
# Compose V2 integrates compose functions into the Docker platform, continuing to support | |
# most of the previous docker-compose features and flags. You can run Compose V2 by | |
# replacing the hyphen (-) with a space, using docker compose, instead of docker-compose. | |
docker compose exec uwsgi bash -c "python manage.py test $TEST_CASE -v2 --keepdb" |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good - only a couple questions/nits
docs/content/en/open_source/contributing/how-to-write-a-parser.md
Outdated
Show resolved
Hide resolved
dojo/models.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe only when setting a CVSS3 vector through the UI or API, but not when a (re)import is done.
dojo/db_migrations/0229_alter_finding_cvssv3_alter_finding_template_cvssv3.py
Outdated
Show resolved
Hide resolved
112adfe
to
a2f1f98
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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
from datetime import datetime | |
from dojo.models import Finding | |
from dojo.utils import parse_cvss_data | |
class JFrogXrayUnifiedParser: |
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
import bleach | |
import crum | |
import cvss.parser | |
import hyperlink | |
import vobject | |
from asteval import Interpreter | |
from cryptography.hazmat.backends import default_backend | |
from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes | |
from cvss.cvss3 import CVSS3 | |
from dateutil.parser import parse | |
from dateutil.relativedelta import MO, SU, relativedelta | |
from django.conf import settings | |
from django.contrib import messages | |
from django.contrib.auth.signals import user_logged_in, user_logged_out, user_login_failed | |
from django.core.paginator import Paginator | |
from django.db.models import Case, Count, IntegerField, Q, Sum, Value, When | |
from django.db.models.query import QuerySet |
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
import logging | |
import re | |
from collections.abc import Callable | |
import cvss.parser | |
from cvss import CVSS2, CVSS3, CVSS4 | |
from django.core.exceptions import ValidationError | |
logger = logging.getLogger(__name__) | |
def tag_validator(value: str | list[str], exception_class: Callable = ValidationError) -> None: | |
TAG_PATTERN = re.compile(r'[ ,\'"]') | |
error_messages = [] | |
if isinstance(value, list): | |
error_messages.extend(f"Invalid tag: '{tag}'. Tags should not contain spaces, commas, or quotes." for tag in value if TAG_PATTERN.search(tag)) | |
elif isinstance(value, str): | |
if TAG_PATTERN.search(value): | |
error_messages.append(f"Invalid tag: '{value}'. Tags should not contain spaces, commas, or quotes.") | |
else: | |
error_messages.append(f"Value must be a string or list of strings: {value} - {type(value)}.") | |
if error_messages: | |
logger.debug(f"Tag validation failed: {error_messages}") | |
raise exception_class(error_messages) | |
def cvss3_validator(value: str | list[str], exception_class: Callable = ValidationError) -> None: | |
logger.error("cvss3_validator called with value: %s", value) | |
cvss_vectors = cvss.parser.parse_cvss_from_text(value) | |
if len(cvss_vectors) > 0: | |
vector_obj = cvss_vectors[0] | |
if isinstance(vector_obj, CVSS3): | |
# all is good | |
return | |
if isinstance(vector_obj, CVSS4): | |
# CVSS4 is not supported yet by the parse_cvss_from_text function, but let's prepare for it anyway: https://github.com/RedHatProductSecurity/cvss/issues/53 | |
msg = "Unsupported CVSS(4) version detected." | |
raise exception_class(msg) | |
if isinstance(vector_obj, CVSS2): | |
msg = "Unsupported CVSS(2) version detected." | |
raise exception_class(msg) | |
msg = "Unsupported CVSS version detected." | |
raise exception_class(msg) | |
# Explicitly raise an error if no CVSS vectors are found, | |
# to avoid 'NoneType' errors during severity processing later. | |
msg = "No valid CVSS vectors found by cvss.parse_cvss_from_text()" | |
raise exception_class(msg) |
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
cvss
library 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.utils
to 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_score
if 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.