Apply automated formatting to first batch of directories.#497
Apply automated formatting to first batch of directories.#497
Conversation
e1f0355 to
9e52d52
Compare
9e52d52 to
1e6c09f
Compare
| print("Resource created with ID: %s.%s" % (resource_name, msg)) | ||
| else: | ||
| print('Resource creation failed: %s' % msg) | ||
| print("Resource creation failed: %s" % msg) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
General approach:
Avoid printing or logging raw error/status messages that may contain sensitive data. Instead, show a generic message to the user/console, and, if needed, log the detailed message to a controlled logger that is already part of the configuration system.
Best concrete fix for this code:
- In
mig/server/createresource.py, change the failure branch so that it does not interpolatemsginto the printed text. Instead:- Print a generic failure message to stdout/stderr that does not contain
msgat all (or at most a high-level hint). - Optionally, log the detailed
msgthrough the configuration logger (configuration.logger) so that operators can still see the precise error in server logs, but users / generic log readers do not see potentially sensitive content. This uses existing configuration/ logging infrastructure and avoids changing the behavior of resource creation itself.
- Print a generic failure message to stdout/stderr that does not contain
Details:
- File:
mig/server/createresource.py-
After
configuration = get_configuration_object(), we can safely accessconfiguration.logger(used similarly in other modules), but we must not assume its exact name in other files; here we just use it locally. -
Replace:
if create_status: print("Resource created with ID: %s.%s" % (resource_name, msg)) else: print("Resource creation failed: %s" % msg)
with something like:
if create_status: print("Resource created with ID: %s.%s" % (resource_name, msg)) else: # Avoid printing internal error details (may contain sensitive data) try: logger = configuration.logger except AttributeError: logger = None if logger: logger.error("Resource creation failed for %s: %s", resource_name, msg) print("Resource creation failed; see server log for details.")
-
This preserves successful behavior exactly (still prints the new ID, which is not sensitive as per current design) and only changes failure behavior to avoid echoing the full error text to the console.
-
No changes are required in the other modules for this particular alert, since the primary problematic sink is the console printing in createresource.py. The flow within vncsession that exposes passwords to the browser (HTML with <param name='password' ...> and “VNC password: %s”) is a separate UX/security decision but not the sink CodeQL is currently complaining about in this alert chain.
| @@ -65,4 +65,9 @@ | ||
| if create_status: | ||
| print("Resource created with ID: %s.%s" % (resource_name, msg)) | ||
| else: | ||
| print("Resource creation failed: %s" % msg) | ||
| # Do not print detailed error message directly; it may contain sensitive data | ||
| logger = getattr(configuration, "logger", None) | ||
| if logger is not None: | ||
| logger.error("Resource creation failed for %s: %s", | ||
| resource_name, msg) | ||
| print("Resource creation failed; see server log for details.") |
| ssl_ctx = hardened_ssl_context( | ||
| configuration, key_path, cert_path, dhparams_path | ||
| ) | ||
| httpserver.socket = ssl_ctx.wrap_socket( |
Check failure
Code scanning / CodeQL
Use of insecure SSL/TLS version High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
In general terms, the fix is to ensure that the SSLContext used for wrapping the server socket only permits modern, secure protocol versions (TLS 1.2 or higher). In Python 3.7+, the preferred method is to set context.minimum_version = ssl.TLSVersion.TLSv1_2. For older Python versions where minimum_version is unavailable, you can explicitly disable TLS 1.0 and 1.1 by setting context.options |= ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1. Both approaches can be used together in a version-aware way so that the same code works on a wide range of interpreters.
The single best change here, without altering the rest of the functionality or relying on the unseen implementation of hardened_ssl_context, is to harden ssl_ctx immediately after it is returned. In mig/server/grid_openid.py, inside start_service, after ssl_ctx = hardened_ssl_context(...) and before httpserver.socket = ssl_ctx.wrap_socket(...), we can import ssl at the top of the file (if not already imported) and then add a small block that: (1) checks for hasattr(ssl_ctx, "minimum_version") and sets it to ssl.TLSVersion.TLSv1_2 when available, and (2) ORs in OP_NO_TLSv1 and OP_NO_TLSv1_1 flags if they exist on the ssl module. This keeps the overall behavior the same except that weak protocol versions can no longer be negotiated, and it addresses both alert variants (TLSv1 and TLSv1_1).
Concretely:
- In
mig/server/grid_openid.pynear the other imports, addimport ssl. - In
start_service, between lines 1835–1837 (wheressl_ctxis created) and line 1838 (wherewrap_socketis called), insert a block that hardensssl_ctxusingminimum_versionif available andOP_NO_TLSv1/OP_NO_TLSv1_1otherwise (or in addition). No other methods or definitions are required.
| @@ -77,6 +77,7 @@ | ||
| import os | ||
| import re | ||
| import socket | ||
| import ssl | ||
| import sys | ||
| import time | ||
| import types | ||
| @@ -1835,6 +1836,17 @@ | ||
| ssl_ctx = hardened_ssl_context( | ||
| configuration, key_path, cert_path, dhparams_path | ||
| ) | ||
| # Ensure only secure TLS protocol versions are allowed | ||
| if hasattr(ssl_ctx, "minimum_version") and hasattr(ssl, "TLSVersion"): | ||
| try: | ||
| ssl_ctx.minimum_version = ssl.TLSVersion.TLSv1_2 | ||
| except Exception: | ||
| pass | ||
| # Additionally disable older protocol versions where flags are available | ||
| for opt_name in ("OP_NO_TLSv1", "OP_NO_TLSv1_1"): | ||
| opt = getattr(ssl, opt_name, None) | ||
| if opt is not None: | ||
| ssl_ctx.options |= opt | ||
| httpserver.socket = ssl_ctx.wrap_socket( | ||
| httpserver.socket, server_side=True | ||
| ) |
| if errors: | ||
| print("Address lookup errors:") | ||
| print('\n'.join(errors)) | ||
| print("\n".join(errors)) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
In general, to fix clear-text logging of sensitive information, you must ensure that any data derived from passwords (or other secrets) is never written directly to logs or stdout/stderr. Either:
- Remove such logging entirely, or
- Replace it with generic, non-sensitive messages (e.g. “Address lookup failed; see server logs” instead of including dynamic error details that may contain secrets).
For this specific case, the simplest safe fix is to avoid printing the errors list contents in mig/server/notifypassword.py. The internal helper _user_general_notify already logs a detailed error message via _logger.error(err_msg) before appending a higher-level string to errors. Therefore, the CLI script does not need to echo those errors back to stdout; it can show only a generic message that an address lookup failed. This preserves existing functional behavior (sending password reminders, handling error conditions, exit codes) while eliminating the risk of leaking password-derived data to a user-visible or log-captured stream.
Concretely:
- In
mig/server/notifypassword.py, replace:with something like:if errors: print("Address lookup errors:") print("\n".join(errors))
or, if you want to keep the fact that errors exist but not their content. No change is needed inif errors: print("Address lookup errors occurred; see server logs for details.")
pwcrypto.pyoruseradm.pyfor this alert, since they are already using internal logging mechanisms, and the CodeQL sink is theprintcall innotifypassword.py.
| @@ -116,8 +116,10 @@ | ||
| ) | ||
|
|
||
| if errors: | ||
| print("Address lookup errors:") | ||
| print("\n".join(errors)) | ||
| # Do not print detailed error messages here to avoid leaking | ||
| # potentially sensitive information such as passwords. Details | ||
| # are already logged via the configuration logger. | ||
| print("Address lookup errors occurred; see server logs for details.") | ||
|
|
||
| if not addresses: | ||
| print("Error: found no suitable addresses") |
| "Sending password reminder(s) for '%s' to:\n%s" | ||
| % (user_id, "\n".join(notify_dict["NOTIFY"])) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
General approach: avoid ever transporting or exposing the actual password in cleartext. Instead of returning and notifying the user’s stored password, generate a one‑time reset mechanism (or similar), or at minimum ensure that no logs or notifications contain the password itself. Within the constraints of the provided snippets, the least intrusive and clearest improvement is to stop including the password in the data passed to notify_user and to ensure that local prints/logs only mention non‑secret metadata (user id, that a reminder/reset was sent, target addresses), not the password.
Best single change with minimal functional impact in the shown code: adjust notifypassword.py so that it no longer passes password into notify_user. The sensitive value is still retrieved in _user_general_notify/user_password_reminder, but the concrete sink CodeQL complains about can be removed by ensuring that the notification function is not given the password at all. Since we don’t have the implementation of notify_user, we can’t safely redesign the full password‑reminder flow to use reset tokens; however, we can keep the protocol invocation shape similar by replacing [user_id, password] with a benign placeholder or omitting the password while still including user_id. This directly removes the sensitive data from the suspected notification/logging path and addresses all the variants pointing at fields['password'] / user_password_reminder(...) as sources for what ultimately gets sent out.
Concretely, in mig/server/notifypassword.py:
- Keep obtaining
configurationandpasswordas before (to avoid broader behavioural changes). - Keep printing “Sending password reminder(s) for …” since it only lists user id and addresses.
- Change the call
notify_user(notify_dict, [user_id, password], "PASSWORDREMINDER", ...)so it does not propagatepassword. For example, pass just[user_id](or[user_id, "PASSWORDREMINDER"]) so the function still receives a list of arguments but without secrets.
No changes are needed in mig/shared/pwcrypto.py or mig/shared/base.py to address this clear‑text logging path, and we avoid altering cryptographic/storage semantics within the limited context. If you later adopt a secure reset‑token design, you would additionally:
- Stop storing passwords in a recoverable form.
- Remove the
unscramble_passworduse in_user_general_notifyand only ever verify hashes or reset tokens.
| @@ -136,7 +136,7 @@ | ||
| ) | ||
| notify_user( | ||
| notify_dict, | ||
| [user_id, password], | ||
| [user_id], | ||
| "PASSWORDREMINDER", | ||
| logger, | ||
| "", |
| % self.retry_url) | ||
| self.send_header('Content-type', 'text/html') | ||
| self.send_header( | ||
| "Set-Cookie", "retry_url=%s;secure;httponly" % self.retry_url |
Check warning
Code scanning / CodeQL
Construction of a cookie using user-supplied input Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
General approach: avoid putting raw, user-controlled URL strings directly into the cookie. Instead, either (1) stop storing user-controlled data in a cookie at all and recompute retry_url from the current request, or (2) encode it safely (e.g., base64) and decode/validate when reading it back. Since we cannot change surrounding logic outside the shown snippets, the least invasive and safest fix is to encode self.retry_url into a cookie-safe form and adjust the reader (__retry_url_from_cookie) to decode and validate it.
Best concrete fix here:
- When setting the cookie in
showPage, do not embedself.retry_urldirectly. Instead, base64-encode its UTF‑8 bytes and use that encoded string as the cookie value. Base64 uses only[A-Za-z0-9+/=]which are safe inside cookie values and cannot break header syntax. - When reading the cookie in
__retry_url_from_cookie, base64‑decode the stored value back to the original URL string (on success), then apply the existinghttpprefix check andvalid_url(retry_url)validation. On any decoding error, treat the cookie as invalid and clearretry_url(returnNone).
Required edits and locations:
- In
mig/server/grid_openid.py, inside__retry_url_from_cookie, replace the simple lookupretry_url = cookie_dict.get("retry_url", "")and immediate validation with:- retrieve the raw cookie value (
encoded_retry_url), - if present, base64‑decode it (handling errors),
- get a Unicode/native string from the decoded bytes (
force_native_str), - then perform the existing validation logic on the decoded
retry_url.
- retrieve the raw cookie value (
- In
mig/server/grid_openid.py, insideshowPage(the snippet around line 1761), change theSet-Cookieheader construction to:- base64‑encode
self.retry_url(after converting to UTF‑8 bytes as needed), - use that encoded string as the cookie value in the header.
- base64‑encode
We already import base64 and have force_native_str / force_utf8 utilities available, so no new imports are necessary. The fix does not change the functional meaning of retry_url for valid values: it is transparently encoded/decoded, and validation remains in place.
| @@ -73,6 +73,7 @@ | ||
| exit(1) | ||
|
|
||
| import base64 | ||
| import binascii | ||
| import cgitb | ||
| import os | ||
| import re | ||
| @@ -435,11 +436,22 @@ | ||
| cookies = self.headers.get("Cookie") | ||
| cookie = http.cookies.SimpleCookie(cookies) | ||
| cookie_dict = dict((k, v.value) for k, v in cookie.items()) | ||
| retry_url = cookie_dict.get("retry_url", "") | ||
| if retry_url and retry_url.startswith("http"): | ||
| raise InputException("invalid retry_url: %s" % retry_url) | ||
| elif retry_url: | ||
| valid_url(retry_url) | ||
| encoded_retry_url = cookie_dict.get("retry_url", "") | ||
| if encoded_retry_url: | ||
| try: | ||
| # Cookie value is base64-encoded to avoid injecting raw user input | ||
| decoded_bytes = base64.b64decode(encoded_retry_url) | ||
| except (TypeError, binascii.Error) as err: | ||
| # Invalid base64 content in cookie | ||
| logger.error("found invalid retry_url cookie encoding: %s" % err) | ||
| retry_url = None | ||
| else: | ||
| # Convert decoded bytes to native string for further validation | ||
| retry_url = force_native_str(decoded_bytes) | ||
| if retry_url and retry_url.startswith("http"): | ||
| raise InputException("invalid retry_url: %s" % retry_url) | ||
| elif retry_url: | ||
| valid_url(retry_url) | ||
| except http.cookies.CookieError as err: | ||
| retry_url = None | ||
| logger.error("found invalid cookie: %s" % err) | ||
| @@ -1760,9 +1772,13 @@ | ||
|
|
||
| self.send_response(response_code) | ||
| self.writeUserHeader() | ||
| self.send_header( | ||
| "Set-Cookie", "retry_url=%s;secure;httponly" % self.retry_url | ||
| ) | ||
| # Encode retry_url to a cookie-safe representation before setting cookie | ||
| if self.retry_url: | ||
| retry_url_bytes = force_utf8(self.retry_url) | ||
| encoded_retry_url = base64.b64encode(retry_url_bytes).decode("ascii") | ||
| self.send_header( | ||
| "Set-Cookie", "retry_url=%s;secure;httponly" % encoded_retry_url | ||
| ) | ||
| self.send_header("Content-type", "text/html") | ||
| self.end_headers() | ||
| page_template = openid_page_template(configuration, head_extras) |
| % self.retry_url) | ||
| self.send_header('Content-type', 'text/html') | ||
| self.send_header( | ||
| "Set-Cookie", "retry_url=%s;secure;httponly" % self.retry_url |
Check warning
Code scanning / CodeQL
HTTP Response Splitting Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
In general, to fix HTTP response splitting issues, any user-controlled data used in HTTP headers must be sanitized or encoded so that it cannot contain carriage return (\r), linefeed (\n), or other characters that break header structure (and in cookie values, semicolons ; are also often problematic). Two common approaches are: (1) strictly validating the data against an allowlist of safe characters (e.g., URL-encoding it or allowing only a limited character set), or (2) stripping or replacing CR/LF characters before inserting into headers. For a cookie value that is itself a URL, URL-encoding the value is typically the most compatible and least intrusive approach.
For this specific case, the best minimal fix is to ensure that self.retry_url is encoded or sanitized right before it is inserted into the Set-Cookie header. Since self.retry_url is a URL-like string, we can URL-encode it when building the cookie value, and correspondingly URL-decode it when reading the cookie back in __retry_url_from_cookie. This way, even if an attacker manages to put control characters into self.retry_url, they will be safely percent-encoded in the header and will not break the HTTP response. This preserves existing semantics for normal URLs and keeps the change localized.
Concretely, in mig/server/grid_openid.py:
- At the header construction site (around line 1761–1765), instead of directly interpolating
self.retry_urlinto theSet-Cookievalue, we should applyurllib.parse.quote(or the Python 2 equivalent) to it, e.g.,quote(self.retry_url, safe=""), to ensure there are no CR/LF characters and no cookie-delimiter characters leaking into the header. - In
__retry_url_from_cookie(around lines 431–450), where theretry_urlcookie is read, we should apply the matchingurllib.parse.unquoteto decode the stored value back into the original URL before validating it withvalid_url. This preserves functionality while ensuring that the cookie value in the header is always a safe, encoded representation. - We need to import
quoteandunquotefromurllib.parse(or the compatibility equivalent already in use in this module, if present elsewhere) at the top ofmig/server/grid_openid.py. Since we are only allowed to use well-known standard libraries and not to alter other imports, we will add a direct import.
This change is fully contained within mig/server/grid_openid.py, in the shown snippets.
| @@ -80,6 +80,7 @@ | ||
| import sys | ||
| import time | ||
| import types | ||
| from urllib.parse import quote, unquote | ||
|
|
||
| try: | ||
| import openid | ||
| @@ -436,6 +437,9 @@ | ||
| cookie = http.cookies.SimpleCookie(cookies) | ||
| cookie_dict = dict((k, v.value) for k, v in cookie.items()) | ||
| retry_url = cookie_dict.get("retry_url", "") | ||
| if retry_url: | ||
| # Decode the stored retry_url value to restore original URL | ||
| retry_url = unquote(retry_url) | ||
| if retry_url and retry_url.startswith("http"): | ||
| raise InputException("invalid retry_url: %s" % retry_url) | ||
| elif retry_url: | ||
| @@ -1760,8 +1764,10 @@ | ||
|
|
||
| self.send_response(response_code) | ||
| self.writeUserHeader() | ||
| # Encode retry_url to ensure it is safe for use in a Set-Cookie header | ||
| safe_retry_url = quote(self.retry_url or "", safe="") | ||
| self.send_header( | ||
| "Set-Cookie", "retry_url=%s;secure;httponly" % self.retry_url | ||
| "Set-Cookie", "retry_url=%s;secure;httponly" % safe_retry_url | ||
| ) | ||
| self.send_header("Content-type", "text/html") | ||
| self.end_headers() |
No description provided.