Keep Tool Configuration credentials out of the edit form

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
 

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)
 

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)