From 08150dc4c729f0b59110b7d0d0f292d0a25594f7 Mon Sep 17 00:00:00 2001 From: svader0 Date: Tue, 11 Aug 2026 14:19:19 -0500 Subject: [PATCH] Keep Tool Configuration credentials out of the edit form The edit view decrypted the stored password and ssh key on GET and the form bound all three credential fields with their values, so anyone permitted to edit a tool configuration could read the credentials it holds. The Django admin form for the same model already masks these fields. Stop sending the stored values to the browser and treat a blank submission as "unchanged" on save, which also fixes clearing a credential by saving the form without re-entering it. The stored value is only reused while the URL is unchanged, so a credential is never paired with a destination supplied in the same request. --- dojo/tool_config/ui/forms.py | 11 +++ dojo/tool_config/ui/views.py | 17 +++- .../test_tool_config_credential_disclosure.py | 95 +++++++++++++++++++ 3 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 unittests/test_tool_config_credential_disclosure.py diff --git a/dojo/tool_config/ui/forms.py b/dojo/tool_config/ui/forms.py index cc769725d78..3d4309701a5 100644 --- a/dojo/tool_config/ui/forms.py +++ b/dojo/tool_config/ui/forms.py @@ -6,13 +6,24 @@ class ToolConfigForm(forms.ModelForm): + # Stored values are never sent back to the browser. A blank submission means + # "unchanged", which dojo.tool_config.ui.views applies on save. + CREDENTIAL_FIELDS = ("password", "ssh", "api_key") + tool_type = forms.ModelChoiceField(queryset=Tool_Type.objects.all(), label="Tool Type") + password = forms.CharField(widget=forms.PasswordInput, required=False, max_length=900) ssh = forms.CharField(widget=forms.Textarea(attrs={}), required=False, label="SSH Key") + api_key = forms.CharField(widget=forms.PasswordInput, required=False, max_length=900, label="API Key") class Meta: model = Tool_Configuration exclude = ["product"] + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + for field in self.CREDENTIAL_FIELDS: + self.initial[field] = "" + def clean(self): form_data = self.cleaned_data diff --git a/dojo/tool_config/ui/views.py b/dojo/tool_config/ui/views.py index 163341a1500..1d75127c31d 100644 --- a/dojo/tool_config/ui/views.py +++ b/dojo/tool_config/ui/views.py @@ -10,7 +10,7 @@ from dojo.tool_config.factory import create_API from dojo.tool_config.models import Tool_Configuration from dojo.tool_config.ui.forms import ToolConfigForm -from dojo.utils import add_breadcrumb, dojo_crypto_encrypt, prepare_for_view +from dojo.utils import add_breadcrumb, dojo_crypto_encrypt logger = logging.getLogger(__name__) @@ -53,12 +53,21 @@ def new_tool_config(request): @deprecated_view("Tool Configuration", removal_version="3.5.0", removal_date="November 2026") def edit_tool_config(request, ttid): tool_config = Tool_Configuration.objects.get(pk=ttid) + # Read before the form binds, which overwrites the instance in place. + stored = {field: getattr(tool_config, field) for field in ToolConfigForm.CREDENTIAL_FIELDS} + stored_url = tool_config.url if request.method == "POST": tform = ToolConfigForm(request.POST, instance=tool_config) if tform.is_valid(): form_copy = tform.save(commit=False) - form_copy.password = dojo_crypto_encrypt(tform.cleaned_data["password"]) - form_copy.ssh = dojo_crypto_encrypt(tform.cleaned_data["ssh"]) + # A blank credential means "leave it as it is", but only while the URL + # is unchanged. Pairing a stored secret with a destination submitted in + # the same request would send it to a host of the editor's choosing. + reuse = form_copy.url == stored_url + submitted = tform.cleaned_data + form_copy.password = stored["password"] if reuse and not submitted["password"] else dojo_crypto_encrypt(submitted["password"]) + form_copy.ssh = stored["ssh"] if reuse and not submitted["ssh"] else dojo_crypto_encrypt(submitted["ssh"]) + form_copy.api_key = stored["api_key"] if reuse and not submitted["api_key"] else submitted["api_key"] try: api = create_API(form_copy) if api and hasattr(api, "test_connection"): @@ -80,8 +89,6 @@ def edit_tool_config(request, ttid): str(e), extra_tags="alert-danger") else: - tool_config.password = prepare_for_view(tool_config.password) - tool_config.ssh = prepare_for_view(tool_config.ssh) tform = ToolConfigForm(instance=tool_config) add_breadcrumb(title="Edit Tool Configuration", top_level=False, request=request) diff --git a/unittests/test_tool_config_credential_disclosure.py b/unittests/test_tool_config_credential_disclosure.py new file mode 100644 index 00000000000..98f1b8d15f1 --- /dev/null +++ b/unittests/test_tool_config_credential_disclosure.py @@ -0,0 +1,95 @@ +""" +The Tool Configuration edit page must not return stored credentials. + +``dojo.change_tool_configuration`` lets a non-superuser edit the instance's tool +configurations. The page used to decrypt the stored password and ssh key and bind +all three credential fields into the rendered form, so the permission also handed +out every stored integration credential in cleartext. +""" + +from django.contrib.auth.models import Permission +from django.test import Client +from django.urls import reverse + +from dojo.models import Tool_Configuration, Tool_Type, User +from dojo.utils import dojo_crypto_encrypt, prepare_for_view + +from .dojo_test_case import DojoTestCase, versioned_fixtures + +PASSWORD = "stored-password-value" +SSH_KEY = "stored-ssh-key-value" +API_KEY = "stored-api-key-value" +URL = "https://scanner.example.com" +NEW_PASSWORD = "replacement-password-value" + + +@versioned_fixtures +class ToolConfigCredentialDisclosureTest(DojoTestCase): + fixtures = ["dojo_testdata.json"] + + def setUp(self): + # A tool type outside SCAN_APIS, so saving does not attempt a connection. + tool_type, _ = Tool_Type.objects.get_or_create(name="Disclosure Test Tool") + self.tool_config = Tool_Configuration.objects.create( + name="victim configuration", + tool_type=tool_type, + url=URL, + authentication_type="Password", + username="service-account", + password=dojo_crypto_encrypt(PASSWORD), + ssh=dojo_crypto_encrypt(SSH_KEY), + api_key=API_KEY, + ) + self.editor = User.objects.create(username="tool_config_editor") + self.editor.user_permissions.add( + Permission.objects.get(content_type__app_label="dojo", codename="change_tool_configuration"), + ) + self.url = reverse("edit_tool_config", args=[self.tool_config.id]) + self.client = Client() + self.client.force_login(self.editor) + + def _post(self, overrides): + data = { + "name": self.tool_config.name, + "tool_type": self.tool_config.tool_type.id, + "url": URL, + "authentication_type": "Password", + "username": "service-account", + "password": "", + "ssh": "", + "api_key": "", + } + data.update(overrides) + response = self.client.post(self.url, data) + self.tool_config.refresh_from_db() + return response + + def test_edit_page_does_not_return_the_stored_credentials(self): + response = self.client.get(self.url) + self.assertEqual(response.status_code, 200, response.content[:300]) + for secret in (PASSWORD, SSH_KEY, API_KEY): + self.assertNotIn(secret.encode(), response.content) + + def test_blank_credentials_keep_the_stored_values(self): + self.assertEqual(self._post({"name": "renamed configuration"}).status_code, 302) + self.assertEqual(self.tool_config.name, "renamed configuration") + self.assertEqual(prepare_for_view(self.tool_config.password), PASSWORD) + self.assertEqual(prepare_for_view(self.tool_config.ssh), SSH_KEY) + self.assertEqual(self.tool_config.api_key, API_KEY) + + def test_a_submitted_credential_replaces_the_stored_one_and_is_encrypted(self): + self.assertEqual(self._post({"password": NEW_PASSWORD}).status_code, 302) + self.assertTrue(self.tool_config.password.startswith("AES.")) + self.assertEqual(prepare_for_view(self.tool_config.password), NEW_PASSWORD) + # The fields left blank are still untouched. + self.assertEqual(prepare_for_view(self.tool_config.ssh), SSH_KEY) + + def test_blank_credentials_are_not_reused_against_a_new_url(self): + """ + An editor cannot read the credentials any more, so they must not be able to + pair them with a destination of their own choosing either. + """ + self.assertEqual(self._post({"url": "https://attacker.example.net"}).status_code, 302) + self.assertNotEqual(prepare_for_view(self.tool_config.password), PASSWORD) + self.assertNotEqual(prepare_for_view(self.tool_config.ssh), SSH_KEY) + self.assertNotEqual(self.tool_config.api_key, API_KEY)