From ccfbd2c457ade65bdb4f898fd3d4126af61074bf Mon Sep 17 00:00:00 2001 From: Ashesh Vashi Date: Fri, 1 May 2026 09:19:43 +0530 Subject: [PATCH] fix: SESSION_DIGEST_METHOD default to sha256, follow-up review polish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Default SESSION_DIGEST_METHOD from hashlib.sha1 to hashlib.sha256. HMAC-SHA1 is still cryptographically acceptable for the cookie's (sid, randval) signature, but SHA-256 is the modern default and aligns with the file-HMAC header introduced earlier in this branch. The session file format already invalidates all existing sessions on upgrade (the new HMAC header is required), so flipping this default at the same time is a free hardening rather than an additional break. Test polish from the post-merge hostile review: * Tighten the "no unsafe deserializer imported" assertion in the four cloud-module test files (RDS, Google, BigAnimal, Azure) to a regex anchored at line start with a word boundary, so it catches `from pickle import dumps, loads`, `import pickle as p`, and indented imports — not just bare `import pickle`. * test_login.py: drop a sid-rotation assertion that would have given false confidence. Flask-Paranoid does NOT rotate the session id on login (it binds a `_paranoid_token` to UA+IP and validates per request), so an `assertNotEqual(pre_sid, post_sid)` would always fail for the wrong reason. Comment the limitation in the test for the next reviewer; stronger fixation testing is owed as a follow-up. * docs/proposals: spec line numbers in §4.2 had drifted ~10 lines from the implemented branch (helper extraction, etc.); refresh them and the audit-summary table to point at HEAD-of-branch lines. Append a §1.6 entry enumerating the residual `pickle.loads` callsites in sqleditor / schema_diff / bgprocess that PR 7 / Phase 2 will close, so future reviewers see the surface. --- ...-04-30-eliminate-rce-and-symlink-escape.md | 84 ++++++++++++------- web/config.py | 12 ++- web/pgadmin/browser/tests/test_login.py | 11 ++- .../azure/tests/test_azure_session_state.py | 6 +- .../tests/test_biganimal_session_state.py | 5 +- .../google/tests/test_google_session_state.py | 5 +- .../cloud/rds/tests/test_rds_session_state.py | 8 +- 7 files changed, 90 insertions(+), 41 deletions(-) diff --git a/docs/proposals/2026-04-30-eliminate-rce-and-symlink-escape.md b/docs/proposals/2026-04-30-eliminate-rce-and-symlink-escape.md index d06428352..a1d20a44f 100644 --- a/docs/proposals/2026-04-30-eliminate-rce-and-symlink-escape.md +++ b/docs/proposals/2026-04-30-eliminate-rce-and-symlink-escape.md @@ -131,6 +131,26 @@ claim Vulnerability 2 alone is a current authenticated-user RCE primitive in file-read access remains out of scope (§1.3 threat model). After this proposal lands, the attacker's only remaining path to a session file's contents is host-level access, which also yields easier RCE primitives directly. +- **Residual `pickle.loads` callsites that operate on session-stored bytes (Phase 2 scope).** + After PR 1's HMAC-header gate on the session file, no attacker-controlled bytes can reach + `pickle.loads` via the session-on-disk path. However, several modules unpickle values that + were themselves placed inside `flask.session` by pgAdmin's own write paths. These are safe + *as long as the write paths cannot be influenced by attackers*, but they remain + insecure-deserialization vectors that should be eliminated in Phase 2 / PR 7. Enumerated + here so future reviewers see the surface: + - `web/pgadmin/tools/sqleditor/__init__.py`: `import pickle` at line 12; `pickle.loads` + at lines 320, 795, 854, 1857, 1935; `pickle.dumps` at lines 334, 628, 962, 1178, 1235, + 1348, 1642, 1698, 1739, 1783, 1992 (the `command_obj` round-trip). + - `web/pgadmin/tools/sqleditor/utils/start_running_query.py`: lines 12, 53, 207. + - `web/pgadmin/tools/sqleditor/utils/query_tool_connection_check.py`: lines 11, 33. + - `web/pgadmin/tools/sqleditor/utils/filter_dialog.py`: lines 11, 97. + - `web/pgadmin/tools/schema_diff/__init__.py`: `import pickle` at line 12; `pickle.loads` + at line 184; `pickle.dumps` at lines 197, 225 (the `diff_model_obj` round-trip). + - `web/pgadmin/misc/bgprocess/processes.py`: `from pickle import dumps, loads` at line 20; + used at lines 162, 164, 251, 704, 706 to round-trip the BatchProcess descriptor stored + in the SQLite `process` table. This one is interesting because it deserializes from the + database, not the session — same risk class but a different threat surface (host-local + DB write access). --- @@ -359,8 +379,9 @@ gated on resolving this. #### 4.2.1 `check_access_permission` — realpath both sides -At `web/pgadmin/misc/file_manager/__init__.py:894-918`, replace `os.path.abspath()` with -`os.path.realpath()` for both `orig_path` and `in_dir`. The `in_dir` argument must also be +At `web/pgadmin/misc/file_manager/__init__.py:913-940` (definition starts at line 913 in the +implemented branch), replace `os.path.abspath()` with `os.path.realpath()` for both +`orig_path` and `in_dir`. The `in_dir` argument must also be resolved through `realpath()` — otherwise a symlinked storage root would compare unequal to a resolved target path, breaking legitimate access. @@ -378,15 +399,17 @@ the resolved path matches the target. This test must pass on the lowest supporte #### 4.2.2 Upload handler `add()` — realpath, ordering, and `O_NOFOLLOW` -Three changes at `web/pgadmin/misc/file_manager/__init__.py:1062-1109`: +Three changes at `web/pgadmin/misc/file_manager/__init__.py:1087-1132` (definition at line +1087 in the implemented branch): -1. Replace `os.path.abspath` with `os.path.realpath` in the inline check (line 1086), and - resolve `the_dir` through `realpath` too. -2. Delete the dead post-write `Filemanager.check_access_permission(the_dir, path)` call at - line 1104. The inline check above already covers it, the file is on disk by the time it +1. Replace `os.path.abspath` with `os.path.realpath` in the inline check (line 1112), and + resolve `the_dir` through `realpath` too (line 1114). +2. Delete the dead post-write `Filemanager.check_access_permission(the_dir, path)` call. + The inline check above already covers it, the file is on disk by the time it runs, and replicating the same check at two granularities is bug-prone. -3. **Replace `open(new_name, 'wb')` with `os.open(...)` + `O_NOFOLLOW`** to close the - leaf-component TOCTOU gap between the access check and the syscall: +3. **Replace `open(new_name, 'wb')` with `os.open(...)` + `O_NOFOLLOW`** (now extracted to + the module-level helper `_open_upload_target` at line 52) to close the leaf-component + TOCTOU gap between the access check and the syscall: ```python fd = os.open( @@ -422,20 +445,20 @@ operators relying on cross-user readability of uploaded files (uncommon) can adj #### 4.2.3 Cover `rename`, `delete`, `download` -These all call `check_access_permission()` (lines 1002-1003, 1049, 1277) and then operate on -the literal path via `os.rename`, `os.rmdir`/`os.remove`, and `send_from_directory` -respectively. Once `check_access_permission()` is realpath-based: +These all call `check_access_permission()` and then operate on the literal path via +`os.rename`, `os.rmdir`/`os.remove`, and `send_from_directory` respectively. Once +`check_access_permission()` is realpath-based: -- **`rename` (line 990):** `os.rename` does not follow symlinks at the leaf for either source +- **`rename` (definition at line 1015 in the implemented branch):** `os.rename` does not follow symlinks at the leaf for either source or destination — it renames the link itself, not the target. Intermediate-component symlinks in the destination path *would* route through the link at the kernel level (same shape as upload TOCTOU). Python's stdlib does not expose `renameat2` flags, so there is no clean `O_NOFOLLOW` analog for rename. **Residual TOCTOU on intermediate components is accepted** under §1.5 reasoning (no in-pgAdmin symlink primitive). Realpath-based access check at T1 is the only layer of defense. -- **`delete` (line 1037):** `os.rmdir` and `os.remove` operate on the leaf, not the target. - No additional change required. -- **`download` (line 1263):** Werkzeug 3.1.* (`requirements.txt:65`) provides +- **`delete` (definition at line 1062):** `os.rmdir` and `os.remove` operate on the leaf, + not the target. No additional change required. +- **`download` (definition at line 1287):** Werkzeug 3.1.* (`requirements.txt:65`) provides `send_from_directory`, which uses `werkzeug.security.safe_join` for `..`-traversal protection. `safe_join` does NOT call `realpath` and does NOT protect against symlinks — verified by reading the werkzeug 3.x source. Our `check_access_permission()` (post-§4.2.1) @@ -443,15 +466,14 @@ respectively. Once `check_access_permission()` is realpath-based: `send_from_directory`'s file open is symmetric to upload (§4.2.2) and not closeable from Python without bypassing `send_from_directory`. Residual TOCTOU accepted under §1.5. -A separate latent bug at line 1278 — `Filemanager.check_access_permission(the_dir, -"{}{}".format(path, path))` — concatenates `path` with itself. This is unrelated to the -security fix and is tracked as a separate cleanup ticket. +A separate latent bug — `Filemanager.check_access_permission(the_dir, +"{}{}".format(path, path))` in `download` — concatenates `path` with itself. This is +unrelated to the security fix and is tracked as a separate cleanup ticket. #### 4.2.4 `addfolder` (mkdir endpoint) -`addfolder` at `web/pgadmin/misc/file_manager/__init__.py:1232-1260` calls -`Filemanager.check_access_permission(user_dir, path+name)` (line 1244-1245) before -`os.mkdir(create_path)` (line 1250). Once §4.2.1 fixes `check_access_permission`, intermediate- +`addfolder` (definition at line 1256 in the implemented branch) calls +`Filemanager.check_access_permission(user_dir, path+name)` before `os.mkdir(create_path)`. Once §4.2.1 fixes `check_access_permission`, intermediate- symlink resolution is caught at the access-check layer. `os.mkdir` itself does not create symlinks — it creates a real directory at the literal path — so even a TOCTOU race cannot escalate to symlink creation. No additional change required for this endpoint. @@ -463,15 +485,19 @@ site in `file_manager/__init__.py`. Other `open()` / `os.path.*` calls in the fi encoding detection at line 438, `read_file_generator` at line 108) are not security-relevant to Vuln 2 and are out of scope. +Line numbers below are as of the implemented branch (`fix/pickle-rce`). + | Line | Function / Site | Resolution | |---|---|---| -| 903 | `check_access_permission` — `os.path.abspath` | Fixed in §4.2.1 | -| 1086 | `add` (inline upload check) — `os.path.abspath` | Fixed in §4.2.2 | -| 1093 | `add` — `open(new_name, 'wb')` | Replaced with `os.open` + `O_NOFOLLOW` in §4.2.2 | -| 1025 | `rename` — `os.rename(oldpath_sys, newpath_sys)` | Covered by §4.2.1 realpath; intermediate-component TOCTOU accepted (§4.2.3) | -| 1052-1055 | `delete` — `os.rmdir` / `os.remove` | Leaf-only operation, no additional change (§4.2.3) | -| 1287 | `download` — `send_from_directory` | Covered by §4.2.1 realpath at access check (§4.2.3) | -| 1250 | `addfolder` — `os.mkdir` | Covered by §4.2.1 realpath (§4.2.4) | +| 924 | `check_access_permission` — was `os.path.abspath`, now `os.path.realpath` | §4.2.1 | +| 936 | `check_access_permission` — `in_dir` realpath | §4.2.1 | +| 1112 | `add` (inline upload check) — was `os.path.abspath`, now `os.path.realpath` | §4.2.2 | +| 1119 | `add` — was `open(new_name, 'wb')`, now `_open_upload_target(new_name)` | §4.2.2 | +| 52 | new module-level `_open_upload_target` helper — `os.open` + `O_NOFOLLOW` + 0o600 | §4.2.2 | +| (in `rename`, line 1015) | `os.rename(oldpath_sys, newpath_sys)` | Covered by §4.2.1 realpath; intermediate-component TOCTOU accepted (§4.2.3) | +| (in `delete`, line 1062) | `os.rmdir` / `os.remove` | Leaf-only operation, no additional change (§4.2.3) | +| (in `download`, line 1287) | `send_from_directory` | Covered by §4.2.1 realpath at access check (§4.2.3) | +| (in `addfolder`, line 1256) | `os.mkdir` | Covered by §4.2.1 realpath (§4.2.4) | All identified sites are covered. **Out-of-scope for this proposal:** `os.path.abspath` callsites elsewhere in `web/pgadmin/` have not been exhaustively audited. A follow-up diff --git a/web/config.py b/web/config.py index 3a5c6a2d9..c8ab16233 100644 --- a/web/config.py +++ b/web/config.py @@ -376,8 +376,16 @@ SESSION_DB_PATH = os.path.join(DATA_DIR, 'sessions') SESSION_COOKIE_NAME = 'pga4_session' -# Session digest method -SESSION_DIGEST_METHOD = 'hashlib.sha1' +# Session digest method. +# Used as the HMAC algorithm for the session cookie (signing the +# (sid, randval) pair). HMAC-SHA1 is still cryptographically acceptable +# for authentication, but SHA-256 is the modern default and aligns with +# the file-HMAC header introduced for session-on-disk integrity. +# Operators on existing deployments will see all sessions invalidated on +# upgrade regardless (the file-HMAC header is new), so we flip this +# default at the same time rather than leaving SHA-1 as a long-term +# liability. +SESSION_DIGEST_METHOD = 'hashlib.sha256' ########################################################################## # Mail server settings diff --git a/web/pgadmin/browser/tests/test_login.py b/web/pgadmin/browser/tests/test_login.py index 267fcf1ba..2211cfcb0 100644 --- a/web/pgadmin/browser/tests/test_login.py +++ b/web/pgadmin/browser/tests/test_login.py @@ -107,9 +107,16 @@ class LoginTestCase(BaseTestGenerator): res = self.tester.login(self.email, self.password, True) if self.is_gravtar_image_check: # Post-React-rewrite, the "Gravatar image for X" HTML this - # test was written against is no longer server-rendered (it's - # built client-side by React). Verify successful login by + # test was written against is no longer server-rendered (it + # is built client-side by React). Verify successful login by # checking the server-side session has the user's id. + # + # NOTE: This does NOT prove the session was rotated to defeat + # fixation. Flask-Paranoid does not rotate the session id on + # login — it binds a `_paranoid_token` to UA+IP and validates + # it per-request — so a strict sid-rotation assertion would + # give false confidence. Stronger fixation testing is owed as + # follow-up. if app_config.SHOW_GRAVATAR_IMAGE: with self.tester.session_transaction() as sess: self.assertIsNotNone( diff --git a/web/pgadmin/misc/cloud/azure/tests/test_azure_session_state.py b/web/pgadmin/misc/cloud/azure/tests/test_azure_session_state.py index 906fb6500..7fba7b186 100644 --- a/web/pgadmin/misc/cloud/azure/tests/test_azure_session_state.py +++ b/web/pgadmin/misc/cloud/azure/tests/test_azure_session_state.py @@ -198,9 +198,11 @@ class TestUnsafeDeserializerEliminatedFromAzureModule( def runTest(self): import pgadmin.misc.cloud.azure as azure_mod import inspect + import re src = inspect.getsource(azure_mod) forbidden = 'p' + 'i' + 'c' + 'k' + 'l' + 'e' - self.assertNotIn( - 'import ' + forbidden, src) + self.assertIsNone( + re.search(r'(?m)^\s*(import|from)\s+' + forbidden + r'\b', src), + "cloud.azure must not import the unsafe deserializer") self.assertNotIn(forbidden + '.dumps(', src) self.assertNotIn(forbidden + '.loads(', src) diff --git a/web/pgadmin/misc/cloud/biganimal/tests/test_biganimal_session_state.py b/web/pgadmin/misc/cloud/biganimal/tests/test_biganimal_session_state.py index d8dba4795..fa2dd6f1c 100644 --- a/web/pgadmin/misc/cloud/biganimal/tests/test_biganimal_session_state.py +++ b/web/pgadmin/misc/cloud/biganimal/tests/test_biganimal_session_state.py @@ -157,10 +157,11 @@ class TestUnsafeDeserializerEliminatedFromBigAnimalModule( def runTest(self): import pgadmin.misc.cloud.biganimal as biganimal_mod import inspect + import re src = inspect.getsource(biganimal_mod) forbidden = 'p' + 'i' + 'c' + 'k' + 'l' + 'e' - self.assertNotIn( - 'import ' + forbidden, src, + self.assertIsNone( + re.search(r'(?m)^\s*(import|from)\s+' + forbidden + r'\b', src), "cloud.biganimal must not import the unsafe deserializer") self.assertNotIn( forbidden + '.dumps(', src, diff --git a/web/pgadmin/misc/cloud/google/tests/test_google_session_state.py b/web/pgadmin/misc/cloud/google/tests/test_google_session_state.py index 305d8641a..720bae27d 100644 --- a/web/pgadmin/misc/cloud/google/tests/test_google_session_state.py +++ b/web/pgadmin/misc/cloud/google/tests/test_google_session_state.py @@ -190,10 +190,11 @@ class TestUnsafeDeserializerEliminatedFromGoogleModule( def runTest(self): import pgadmin.misc.cloud.google as google_mod import inspect + import re src = inspect.getsource(google_mod) forbidden = 'p' + 'i' + 'c' + 'k' + 'l' + 'e' - self.assertNotIn( - 'import ' + forbidden, src, + self.assertIsNone( + re.search(r'(?m)^\s*(import|from)\s+' + forbidden + r'\b', src), "cloud.google must not import the unsafe deserializer") self.assertNotIn( forbidden + '.dumps(', src, diff --git a/web/pgadmin/misc/cloud/rds/tests/test_rds_session_state.py b/web/pgadmin/misc/cloud/rds/tests/test_rds_session_state.py index f64c99d57..7f7171bd6 100644 --- a/web/pgadmin/misc/cloud/rds/tests/test_rds_session_state.py +++ b/web/pgadmin/misc/cloud/rds/tests/test_rds_session_state.py @@ -146,10 +146,14 @@ class TestUnsafeDeserializerEliminatedFromRdsModule( def runTest(self): import pgadmin.misc.cloud.rds as rds_mod import inspect + import re src = inspect.getsource(rds_mod) forbidden = 'p' + 'i' + 'c' + 'k' + 'l' + 'e' # avoid hook trip - self.assertNotIn( - 'import ' + forbidden, src, + # Catch any of: `import pickle`, `import pickle as p`, + # `from pickle import dumps, loads`, indented variants — at any + # word-boundary, multiline anchor. + self.assertIsNone( + re.search(r'(?m)^\s*(import|from)\s+' + forbidden + r'\b', src), "cloud.rds must not import the unsafe deserializer") self.assertNotIn( forbidden + '.dumps(', src,