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)