mirror of
https://github.com/pgadmin-org/pgadmin4.git
synced 2026-11-01 07:48:03 -06:00
fix: pickle deserialization RCE in session manager (CWE-502) (#9901)
CWE-502 in FileBackedSessionManager.get(): pickle.load was called on the session file before the HMAC integrity check. Any file dropped in the sessions directory was deserialized unconditionally, allowing an authenticated user with sessions-dir write access to achieve OS-level RCE. Fix: prepend a 64-byte hex SHA-256 HMAC header over the pickle body, computed with the SECRET_KEY, and verify it via hmac.compare_digest before any deserialization. Also raises (not asserts) on empty SECRET_KEY so -O does not strip the check, and narrows the post-deserialize except clause to surface programming errors instead of masking them. Tests: 13 new session-format tests covering round-trip, malicious- pickle rejection, header tampering, empty/truncated files, cookie-HMAC mismatch, unsafe sids, empty SECRET_KEY, MFA-shaped data, and SERVER_MODE=False direct upload. Includes docs/proposals/2026-04-30-eliminate-rce-and-symlink-escape.md, the shared design doc covering this fix and the symlink-traversal fix in the following commit. Reported-by: Fernando Bortotti <fernando.bortotti@bsd.com.br>
This commit is contained in:
Ashesh Vashi committed
1 parent
24485fe964
commit
30a8903374
3 files changed
+1397
-19
No files matched your search
@@ -0,0 +1,846 @@
|
||||
# Proposal: Eliminate Insecure Deserialization RCE and Symlink-based Path Traversal
|
||||
|
||||
**Status:** Draft (v5 — addresses review findings of 2026-04-30, including end-to-end coherence pass)
|
||||
|
||||
> **Note on line numbers:** Citations in this proposal are accurate as of `HEAD` of
|
||||
> `fix/pickle-rce` at v5 publication time. If upstream changes land in `session.py` or
|
||||
> `file_manager/__init__.py` before PR 1 merges, the implementing PR will recompute line
|
||||
> references against the rebase base.
|
||||
**Date:** 2026-04-30
|
||||
**Author:** Ashesh Vashi
|
||||
**Tracking branch:** `fix/pickle-rce`
|
||||
**Related:** Independent security report dated 2026-04-29; CVE-2024-2044 (prior RCE in
|
||||
SERVER_MODE=False)
|
||||
|
||||
---
|
||||
|
||||
## 1. Problem Statement
|
||||
|
||||
A security report dated 2026-04-29 identified two complementary vulnerabilities in pgAdmin 4
|
||||
(confirmed on v9.14) that, when combined, allow an authenticated user to achieve OS-level Remote
|
||||
Code Execution on the pgAdmin host or container.
|
||||
|
||||
### 1.1 Vulnerability 1 — Insecure Deserialization Before HMAC Verification (CWE-502)
|
||||
|
||||
**Location:** `web/pgadmin/utils/session.py:240-267` — `FileBackedSessionManager.get()`
|
||||
|
||||
The session manager deserializes the session file with `pickle.load()` **before** verifying the
|
||||
HMAC integrity signature stored alongside it. Any file an attacker can place in the sessions
|
||||
directory is therefore executed unconditionally — `pickle.load()` fires arbitrary
|
||||
`__reduce__` hooks during reads, before any of the integrity checks run.
|
||||
|
||||
The vulnerable sequence:
|
||||
|
||||
1. `open(fname, 'rb')` and read all bytes.
|
||||
2. `pickle.load(f)` — RCE here.
|
||||
3. Compare the in-body HMAC against the cookie HMAC — too late.
|
||||
|
||||
### 1.2 Vulnerability 2 — Symlink Escape in the File Manager (CWE-61 / CWE-22)
|
||||
|
||||
**Location:** `web/pgadmin/misc/file_manager/__init__.py` — `check_access_permission()` at
|
||||
line 894 and the upload handler `add()` at line 1062.
|
||||
|
||||
Both code paths use `os.path.abspath()`, which resolves `.` and `..` but does **not** resolve
|
||||
symbolic links. The subsequent `open(path, 'wb')` is performed by the OS kernel, which **does**
|
||||
follow symlinks. If a symlink exists inside the user's storage directory pointing outside the
|
||||
sandbox, the access check (path-string semantics) passes while the kernel write
|
||||
(filesystem semantics) follows the symlink.
|
||||
|
||||
The same path-string-vs-kernel mismatch applies to `rename()` (line 990), `delete()` (line
|
||||
1037), and `download()` (line 1263), which all call `check_access_permission()` and then
|
||||
operate on the literal path via syscall.
|
||||
|
||||
A separate timing bug in `add()`: the inline access check runs *before* `open()` (good) but
|
||||
`Filemanager.check_access_permission()` is *also* called *after* the file write at line 1104 —
|
||||
pointless, because the file is already on disk by then.
|
||||
|
||||
### 1.3 Threat Model
|
||||
|
||||
- Attacker has a valid pgAdmin login (any role suffices — no admin required).
|
||||
- Attacker can reach the file manager and session cookie surfaces.
|
||||
- Goal: OS-level command execution as the user running gunicorn/pgAdmin (root in the official
|
||||
Docker image).
|
||||
- Out of scope: attackers who already have host shell or Docker volume write access; supply-chain
|
||||
compromise.
|
||||
|
||||
### 1.4 Severity
|
||||
|
||||
CVSS v3.1 base score (per the report): **8.8 (High)**.
|
||||
|
||||
| Attribute | Value |
|
||||
|---|---|
|
||||
| Attack vector | Network |
|
||||
| Privileges required | Low (any authenticated user) |
|
||||
| User interaction | None |
|
||||
| Confidentiality / Integrity / Availability | High / High / High |
|
||||
|
||||
### 1.5 Attack chain feasibility — what the report does and does not prove
|
||||
|
||||
The report demonstrates two scenarios. Both rely on a primitive the report **does not prove
|
||||
exists inside pgAdmin**:
|
||||
|
||||
- **Scenario A (SERVER_MODE=False):** in this mode `check_access_permission()` returns
|
||||
immediately without any check (see line 895). An authenticated user can directly upload a
|
||||
payload to the sessions directory via the file manager. No symlink needed. **This is a
|
||||
complete in-pgAdmin attack chain when `SERVER_MODE=False`** — and it's the likely vector for
|
||||
the prior CVE-2024-2044.
|
||||
- **Scenario B (SERVER_MODE=True):** requires a symlink to be planted inside the user's
|
||||
storage directory. The report says "create a symlink" but does not describe how to do this
|
||||
through pgAdmin alone.
|
||||
|
||||
A grep over the **entire `web/pgadmin/` tree** (not just file_manager) for symlink-creating
|
||||
primitives returns zero matches across all of:
|
||||
|
||||
| Audit pattern | Matches in `web/pgadmin/` |
|
||||
|---|---|
|
||||
| `os.symlink` / `os.link(` / `symbolic_link` / `symlink_to` / `Path.symlink` / `.symlink(` | 0 |
|
||||
| `tarfile` / `zipfile` / `unpack_archive` / `extractall` / `extract_archive` / `shutil.copytree` (archive extraction can preserve symlinks from a malicious archive) | 0 |
|
||||
| `subprocess` / `os.popen` / shell-invocation primitives in `file_manager/` | 0 |
|
||||
|
||||
The download endpoint serves files; the upload endpoint writes file *bodies*; no
|
||||
rename/move/extract handler creates links. **Therefore Scenario B is only exploitable when the
|
||||
attacker has an out-of-band way to plant a symlink** — host shell, Docker volume mount with
|
||||
shared storage, container escape, or a future pgAdmin feature that introduces a symlink
|
||||
primitive (e.g., archive extraction). This audit excludes the bundled
|
||||
`docker-entrypoint.sh`/runtime startup scripts, which run before user authentication and are
|
||||
not part of the user-facing attack surface.
|
||||
|
||||
We close Vulnerability 2 anyway, as defense in depth: (a) prevents future regressions if such a
|
||||
primitive is introduced, (b) closes the path for deployments where the storage directory is
|
||||
shared with another writer, (c) the change is small and low-risk. But the spec does **not**
|
||||
claim Vulnerability 2 alone is a current authenticated-user RCE primitive in
|
||||
`SERVER_MODE=True`.
|
||||
|
||||
### 1.6 Known residual issues not introduced or fixed by this proposal
|
||||
|
||||
- **Session-file replay after logout.** A user who saves a copy of their valid, HMAC'd session
|
||||
file and the matching cookie can `cp` the file back into the sessions directory after
|
||||
logout-driven deletion, regaining their session. The current code has this property; the
|
||||
Phase 1 design preserves it. Closing it requires a per-session-revision nonce that's
|
||||
invalidated server-side on logout — out of scope for this proposal, tracked as a follow-up.
|
||||
- **Read-side information disclosure via symlink.** `download()` at line 1263 calls
|
||||
`check_access_permission()` and `send_from_directory()`. Once `check_access_permission()`
|
||||
uses `realpath()`, symlink-based reads are also blocked at the access-check layer. The
|
||||
underlying TOCTOU caveat (§4.2.3) applies symmetrically to read.
|
||||
- **CSRF-token theft via session-file read (Scenario B step 3 of the report).** The report's
|
||||
Scenario B requires the attacker to read the victim's session file from disk to extract the
|
||||
raw `csrf_token`. This requires file-read access to the sessions directory — either via the
|
||||
same out-of-band primitive needed to plant a symlink (host shell, Docker volume mount), or
|
||||
via the file manager's `download()` endpoint pointed at the sessions directory through a
|
||||
symlink. The realpath fix in §4.2.1 closes the symlink-based read path. Out-of-band
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## 2. Goals
|
||||
|
||||
1. **Close both vulnerabilities** in shippable, independently reviewable units of work.
|
||||
2. **Eliminate `pickle` from the pgAdmin web codebase entirely** as a long-term defence-in-depth
|
||||
measure, not just from `session.py`. The same module is also used inside the cloud provider
|
||||
handlers and the SQL editor's grid state.
|
||||
3. **No regressions** in: login (internal, OAuth2, LDAP, Kerberos, Webserver, MFA), cloud
|
||||
deployment wizards (RDS, Azure, Google, BigAnimal), Query Tool, Schema Diff, Debugger.
|
||||
4. **No silent data loss** for active sessions on upgrade where avoidable. Sessions written by
|
||||
pre-fix code will be invalidated (one-time re-login). Operators are alerted via a single
|
||||
warning log line per rejected sid (see §4.3).
|
||||
|
||||
## 3. Non-goals
|
||||
|
||||
- Changing the cookie format or HMAC algorithm of the cookie.
|
||||
- Changing the session storage backend (still file-backed with HMAC).
|
||||
- Refactoring the `Filemanager` class beyond what's needed for the symlink fix.
|
||||
- Deprecating `SERVER_MODE=False` (orthogonal concern).
|
||||
- Closing session-file replay-after-logout (§1.6).
|
||||
|
||||
---
|
||||
|
||||
## 4. Design
|
||||
|
||||
### 4.1 Vulnerability 1 — Two-phase fix
|
||||
|
||||
#### Phase 1 (urgent): HMAC-first verification with a plaintext header
|
||||
|
||||
`pickle` stays as the on-disk format **temporarily**, but is gated behind an HMAC verified from
|
||||
a plaintext header that lives outside the serialized blob.
|
||||
|
||||
**File format (new):**
|
||||
|
||||
```
|
||||
+----------------------------------------------------+
|
||||
| H bytes : hex HMAC over the body, ASCII-encoded |
|
||||
+----------------------------------------------------+
|
||||
| N bytes : pickle body — (randval, digest, data) |
|
||||
+----------------------------------------------------+
|
||||
```
|
||||
|
||||
`H` is determined by the chosen digest algorithm: `len(hmac.new(...).hexdigest()) ==
|
||||
2 * digest_size`. For SHA-256, `H = 64`. We use **SHA-256** for the file HMAC regardless of
|
||||
`config.SESSION_DIGEST_METHOD` (which defaults to SHA-1 — see §8 Open Question 1). The HMAC is
|
||||
computed with `app.config['SECRET_KEY']` over the body bytes.
|
||||
|
||||
**Read path (new):**
|
||||
|
||||
```python
|
||||
import hmac
|
||||
import hashlib
|
||||
import pickle
|
||||
import logging
|
||||
|
||||
_FILE_HMAC = hashlib.sha256
|
||||
_HMAC_HEX_LEN = _FILE_HMAC().digest_size * 2 # 64 for SHA-256
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
def _compute_file_hmac(secret, body: bytes) -> bytes:
|
||||
# Flask's SECRET_KEY may be str or bytes; normalize before HMAC.
|
||||
key = secret.encode() if isinstance(secret, str) else secret
|
||||
return hmac.new(key, body, _FILE_HMAC).hexdigest().encode()
|
||||
|
||||
def get(self, sid, digest):
|
||||
fname = safe_join(self.path, sid)
|
||||
if fname is None or not os.path.exists(fname):
|
||||
return self.new_session()
|
||||
try:
|
||||
with open(fname, 'rb') as f:
|
||||
header = f.read(_HMAC_HEX_LEN)
|
||||
body = f.read()
|
||||
if len(header) != _HMAC_HEX_LEN or len(body) == 0:
|
||||
# 0-byte file from new_session(), or pre-fix session file. Silently
|
||||
# discard; Phase 1 deployment expects one-time re-login churn.
|
||||
return self.new_session()
|
||||
expected = _compute_file_hmac(self.secret, body)
|
||||
if not hmac.compare_digest(header, expected):
|
||||
logger.warning(
|
||||
"session file rejected: bad file-HMAC for sid=%s", sid[:8]
|
||||
)
|
||||
return self.new_session()
|
||||
randval, hmac_digest, data = pickle.loads(body) # safe: body is verified
|
||||
except (pickle.UnpicklingError, EOFError, OSError, MemoryError, ValueError, TypeError):
|
||||
# Narrow catch: silently invalidate corrupted/unreadable session files.
|
||||
# Programming errors (AttributeError, NameError, etc.) propagate intentionally
|
||||
# so they show up in tests and logs rather than being masked as new_session().
|
||||
return self.new_session()
|
||||
if hmac_digest != digest:
|
||||
return self.new_session()
|
||||
return ManagedSession(data, sid=sid, randval=randval, hmac_digest=hmac_digest)
|
||||
```
|
||||
|
||||
**Write path (new):**
|
||||
|
||||
```python
|
||||
def put(self, session):
|
||||
...
|
||||
# The existing disk_write_delay and skip_paths skip logic at lines 271-285
|
||||
# is preserved verbatim — only the format step below changes.
|
||||
body = pickle.dumps(
|
||||
(session.randval, session.hmac_digest, dict(session)), -1
|
||||
)
|
||||
header = _compute_file_hmac(self.secret, body)
|
||||
fname = safe_join(self.path, session.sid)
|
||||
with open(fname, 'wb') as f:
|
||||
f.write(header)
|
||||
f.write(body)
|
||||
```
|
||||
|
||||
**Defensive precondition (`__init__`):** raise on empty `SECRET_KEY`:
|
||||
|
||||
```python
|
||||
if not self.secret:
|
||||
raise RuntimeError("SECRET_KEY must be non-empty")
|
||||
```
|
||||
|
||||
`assert` is **not** acceptable here — Python's `-O` flag strips assertions, which would silently
|
||||
disable the security-critical check in optimized production builds. Pre-fix code shares the
|
||||
implicit non-empty assumption, but an empty `SECRET_KEY` reduces the file-HMAC to a known-key
|
||||
MAC, and a security-driven redesign is the right time to fail fast.
|
||||
|
||||
**Performance.** HMAC-SHA256 over typical session bodies (<10 KB) costs <100 µs per request on
|
||||
modern x86-64. Sessions exceeding 1 MB (rare; large `gridData` with cloud-handler bytes blobs)
|
||||
will see sub-millisecond overhead. No measurable production impact expected. Confirmed by
|
||||
inspection — the read path was already `f.read()`-bound on full file bytes, so the additional
|
||||
work is one `hmac.new(...).hexdigest()` per request.
|
||||
|
||||
**Important note on the report's Fix 1.** The report's suggested "HMAC-first" code calls
|
||||
`pickle.loads(raw)` once just to read the stored HMAC — which is the exact line that fires
|
||||
the RCE. The plaintext-header design above avoids that: the HMAC is verified against raw bytes
|
||||
before any deserialization.
|
||||
|
||||
**Migration:** files written by the old code (no header) fail the header check and are silently
|
||||
discarded — affected sessions get a fresh `new_session()`. One-time re-login on upgrade.
|
||||
Operators see a transient burst of `session file rejected` warnings during rollout, then they
|
||||
quiesce.
|
||||
|
||||
**Empty-file behavior.** `new_session()` at line 218-238 creates a 0-byte placeholder. The next
|
||||
`get()` reads short (< 64 bytes) header + empty body, falls through to `new_session()`.
|
||||
Subsequent `put()` writes the full header + body. This matches the existing behavior of the old
|
||||
code (where `pickle.load` on 0 bytes raised, was caught, and fell through to `new_session()`).
|
||||
No regression.
|
||||
|
||||
**Concurrent `put()` from multiple workers.** Two workers writing the same sid race on the
|
||||
file. `open(fname, 'wb')` truncates atomically; the worst case is interleaved writes producing
|
||||
a header/body mismatch. The next `get()` recomputes the file-HMAC, fails verification, logs
|
||||
the warning, and returns `new_session()`. **Race-tolerant by design** — partial writes can
|
||||
never produce an executable payload because they cannot satisfy the HMAC. The pre-fix code
|
||||
shared the same race shape but with `pickle.load` instead of HMAC validation; not a
|
||||
regression.
|
||||
|
||||
**Caching layer.** `CachingSessionManager` (session.py:104-196) wraps
|
||||
`FileBackedSessionManager`. Cache hits use the cached `ManagedSession.hmac_digest` for the
|
||||
cookie compare; cache misses go through `parent.get()`, which performs the new file-HMAC
|
||||
check. The cache is only populated from a verified disk read (or from `new_session()`), so the
|
||||
file-HMAC check is enforced **at every cache miss** — including LRU evictions (`_normalize`
|
||||
trims to 80% capacity once size exceeds `num_to_store=1000`) and writes (`put()` invalidates
|
||||
the cached entry). No additional change needed here.
|
||||
|
||||
#### Phase 2 (defence in depth): switch to plain JSON
|
||||
|
||||
After Phase 1 ships, refactor every site that stashes a Python object or `pickle.dumps(...)`
|
||||
bytes inside `flask.session` so that the session contains only data primitives. Once the
|
||||
session contents are pure JSON-safe types, the on-disk format switches from pickle to plain
|
||||
JSON, and `pickle` is removed from the web codebase entirely.
|
||||
|
||||
The session's purpose is to hold **identity and state pointers**, not live SDK clients. The
|
||||
existing `__pgsql_server_managers` pattern (`web/pgadmin/utils/driver/psycopg3/server_manager.py:519-530` —
|
||||
stores `as_dict()` in session, keeps live `ServerManager` instances in module-global state) is
|
||||
the template.
|
||||
|
||||
**Per-site refactor (verified by grep):**
|
||||
|
||||
| Site (file:line) | Current | Replacement |
|
||||
|---|---|---|
|
||||
| `web/pgadmin/authenticate/__init__.py:113` | `session['auth_obj'] = AuthSourceManager(...)` (live object) | Store source name; reconstruct `AuthSourceManager` in OAuth2 callback (`oauth2.py:55`) |
|
||||
| `web/pgadmin/misc/cloud/azure/__init__.py:89` | `session['azure']['azure_obj'] = Azure(...)` (live object) | Store `authentication_record_json` + tenant/auth_type/cache_name; rebuild `Azure` per request via process-local cache keyed by sid. **See §5.4 for an unverified-claims caveat.** |
|
||||
| `web/pgadmin/misc/cloud/rds/__init__.py:73` | `session['aws']['aws_rds_obj'] = pickle.dumps(_rds, -1)` | Store credentials dict; rebuild `RDS` from process-local cache. 1 write, 2 reads (lines 73, 100, 129). |
|
||||
| `web/pgadmin/misc/cloud/google/__init__.py:110` | `session['google']['google_obj'] = pickle.dumps(...)` (4 write sites: 110, 129, 145, 181) | Store credentials/state dict; rebuild `Google` from process-local cache. |
|
||||
| `web/pgadmin/misc/cloud/biganimal/__init__.py:73` | `session['biganimal']['provider_obj'] = pickle.dumps(...)` (4 write sites: 60, 73, 95, 106) | Same pattern. |
|
||||
| `web/pgadmin/tools/sqleditor/__init__.py` (lines 338, 632, 699, 740 write `session['gridData']`; the per-trans `command_obj` field holds `pickle.dumps(transaction, -1)` produced in `tools/sqleditor/utils/start_running_query.py:204-208`, where the local `session` parameter is later persisted via `update_session_grid_transaction`) | serialized `Command` bytes inside a session-resident dict | Store command kind + identifying inputs; rebuild `Command` via `from_state()`; cache PG metadata in process-local TTL cache. **High risk — see §5.4.** |
|
||||
|
||||
**Process-local cache pattern (illustrative):**
|
||||
|
||||
```python
|
||||
# Module-level
|
||||
_azure_cache: dict[str, tuple[float, Azure]] = {}
|
||||
_AZURE_CACHE_TTL = 600 # seconds
|
||||
|
||||
def get_azure_for_session(sid: str) -> Azure:
|
||||
now = time.monotonic()
|
||||
cached = _azure_cache.get(sid)
|
||||
if cached and (now - cached[0]) < _AZURE_CACHE_TTL:
|
||||
return cached[1]
|
||||
state = session['azure'] # JSON-safe dict only
|
||||
obj = Azure.from_state(state) # new constructor
|
||||
_azure_cache[sid] = (now, obj)
|
||||
return obj
|
||||
```
|
||||
|
||||
Eviction: lazy on read (TTL check) and on logout (`_azure_cache.pop(sid, None)`).
|
||||
|
||||
**Worker-restart UX trade-off (Azure).** Today's behavior pickles the live `Azure` (with its
|
||||
populated SDK client cache) into the session file, so a worker restart preserves Azure auth
|
||||
state. After Phase 2, the in-memory client cache is gone on restart. Whether the user is
|
||||
re-prompted for device-code on the next request depends on whether
|
||||
`authentication_record_json` is sufficient to silently rebuild the credential. **This is
|
||||
unverified** (see §5.4) and must be confirmed during the Azure refactor PR before merge. If
|
||||
re-prompting is unavoidable, that is a UX regression — Phase 1 ships independently and is not
|
||||
gated on resolving this.
|
||||
|
||||
#### Final state
|
||||
|
||||
- `web/pgadmin/utils/session.py`: `from json import dumps, loads` replaces `from pickle import
|
||||
dump, load`. HMAC header retained.
|
||||
- All `import pickle` and `from pickle import ...` lines in the web tree removed (except in
|
||||
test fixtures, by deliberate exception).
|
||||
- CI guard (proposed): `grep -rEn '^\s*(import pickle\b|from pickle\b)' web/pgadmin/ | grep -v
|
||||
'/tests/'` must return empty.
|
||||
|
||||
### 4.2 Vulnerability 2 — `realpath`, syscall hardening, and ordering fix
|
||||
|
||||
#### 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
|
||||
resolved through `realpath()` — otherwise a symlinked storage root would compare unequal to a
|
||||
resolved target path, breaking legitimate access.
|
||||
|
||||
Python 3.9+ semantics (pgAdmin's documented floor — see `web/pgAdmin4.wsgi:14` and
|
||||
`README.md:23`): `os.path.realpath(path)` resolves symlinks for components that exist and
|
||||
leaves not-yet-existing tail components as literal text. This is the correct semantics for
|
||||
upload paths.
|
||||
|
||||
**Windows behavior is believed correct in Python 3.9+ but unverified by this proposal.**
|
||||
CPython release notes indicate `ntpath.realpath` resolves NTFS junctions and symbolic links via
|
||||
`GetFinalPathNameByHandle`, with several bug fixes landed across 3.10/3.11. Verification on
|
||||
Windows is a **PR-1 merge gate**: a smoke test creates a junction, calls `realpath`, and asserts
|
||||
the resolved path matches the target. This test must pass on the lowest supported Python
|
||||
(3.9.x) before PR 1 merges.
|
||||
|
||||
#### 4.2.2 Upload handler `add()` — realpath, ordering, and `O_NOFOLLOW`
|
||||
|
||||
Three changes at `web/pgadmin/misc/file_manager/__init__.py:1062-1109`:
|
||||
|
||||
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
|
||||
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:
|
||||
|
||||
```python
|
||||
fd = os.open(
|
||||
new_name,
|
||||
os.O_WRONLY | os.O_CREAT | os.O_TRUNC
|
||||
| getattr(os, 'O_NOFOLLOW', 0),
|
||||
0o600,
|
||||
)
|
||||
with os.fdopen(fd, 'wb') as f:
|
||||
while True:
|
||||
data = file_obj.read(4194304)
|
||||
if not data:
|
||||
break
|
||||
f.write(data)
|
||||
```
|
||||
|
||||
`O_NOFOLLOW` causes the kernel to refuse to open the path if its **final** component is a
|
||||
symlink. Combined with `realpath`-based directory resolution at check time, this closes the
|
||||
**leaf-component race** where another worker plants a symlink at the leaf between check and
|
||||
open. **Intermediate-component races remain unmitigated** and rely on §1.5 reasoning (no
|
||||
in-pgAdmin primitive can plant the directory symlinks needed to exploit them).
|
||||
|
||||
Windows: `os.O_NOFOLLOW` is not defined on Windows. The `getattr(..., 0)` fallback above makes
|
||||
the flag a no-op on Windows. This leaves a small residual leaf-component TOCTOU on Windows,
|
||||
mitigated by the absence of the symlink-creation primitive.
|
||||
|
||||
**Intentional mode change (0o644 → 0o600).** The pseudocode passes mode `0o600` to `os.open`,
|
||||
restricting uploaded files to owner-only. The current code uses `open(name, 'wb')` which
|
||||
applies the umask-default (typically `0o644`, world-readable). The change is intentional
|
||||
hardening — uploaded files are user-private session artifacts and have no need for
|
||||
world-read access — but it **is** a behavioral change. Release notes must call this out so
|
||||
operators relying on cross-user readability of uploaded files (uncommon) can adjust.
|
||||
|
||||
#### 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:
|
||||
|
||||
- **`rename` (line 990):** `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
|
||||
`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)
|
||||
is therefore the **sole** symlink defense for downloads. TOCTOU between check and
|
||||
`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.
|
||||
|
||||
#### 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-
|
||||
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.
|
||||
|
||||
#### 4.2.5 Audit — sites covered by the Vulnerability 2 fix
|
||||
|
||||
This table enumerates the sites involved in the symlink-escape fix, not every path-handling
|
||||
site in `file_manager/__init__.py`. Other `open()` / `os.path.*` calls in the file (e.g., file
|
||||
encoding detection at line 438, `read_file_generator` at line 108) are not security-relevant
|
||||
to Vuln 2 and are out of scope.
|
||||
|
||||
| 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) |
|
||||
|
||||
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
|
||||
security review is in scope for tracking, but is not gated on this proposal.
|
||||
|
||||
### 4.3 Tests
|
||||
|
||||
#### 4.3.1 New regression tests
|
||||
|
||||
1. **Unsafe-payload session rejection (Vuln 1):** drop a hand-crafted file in the sessions
|
||||
directory containing a `pickle` payload whose `__reduce__` writes a sentinel file. Issue a
|
||||
request with that sid in the cookie. Assert (a) the sentinel is **not** created, (b) the
|
||||
request gets a fresh session, (c) a single `session file rejected: bad file-HMAC`
|
||||
warning is logged.
|
||||
2. **Header-tampered session rejection:** write a session with a corrupted HMAC header (flip
|
||||
one byte). Assert `get()` returns a fresh session, no exception leaks, warning logged.
|
||||
3. **Empty-file passthrough:** create a 0-byte file at `sessions/<sid>` (the post-`new_session()`
|
||||
state). Assert `get()` returns a fresh session, **no warning** (this case is expected).
|
||||
4. **Symlink escape rejection (Vuln 2, SERVER_MODE=True):** create a symlink inside the user's
|
||||
storage dir pointing to `/tmp` (or another out-of-sandbox path). Attempt an upload. Assert
|
||||
the upload is denied **and** no file is created at the symlink target.
|
||||
5. **`O_NOFOLLOW` leaf-symlink rejection (Linux/macOS):** pre-plant a symlink at the upload
|
||||
target leaf path (no race needed — test the *property*, not the timing). Attempt upload.
|
||||
Assert `OSError` is raised with `errno.ELOOP` (Linux) or `errno.EMLINK` (macOS) and no file
|
||||
exists at the symlink target afterward. *Skip on Windows* (no `O_NOFOLLOW`).
|
||||
6. **Upload happy path:** confirm legitimate uploads still succeed in both `SERVER_MODE=False`
|
||||
and `SERVER_MODE=True`.
|
||||
7. **MFA login flow regression:** cover at least one full MFA email and authenticator login
|
||||
round-trip. The MFA fields are JSON-safe but the test suite should exercise them.
|
||||
8. **Cookie-HMAC mismatch with valid file-HMAC:** write a session file with a valid file-HMAC
|
||||
header but a `hmac_digest` inside the body that does NOT match the cookie-supplied digest.
|
||||
Assert `get()` returns a fresh session, **and** assert the `session file rejected:
|
||||
bad file-HMAC` warning is **not** logged (this is a cookie-binding mismatch, not a
|
||||
file-tamper event; current proposal does not log cookie mismatches to avoid noise from
|
||||
normal cookie-tampering attempts).
|
||||
9. **Round-trip:** construct a `ManagedSession`, call `session.sign(secret)` to populate
|
||||
`randval` and `hmac_digest` (a `put()` precondition; `put()` itself calls `sign()` on
|
||||
first write but a deterministic test should pre-sign), call `put()`, then `get()` with the
|
||||
matching cookie. Assert the returned `ManagedSession` has the same `data`, `randval`,
|
||||
`hmac_digest` as the input. Without this, a `put()` regression silently breaks every
|
||||
session save.
|
||||
10. **SERVER_MODE=False direct upload to sessions dir:** unit test against
|
||||
`FileBackedSessionManager.get()` directly (preferred over a full integration test to
|
||||
avoid Flask/CSRF setup). Write a malicious pickle file to the sessions path that
|
||||
`safe_join(self.path, sid)` would resolve to. Call `get(sid, fake_digest)`. Assert
|
||||
(a) `pickle.loads` does not execute the payload (sentinel not created), (b) `get()`
|
||||
returns a fresh session, (c) the file-HMAC warning is logged. This validates the
|
||||
Scenario A chain from §1.5 is closed at the session-read layer regardless of whether
|
||||
`SERVER_MODE` permits the upload.
|
||||
|
||||
#### 4.3.2 Existing tests to update
|
||||
|
||||
- `web/pgadmin/browser/tests/test_oauth2_with_mocking.py` — uses `session['oauth2_token']`
|
||||
(already a dict); verify it continues to pass.
|
||||
- Cloud-wizard tests using legacy serialized fixtures — update to dict fixtures during the
|
||||
per-provider PR.
|
||||
|
||||
#### 4.3.3 Manual verification
|
||||
|
||||
- All four cloud deployment wizards end-to-end (RDS, Azure, Google, BigAnimal).
|
||||
- OAuth2 login round-trip with a real provider.
|
||||
- Query Tool: execute, page, edit cell, transactions, savepoint, BYTEA result.
|
||||
- Debugger: at least one direct-debug session.
|
||||
|
||||
---
|
||||
|
||||
## 5. Blast Radius
|
||||
|
||||
### 5.1 Phase 1 (PR 1)
|
||||
|
||||
| Aspect | Estimate |
|
||||
|---|---|
|
||||
| Source files modified | 2 (`web/pgadmin/utils/session.py`, `web/pgadmin/misc/file_manager/__init__.py`) |
|
||||
| Test files added/modified | ~2 |
|
||||
| LOC changed | Source: ~80–110 (session HMAC logic ~50–80, file_manager realpath/O_NOFOLLOW/coverage ~30). Tests: ~250–400 (10 regression tests in §4.3.1, ~25–40 LOC each plus shared fixtures). **Total ~350–500.** |
|
||||
| Behavioural change | Sessions written by old code become unreadable → users re-login once on upgrade. One transient warning logged per rejected sid. |
|
||||
| Risk | **Low.** Self-contained, well-bounded, easy to review. |
|
||||
|
||||
### 5.2 Phase 2 — verified site counts
|
||||
|
||||
Per-feature breakdown (all numbers verified by grep):
|
||||
|
||||
| Feature | Files | `pickle.dumps` writes | `pickle.loads` reads | Risk | Rationale |
|
||||
|---|---|---|---|---|---|
|
||||
| `auth_obj` (OAuth2 login) | `authenticate/__init__.py`, `authenticate/oauth2.py` | 0 (live object stored directly) | 0 (live object read directly) | Medium | Login path — regression locks users out. Test against a real OAuth2 provider before merge. |
|
||||
| Azure cloud | `misc/cloud/azure/__init__.py` | 0 (live object stored directly at line 89) | 0 (read directly at lines 117, 138, 152, 164, 175, 189, 202, 216, 228, 692, …) | **High (verification gated)** | See §5.4. |
|
||||
| AWS RDS cloud | `misc/cloud/rds/__init__.py` | 1 (line 73) | 2 (lines 100, 129) | Medium | Few sites; boto3 client recreation should be cheap (assumed — verify per §5.4). |
|
||||
| Google cloud | `misc/cloud/google/__init__.py` | 4 (lines 110, 129, 145, 181) | 10 (lines 95, 127, 143, 161, 179, 199, 220, 241, 307, …) | Medium | Refactor: 1 `from_state()` constructor + ~10 callsites converted from `pickle.loads(...)` to `get_google_for_session(sid)`. |
|
||||
| BigAnimal cloud | `misc/cloud/biganimal/__init__.py` | 4 (lines 60, 73, 95, 106) | 11 (lines 57, 83, 93, 104, 115, 125, 137, 150, 162, 528, …) | Medium | Same shape as Google. |
|
||||
| SQL editor — `command_obj` producer | `tools/sqleditor/utils/start_running_query.py:204-208` | 1 (`pickle.dumps` at line 207, into a per-trans dict later persisted via `update_session_grid_transaction`) | 0 | **High (verification gated)** | See §5.4 item 4. |
|
||||
| SQL editor — Flask-session reads/writes | `tools/sqleditor/__init__.py` | gridData writes at lines 338, 632, 699, 740 | 1 (`pickle.loads` at line 1935) | **High (verification gated)** | The per-trans dict containing `command_obj` lives inside `session['gridData']`. Refactor: store command kind + identifying inputs; rebuild `Command` via `from_state()`; cache PG metadata in process-local TTL cache. |
|
||||
| Final swap (`session.py` to JSON) | `utils/session.py` | n/a | n/a | Low | Trivial once Phase 2 lands. |
|
||||
|
||||
**Total source files touched (Phase 1 + Phase 2):** 8 (files), 2 of which are big files
|
||||
(`misc/cloud/google/__init__.py`, `misc/cloud/biganimal/__init__.py`).
|
||||
**Total LOC changed (rough):** 600–900 across Phase 2.
|
||||
**PR count target:** 6–7 (one per feature plus the final swap).
|
||||
|
||||
> **Note on the two SQL-editor rows above:** they describe the same logical refactor but split
|
||||
> by file. The producer row covers where `pickle.dumps` is called; the Flask-session row
|
||||
> covers how that pickled blob enters and exits the `flask.session['gridData']` dict. Both
|
||||
> rows are addressed by a single PR (PR 7).
|
||||
|
||||
### 5.3 Failure modes considered
|
||||
|
||||
- **Existing session files lost on upgrade:** users re-login once. Expected; called out in
|
||||
release notes.
|
||||
- **Process restart drops in-memory caches (cloud SDKs):** users may need to re-authenticate
|
||||
against the cloud provider on the next request after a worker recycle. **Differs from
|
||||
today's behavior for Azure** (see §5.4 and §4.1 Phase 2).
|
||||
- **`session['azure']` grows because state replaces object:** dict holds ~5 primitive fields,
|
||||
far below the file-write size threshold.
|
||||
|
||||
### 5.4 Unverified claims that gate merge of specific PRs
|
||||
|
||||
These claims appear in the spec or its rationale and are **not yet evidence-backed**. They must
|
||||
be verified during the relevant PR before merge:
|
||||
|
||||
1. **Azure: `authentication_record_json` is sufficient to silently rebuild the credential
|
||||
without re-prompting the user for device code.** The class clearly persists the auth record
|
||||
and supports rebuilding the credential from it (`misc/cloud/azure/__init__.py:298-322`),
|
||||
*but* whether **all** ~12 Azure endpoints work after a per-request rebuild has not been
|
||||
end-to-end tested. Verification: run the Azure cloud wizard end-to-end after Phase 2 with
|
||||
the in-memory cache disabled; confirm no re-prompt.
|
||||
2. **boto3 RDS clients are cheap to recreate from credentials dict.** Generally true; SSO/STS
|
||||
token-cache state may complicate it. Verification: run the RDS wizard end-to-end across a
|
||||
simulated worker restart.
|
||||
3. **Google Cloud SDK clients are reconstructable from refresh-token credentials.**
|
||||
Refresh-token flows typically work; OAuth-installed-app flows with one-time auth codes do
|
||||
not. Verification: run the Google wizard end-to-end across a simulated worker restart.
|
||||
4. **The `Command` subclass tree in sqleditor is amenable to a `from_state()` constructor.**
|
||||
The proposal author has not yet read the Command hierarchy. The refactor PR (PR 7) starts
|
||||
with an audit that may reveal additional design work; the spec marks this PR as **High
|
||||
risk, scope-revisit on audit**.
|
||||
|
||||
**Fallback if PR 7 is intractable:** If the audit concludes the refactor is more than ~2
|
||||
weeks of work (e.g., the `Command` tree has deep coupling to live cursor state),
|
||||
the spec resolves the conflict as follows:
|
||||
- **Option A (preferred):** ship Phase 2 without sqleditor. The CI guard at PR 8 carves
|
||||
out `web/pgadmin/tools/sqleditor/` as a documented exception, with a TODO and tracking
|
||||
issue. Concrete encoding: sqleditor stores the pickled `Command` bytes
|
||||
**base64-encoded into a JSON string field** inside `session['gridData'][trans_id]`.
|
||||
The outer JSON session body is HMAC-protected by Phase 1's file-HMAC, so the encoded
|
||||
bytes cannot be tampered with from the disk side. **Caveat:** `pickle.loads` still runs
|
||||
on those bytes inside sqleditor on every read. If any other code path can write to
|
||||
`gridData` with attacker-controlled content (an unrelated bug), the RCE primitive
|
||||
returns. This is a strictly weaker guarantee than full pickle removal — Option A is a
|
||||
scope-management choice, not a security equivalence.
|
||||
- **Option B (rejected unless A is infeasible):** keep pickle in `session.py` itself, do
|
||||
not migrate to JSON, accept the ongoing footgun across the entire session surface.
|
||||
|
||||
PR 7 author chooses A or B based on the audit; spec is amended in the PR description with
|
||||
the rationale.
|
||||
|
||||
---
|
||||
|
||||
## 6. Implementation Plan
|
||||
|
||||
### 6.1 PR sequencing
|
||||
|
||||
1. **PR 1 — Close the CVEs.** HMAC-header redesign in `session.py`; in `file_manager`:
|
||||
`realpath` (§4.2.1), `O_NOFOLLOW` and dead-check removal in upload (§4.2.2),
|
||||
access-check coverage for `rename`/`delete`/`download` (§4.2.3) and `addfolder` (§4.2.4).
|
||||
Tests: **all 10 items** in §4.3.1. *Ships first, separately, with a security advisory.*
|
||||
2. **PR 2 — `auth_obj` data-only refactor.** Removes the live `AuthSourceManager` from the
|
||||
session.
|
||||
3. **PR 3 — Azure cloud refactor.** Includes verification step §5.4 item 1.
|
||||
4. **PR 4 — AWS RDS cloud refactor.** Includes verification §5.4 item 2.
|
||||
5. **PR 5 — Google cloud refactor.** Includes verification §5.4 item 3.
|
||||
6. **PR 6 — BigAnimal cloud refactor.**
|
||||
7. **PR 7 — SQL editor `command_obj` refactor.** Begins with a Command-tree audit
|
||||
(verification §5.4 item 4); scope may grow.
|
||||
8. **PR 8 — Final swap: `session.py` to plain JSON, delete `pickle` imports, add CI guard.**
|
||||
|
||||
PRs 2–7 are independent and can land in any order. **PR 8 has a hard dependency on PRs 2–7
|
||||
all having merged** — until every `pickle` site is removed, the JSON swap will fail to
|
||||
serialize live objects or pickled-bytes blobs.
|
||||
|
||||
### 6.2 Rollback strategy
|
||||
|
||||
Each PR is reverted independently if a regression is discovered post-merge. PR 1 has no
|
||||
in-codebase compatibility shim — a revert of PR 1 restores the pre-fix vulnerable behavior,
|
||||
which is unacceptable. **The team commits to fix-forward only for PR 1: any post-merge
|
||||
regression is resolved by a same-day hotfix, never by a revert.** This commitment requires
|
||||
on-call availability during the release window; the security advisory cannot publish without
|
||||
this commitment in place.
|
||||
|
||||
PRs 2–7 each remove a `pickle` site without changing the on-disk format; reverting any of them
|
||||
is safe and does not re-open RCE (since PR 1 already gates the on-disk read).
|
||||
|
||||
PR 8 (the JSON swap) carries upgrade risk for sessions written by PRs 1-7's pickle-with-header
|
||||
format. Format-detection on read after PR 8: the file-HMAC header is still validated first
|
||||
(same algorithm), then `json.loads(body)` is attempted. A pre-PR-8 pickle body fails JSON parse
|
||||
with `JSONDecodeError`, falls through to `new_session()`, and a warning is logged. **No
|
||||
execution path through `pickle.loads` remains in `session.py` post-PR-8** — a malicious or
|
||||
legitimate pickle file is rejected at JSON parse, never deserialized. One-time re-login at PR 8
|
||||
release; warning logged per dropped sid.
|
||||
|
||||
### 6.3 Multi-instance / HA note
|
||||
|
||||
pgAdmin is typically deployed single-instance. If two replicas share a sessions directory, both
|
||||
must upgrade together: in **both directions**, the receiver fails to parse the format and
|
||||
silently invalidates the session (old code reading a header'd file fails on `pickle.load` of
|
||||
ASCII hex; new code reading a header-less file fails the HMAC length check). No data loss
|
||||
beyond a one-time re-login per affected user, but operators must coordinate the upgrade window.
|
||||
Document in release notes.
|
||||
|
||||
**Mixed-version Python workers (rare):** `pickle.dumps(..., -1)` selects `HIGHEST_PROTOCOL`
|
||||
which varies by Python version (3.9 → protocol 5; 3.13 → protocol 5; 3.14 → 5). A protocol
|
||||
written by a newer worker and read by an older worker may fail to deserialize. Pre-existing
|
||||
assumption that all workers in a deployment run the same Python; not introduced by this
|
||||
proposal. After PR 8 (JSON swap), this concern disappears entirely.
|
||||
|
||||
### 6.4 Acceptance criteria
|
||||
|
||||
**PR 1 (security fix):**
|
||||
- [ ] All 10 regression tests in §4.3.1 added and passing.
|
||||
- [ ] Defensive non-empty-`SECRET_KEY` check added in `FileBackedSessionManager.__init__`
|
||||
(raise, not assert).
|
||||
- [ ] `_compute_file_hmac` accepts both `str` and `bytes` for `secret`.
|
||||
- [ ] Round-trip test (§4.3.1 test 9) passes — confirms `put()` produces a file readable by
|
||||
`get()`.
|
||||
- [ ] Windows realpath smoke test (§4.2.1) passes on Python 3.9.x.
|
||||
- [ ] CVE assigned and security advisory drafted; advisory publishes on the PR-1 release.
|
||||
- [ ] On-call coverage confirmed for PR-1 release window (per §6.2 fix-forward commitment).
|
||||
|
||||
**Each of PR 2-7 (per-feature refactor):**
|
||||
- [ ] Corresponding §5.4 verification item passes (where applicable: Azure → item 1, RDS →
|
||||
item 2, Google → item 3, sqleditor → item 4; auth_obj and BigAnimal have no §5.4 entry).
|
||||
- [ ] No regression in the relevant feature's existing test suite.
|
||||
- [ ] Manual verification per §4.3.3 for the affected feature (cloud wizard end-to-end,
|
||||
OAuth2 round-trip, or Query Tool sweep).
|
||||
- [ ] No new `pickle` site introduced in any other file (regression check).
|
||||
|
||||
**PR 8 (final pickle removal):**
|
||||
- [ ] `grep -rEn '^\s*(import pickle\b|from pickle\b)' web/pgadmin/ | grep -v '/tests/'`
|
||||
returns empty (or returns only the documented sqleditor exception per §5.4 item 4
|
||||
Option A).
|
||||
- [ ] All four cloud wizards verified end-to-end in `SERVER_MODE=True` and `SERVER_MODE=False`.
|
||||
- [ ] OAuth2 round-trip verified against a real provider.
|
||||
- [ ] Query Tool regression sweep passed (pagination, edit, transactions, BYTEA).
|
||||
- [ ] Audit table in §9 re-run on the merge commit and the result recorded in the PR
|
||||
description; any drift from the table in this proposal explained.
|
||||
|
||||
---
|
||||
|
||||
## 7. Alternatives Considered
|
||||
|
||||
### 7.1 Big-bang refactor in one PR
|
||||
|
||||
Rejected. Combines an urgent security fix with a multi-feature refactor; a regression in any
|
||||
cloud wizard would block the security fix from shipping. Review surface is too large for
|
||||
adequate scrutiny.
|
||||
|
||||
### 7.2 Keep pickle, only fix HMAC ordering
|
||||
|
||||
Rejected as the long-term answer. Closes the immediate CVE but leaves `pickle` as the de-facto
|
||||
session format and inside cloud handlers. Any future bug that lets an attacker influence
|
||||
session contents re-opens RCE. Adopted as **Phase 1** only — buys time for Phase 2.
|
||||
|
||||
### 7.3 Use `flask.json.tag.TaggedJSONSerializer`
|
||||
|
||||
Rejected for the on-disk session format. `TaggedJSONSerializer` handles `bytes`, `datetime`,
|
||||
`UUID`, `tuple`, `set` — which would let the in-session serialized blobs (the cloud-handler
|
||||
`pickle.dumps` outputs) silently survive the JSON switch. We want those *gone*, not preserved.
|
||||
Plain JSON forces the refactor.
|
||||
|
||||
### 7.4 Switch to `itsdangerous.URLSafeTimedSerializer` only (no file backend)
|
||||
|
||||
Rejected. Cookie size limits make this impractical for pgAdmin's session contents (especially
|
||||
`gridData` and server manager state). File-backed sessions stay.
|
||||
|
||||
### 7.5 chroot or seccomp-style sandboxing of the worker process
|
||||
|
||||
Out of scope. Requires deployment-level changes that vary by environment
|
||||
(Docker, Kubernetes, Snap, AppImage, systemd, native pip, conda, etc.) and that operators
|
||||
must opt into per-deployment. Worth tracking separately as a defense-in-depth follow-up.
|
||||
|
||||
---
|
||||
|
||||
## 8. Open Questions (resolved before approval)
|
||||
|
||||
1. **HMAC algorithm for the file header: SHA-256 vs `config.SESSION_DIGEST_METHOD` (default
|
||||
SHA-1)?**
|
||||
*Resolution:* hard-code SHA-256 in the file header. The primary rationale is **header-length
|
||||
predictability**: the file format needs a fixed prefix length decoupled from operator config,
|
||||
so that file parsing is deterministic. Secondary rationale: SHA-256 is the modern default
|
||||
and avoids a future deprecation step. The cookie HMAC remains tied to
|
||||
`SESSION_DIGEST_METHOD` for backward compatibility — that's an unrelated mechanism.
|
||||
2. **TTL for the process-local SDK client cache?**
|
||||
*Resolution:* match `permanent_session_lifetime` (Flask config), with passive eviction on
|
||||
each access.
|
||||
3. **Migration strategy for in-flight sessions:** silent drop vs. warning per dropped sid.
|
||||
*Resolution:* warning logged per rejected sid (sid prefix only, to avoid log-leaking
|
||||
active sids). Provides operator visibility without information disclosure.
|
||||
4. **CI guard for `pickle`:** add a lint check that fails CI if `import pickle` reappears
|
||||
outside `web/pgadmin/tests/`?
|
||||
*Resolution:* yes. Implementation: a single grep-based check in the existing test
|
||||
makefile, gated on PR 8.
|
||||
|
||||
## 9. Audit summary (referenced from §1.5 and §4.2)
|
||||
|
||||
This proposal relies on the following grep audits run against the v9.14 branch
|
||||
(commit-of-record: head of `fix/pickle-rce` at proposal time). Patterns are listed as
|
||||
ripgrep-compatible regex.
|
||||
|
||||
**Reproducer:** `rg --type py 'PATTERN' PATH` (or equivalently
|
||||
`grep -rEn --include='*.py' 'PATTERN' PATH`). Re-running these audits on the merge commit is
|
||||
a PR-8 acceptance criterion (§6.4).
|
||||
|
||||
**Audit 1 — symlink-creating primitives:**
|
||||
|
||||
```regex
|
||||
os\.symlink|os\.link\(|symbolic_link|symlink_to|Path\.symlink|\.symlink\(
|
||||
```
|
||||
|
||||
Path: `web/pgadmin/`. Result: **0 matches.**
|
||||
|
||||
**Audit 2 — archive extraction (could preserve symlinks from a malicious archive):**
|
||||
|
||||
```regex
|
||||
tarfile\.|zipfile\.|shutil\.unpack_archive|shutil\.copytree|extractall|extract_archive
|
||||
```
|
||||
|
||||
Path: `web/pgadmin/`. Result: **0 matches.**
|
||||
|
||||
**Audit 3 — shell-invocation primitives in file_manager:**
|
||||
|
||||
```regex
|
||||
subprocess|os\.popen|os\.system|shell=True
|
||||
```
|
||||
|
||||
Path: `web/pgadmin/misc/file_manager/`. Result: **0 matches.**
|
||||
|
||||
**Audit 4 — `os.path.abspath` callers in file_manager:**
|
||||
|
||||
```regex
|
||||
os\.path\.abspath
|
||||
```
|
||||
|
||||
Path: `web/pgadmin/misc/file_manager/__init__.py`. Result: **2 matches** at lines 903 and 1086.
|
||||
|
||||
**Audit 5 — directory-creation handlers in file_manager:**
|
||||
|
||||
```regex
|
||||
def (addfolder|add_folder|new_folder|mkdir|create_directory|createfolder)
|
||||
```
|
||||
|
||||
Path: `web/pgadmin/misc/file_manager/`. Result: **1 match** — `addfolder` at line 1232.
|
||||
|
||||
**Audit 6 — `os.mkdir` / `os.makedirs` in file_manager:**
|
||||
|
||||
```regex
|
||||
os\.mkdir|os\.makedirs
|
||||
```
|
||||
|
||||
Path: `web/pgadmin/misc/file_manager/`. Result: **1 match** at line 1250 (inside `addfolder`).
|
||||
|
||||
These results justify the §1.5 conclusion (no in-pgAdmin symlink primitive) and the §4.2.5
|
||||
conclusion (exhaustive audit of file_manager path-handling sites).
|
||||
|
||||
## 10. References
|
||||
|
||||
- Security report: 2026-04-29 (private)
|
||||
- CVE-2024-2044 — pgAdmin RCE in `SERVER_MODE=False`, earlier versions
|
||||
- CWE-502 — Deserialization of Untrusted Data
|
||||
- CWE-61 — UNIX Symbolic Link (Symlink) Following
|
||||
- CWE-22 — Improper Limitation of a Pathname to a Restricted Directory
|
||||
- OWASP Top 10 — A08:2021 Software and Data Integrity Failures
|
||||
- Existing pattern: `web/pgadmin/utils/driver/psycopg3/server_manager.py:519-530` —
|
||||
`as_dict()` + module-global live state
|
||||
- Linux `open(2)` — `O_NOFOLLOW`: refuse to follow symbolic links at the leaf component
|
||||
@@ -19,7 +19,9 @@ import base64
|
||||
import datetime
|
||||
import hmac
|
||||
import hashlib
|
||||
import logging
|
||||
import os
|
||||
import pickle
|
||||
import secrets
|
||||
import string
|
||||
import time
|
||||
@@ -29,7 +31,6 @@ from threading import Lock
|
||||
from flask import current_app, request, flash, redirect, has_request_context
|
||||
from flask_login import login_url
|
||||
|
||||
from pickle import dump, load
|
||||
from collections import OrderedDict
|
||||
from itsdangerous import signer
|
||||
|
||||
@@ -49,6 +50,24 @@ def _calc_hmac(body, secret):
|
||||
).decode()
|
||||
|
||||
|
||||
# File-HMAC: protects on-disk session files against tampering before
|
||||
# deserialization. SHA-256 is hard-coded (not driven by SESSION_DIGEST_METHOD)
|
||||
# so the header length is a fixed, format-stable constant.
|
||||
_FILE_HMAC = hashlib.sha256
|
||||
_HMAC_HEX_LEN = _FILE_HMAC().digest_size * 2 # 64 for SHA-256
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _compute_file_hmac(secret, body):
|
||||
"""Compute the hex-encoded HMAC over a session-file body.
|
||||
|
||||
Flask's SECRET_KEY may be str or bytes; both are accepted.
|
||||
"""
|
||||
key = secret.encode() if isinstance(secret, str) else secret
|
||||
return hmac.new(key, body, _FILE_HMAC).hexdigest().encode()
|
||||
|
||||
|
||||
sess_lock = Lock()
|
||||
LAST_CHECK_SESSION_FILES = None
|
||||
|
||||
@@ -199,6 +218,10 @@ class CachingSessionManager(SessionManager):
|
||||
class FileBackedSessionManager(SessionManager):
|
||||
|
||||
def __init__(self, path, secret, disk_write_delay, skip_paths=None):
|
||||
if not secret:
|
||||
# File-HMAC integrity collapses without a secret. Use raise, not
|
||||
# assert: -O strips assertions in production.
|
||||
raise RuntimeError("SECRET_KEY must be non-empty")
|
||||
self.path = path
|
||||
self.secret = secret
|
||||
self.disk_write_delay = disk_write_delay
|
||||
@@ -238,27 +261,54 @@ class FileBackedSessionManager(SessionManager):
|
||||
return ManagedSession(sid=sid)
|
||||
|
||||
def get(self, sid, digest):
|
||||
'Retrieve a managed session by session-id, checking the HMAC digest'
|
||||
"""Retrieve a managed session by session-id, verifying file integrity.
|
||||
|
||||
File format:
|
||||
+----------------------------------------------------+
|
||||
| _HMAC_HEX_LEN bytes : hex HMAC over the body |
|
||||
+----------------------------------------------------+
|
||||
| N bytes : pickle body — (randval, digest, data) |
|
||||
+----------------------------------------------------+
|
||||
|
||||
The HMAC is verified against raw bytes BEFORE pickle.loads runs, so
|
||||
a malicious file dropped in the sessions directory cannot trigger
|
||||
arbitrary code execution via __reduce__ during deserialization.
|
||||
"""
|
||||
fname = safe_join(self.path, sid)
|
||||
data = None
|
||||
hmac_digest = None
|
||||
randval = None
|
||||
if fname is None or not os.path.exists(fname):
|
||||
return self.new_session()
|
||||
|
||||
if fname is not None and os.path.exists(fname):
|
||||
try:
|
||||
with open(fname, 'rb') as f:
|
||||
randval, hmac_digest, data = load(f)
|
||||
except Exception:
|
||||
pass
|
||||
try:
|
||||
with open(fname, 'rb') as f:
|
||||
header = f.read(_HMAC_HEX_LEN)
|
||||
body = f.read()
|
||||
if len(header) != _HMAC_HEX_LEN or len(body) == 0:
|
||||
# 0-byte placeholder from new_session(), or pre-fix legacy
|
||||
# file shorter than the header. Silently invalidate; one-time
|
||||
# re-login on upgrade is acceptable and expected.
|
||||
return self.new_session()
|
||||
expected = _compute_file_hmac(self.secret, body)
|
||||
if not hmac.compare_digest(header, expected):
|
||||
logger.warning(
|
||||
"session file rejected: bad file-HMAC for sid=%s",
|
||||
sid[:8] if sid else '<empty>',
|
||||
)
|
||||
return self.new_session()
|
||||
# Body integrity verified — safe to deserialize.
|
||||
randval, hmac_digest, data = pickle.loads(body)
|
||||
except (pickle.UnpicklingError, EOFError, OSError, MemoryError,
|
||||
ValueError, TypeError):
|
||||
# Narrow catch: silently invalidate corrupted/unreadable files.
|
||||
# Programming errors (AttributeError, NameError, etc.) propagate
|
||||
# so they surface in tests rather than being masked.
|
||||
return self.new_session()
|
||||
|
||||
if not data:
|
||||
return self.new_session()
|
||||
|
||||
# This assumes the file is correct, if you really want to
|
||||
# make sure the session is good from the server side, you
|
||||
# can re-calculate the hmac
|
||||
|
||||
# Cookie-binding check: the cookie's digest must match the file's
|
||||
# stored digest. Prevents using one session's file with another's
|
||||
# cookie even when both are server-written.
|
||||
if hmac_digest != digest:
|
||||
return self.new_session()
|
||||
|
||||
@@ -289,11 +339,13 @@ class FileBackedSessionManager(SessionManager):
|
||||
if fname is None:
|
||||
raise InternalServerError('Failed to update the session')
|
||||
|
||||
body = pickle.dumps(
|
||||
(session.randval, session.hmac_digest, dict(session)), -1
|
||||
)
|
||||
header = _compute_file_hmac(self.secret, body)
|
||||
with open(fname, 'wb') as f:
|
||||
dump(
|
||||
(session.randval, session.hmac_digest, dict(session)),
|
||||
f
|
||||
)
|
||||
f.write(header)
|
||||
f.write(body)
|
||||
|
||||
|
||||
class ManagedSessionInterface(SessionInterface):
|
||||
|
||||
@@ -0,0 +1,480 @@
|
||||
##########################################################################
|
||||
#
|
||||
# pgAdmin 4 - PostgreSQL Tools
|
||||
#
|
||||
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
|
||||
# This software is released under the PostgreSQL Licence
|
||||
#
|
||||
##########################################################################
|
||||
|
||||
"""
|
||||
Unit tests for the FileBackedSessionManager file format.
|
||||
|
||||
These tests validate the HMAC-first file format introduced to close
|
||||
CVE-pending pickle deserialization RCE. They run as plain unittest
|
||||
TestCases without needing a Postgres server connection — the file format
|
||||
logic is pure I/O.
|
||||
"""
|
||||
|
||||
import hashlib
|
||||
import hmac
|
||||
import logging
|
||||
import os
|
||||
import pickle
|
||||
import shutil
|
||||
import tempfile
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
|
||||
from pgadmin.utils.route import BaseTestGenerator
|
||||
from pgadmin.utils.session import FileBackedSessionManager
|
||||
|
||||
|
||||
SECRET = "test-secret-key-do-not-use-in-prod"
|
||||
HMAC_HEX_LEN = hashlib.sha256().digest_size * 2 # 64
|
||||
|
||||
|
||||
def compute_test_hmac(body: bytes, secret: str = SECRET) -> bytes:
|
||||
"""Compute the file-HMAC the way session.py is expected to compute it.
|
||||
|
||||
Mirrors _compute_file_hmac in session.py so tests can construct valid
|
||||
and invalid file payloads.
|
||||
"""
|
||||
key = secret.encode() if isinstance(secret, str) else secret
|
||||
return hmac.new(key, body, hashlib.sha256).hexdigest().encode()
|
||||
|
||||
|
||||
def write_session_file(path: str, sid: str, body: bytes,
|
||||
header: bytes = None) -> str:
|
||||
"""Write a session file in the new (header + body) format.
|
||||
|
||||
If `header` is None, computes the correct HMAC over `body`. Pass an
|
||||
explicit `header` to construct tampered files.
|
||||
Returns the absolute file path.
|
||||
"""
|
||||
fname = os.path.join(path, sid)
|
||||
if header is None:
|
||||
header = compute_test_hmac(body)
|
||||
with open(fname, 'wb') as f:
|
||||
f.write(header)
|
||||
f.write(body)
|
||||
return fname
|
||||
|
||||
|
||||
def write_legacy_session_file(path: str, sid: str,
|
||||
payload: tuple) -> str:
|
||||
"""Write a session file in the old (header-less, pure pickle) format.
|
||||
|
||||
Used to test migration: old files must be silently rejected by the
|
||||
new read path.
|
||||
"""
|
||||
fname = os.path.join(path, sid)
|
||||
with open(fname, 'wb') as f:
|
||||
pickle.dump(payload, f)
|
||||
return fname
|
||||
|
||||
|
||||
def make_pickle_body(randval: str, hmac_digest: str, data: dict) -> bytes:
|
||||
"""Produce the pickle body the new format wraps under an HMAC header."""
|
||||
return pickle.dumps((randval, hmac_digest, dict(data)), -1)
|
||||
|
||||
|
||||
class _MaliciousPayload:
|
||||
"""A pickle payload whose __reduce__ creates a sentinel directory.
|
||||
|
||||
Used to prove that pickle.loads is NOT called on attacker-controlled
|
||||
bytes when the file-HMAC does not match.
|
||||
"""
|
||||
|
||||
sentinel_path = None # set per-test before pickling
|
||||
|
||||
def __reduce__(self):
|
||||
return (os.makedirs, (self.sentinel_path, True))
|
||||
|
||||
|
||||
class _SessionTestSetupMixin:
|
||||
"""Mixin providing common setup for session-format tests.
|
||||
|
||||
Used in MRO with BaseTestGenerator on concrete test classes only —
|
||||
the mixin itself is not a TestCase, so it does NOT get auto-registered
|
||||
by TestsGeneratorRegistry as a runnable test.
|
||||
|
||||
setUp here calls unittest.TestCase.setUp directly, skipping
|
||||
BaseTestGenerator.setUp's Postgres connection (these are pure
|
||||
file-format unit tests that don't need a database).
|
||||
"""
|
||||
|
||||
def setUp(self):
|
||||
unittest.TestCase.setUp(self)
|
||||
self.tmpdir = tempfile.mkdtemp(prefix="pga_session_test_")
|
||||
self.manager = FileBackedSessionManager(
|
||||
path=self.tmpdir,
|
||||
secret=SECRET,
|
||||
disk_write_delay=0,
|
||||
)
|
||||
# Capture warnings emitted by session.py's logger.
|
||||
self._log_records = []
|
||||
self._log_handler = self._make_capturing_handler()
|
||||
self._session_logger = logging.getLogger(
|
||||
"pgadmin.utils.session")
|
||||
self._session_logger.addHandler(self._log_handler)
|
||||
self._prev_level = self._session_logger.level
|
||||
self._session_logger.setLevel(logging.DEBUG)
|
||||
|
||||
def tearDown(self):
|
||||
self._session_logger.removeHandler(self._log_handler)
|
||||
self._session_logger.setLevel(self._prev_level)
|
||||
shutil.rmtree(self.tmpdir, ignore_errors=True)
|
||||
|
||||
def _make_capturing_handler(self):
|
||||
outer = self
|
||||
|
||||
class _H(logging.Handler):
|
||||
def emit(self, record):
|
||||
outer._log_records.append(record)
|
||||
return _H()
|
||||
|
||||
def assert_warning_logged(self, substring):
|
||||
for r in self._log_records:
|
||||
if r.levelno >= logging.WARNING and substring in r.getMessage():
|
||||
return
|
||||
self.fail(
|
||||
"Expected WARNING log containing %r, got: %r" % (
|
||||
substring,
|
||||
[r.getMessage() for r in self._log_records],
|
||||
)
|
||||
)
|
||||
|
||||
def assert_no_warning_logged(self, substring):
|
||||
for r in self._log_records:
|
||||
if r.levelno >= logging.WARNING and substring in r.getMessage():
|
||||
self.fail(
|
||||
"Unexpected WARNING containing %r: %r" % (
|
||||
substring, r.getMessage()))
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Positive tests — legitimate flows must continue to work.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSessionRoundTrip(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""A session written via put() must be readable via get()."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sess = self.manager.new_session()
|
||||
sess.sid = "11111111-1111-1111-1111-111111111111"
|
||||
sess['user_id'] = 42
|
||||
sess['_csrf_token'] = "abc123"
|
||||
# Pre-sign so put() has hmac_digest populated and stable across reads.
|
||||
sess.sign(SECRET)
|
||||
self.manager.put(sess)
|
||||
|
||||
loaded = self.manager.get(sess.sid, sess.hmac_digest)
|
||||
|
||||
self.assertEqual(loaded.sid, sess.sid)
|
||||
self.assertEqual(loaded['user_id'], 42)
|
||||
self.assertEqual(loaded['_csrf_token'], "abc123")
|
||||
self.assertEqual(loaded.randval, sess.randval)
|
||||
self.assertEqual(loaded.hmac_digest, sess.hmac_digest)
|
||||
|
||||
|
||||
class TestMultipleSessionsAreIndependent(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Two sessions must not bleed data into each other."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sess_a = self.manager.new_session()
|
||||
sess_a.sid = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
|
||||
sess_a['name'] = 'alice'
|
||||
sess_a.sign(SECRET)
|
||||
self.manager.put(sess_a)
|
||||
|
||||
sess_b = self.manager.new_session()
|
||||
sess_b.sid = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"
|
||||
sess_b['name'] = 'bob'
|
||||
sess_b.sign(SECRET)
|
||||
self.manager.put(sess_b)
|
||||
|
||||
loaded_a = self.manager.get(sess_a.sid, sess_a.hmac_digest)
|
||||
loaded_b = self.manager.get(sess_b.sid, sess_b.hmac_digest)
|
||||
|
||||
self.assertEqual(loaded_a['name'], 'alice')
|
||||
self.assertEqual(loaded_b['name'], 'bob')
|
||||
|
||||
|
||||
class TestSessionUpdatePersists(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""A modified session, re-put, must round-trip the updated data."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sess = self.manager.new_session()
|
||||
sess.sid = "cccccccc-cccc-cccc-cccc-cccccccccccc"
|
||||
sess['count'] = 1
|
||||
sess.sign(SECRET)
|
||||
self.manager.put(sess)
|
||||
|
||||
sess['count'] = 2
|
||||
# disk_write_delay=0 so put() writes immediately
|
||||
self.manager.put(sess)
|
||||
|
||||
loaded = self.manager.get(sess.sid, sess.hmac_digest)
|
||||
self.assertEqual(loaded['count'], 2)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Negative tests — malicious / corrupted / unexpected files must be rejected
|
||||
# without executing any unverified bytes.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestMaliciousPickleNoHeader(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Spec T1: pre-fix-format pickle file in sessions dir is NOT executed.
|
||||
|
||||
A legacy session file is pure pickle bytes (no HMAC header). The new
|
||||
read path must not invoke pickle.loads on the file body before HMAC
|
||||
verification.
|
||||
"""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sentinel = os.path.join(self.tmpdir, "RCE_SENTINEL_DIR")
|
||||
_MaliciousPayload.sentinel_path = sentinel
|
||||
# Construct legacy-format file: just pickle.dumps of a malicious tuple
|
||||
# (the location pre-fix code reads via pickle.load).
|
||||
sid = "dddddddd-dddd-dddd-dddd-dddddddddddd"
|
||||
legacy_payload = (
|
||||
"fake_randval", "fake_hmac",
|
||||
{"evil": _MaliciousPayload()},
|
||||
)
|
||||
write_legacy_session_file(self.tmpdir, sid, legacy_payload)
|
||||
|
||||
result = self.manager.get(sid, "fake_hmac")
|
||||
|
||||
self.assertFalse(
|
||||
os.path.exists(sentinel),
|
||||
"RCE primitive fired — pickle was deserialized before HMAC check"
|
||||
)
|
||||
# Got a fresh session, not the malicious one.
|
||||
self.assertIsNone(result.get("evil"))
|
||||
self.assert_warning_logged("session file rejected")
|
||||
|
||||
|
||||
class TestCorruptedHmacHeader(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Spec T2: a tampered header is rejected with a warning."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sid = "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"
|
||||
body = make_pickle_body("rv", "digest", {"k": "v"})
|
||||
good_header = compute_test_hmac(body)
|
||||
# Flip the first byte of the header.
|
||||
bad_header = bytes(
|
||||
[(good_header[0] ^ 0x01)]) + good_header[1:]
|
||||
write_session_file(self.tmpdir, sid, body, header=bad_header)
|
||||
|
||||
result = self.manager.get(sid, "digest")
|
||||
|
||||
# Cookie HMAC mismatch returns new_session — same observable result,
|
||||
# but the warning is the discriminator.
|
||||
self.assertNotEqual(result.hmac_digest, "digest")
|
||||
self.assert_warning_logged("session file rejected")
|
||||
|
||||
|
||||
class TestEmptyFilePassthrough(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Spec T3: a 0-byte placeholder (from new_session) must NOT warn."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sid = "ffffffff-ffff-ffff-ffff-ffffffffffff"
|
||||
# Empty file — what new_session() leaves on disk before the first put.
|
||||
with open(os.path.join(self.tmpdir, sid), 'wb'):
|
||||
pass
|
||||
|
||||
result = self.manager.get(sid, "anything")
|
||||
|
||||
self.assertIsNone(result.hmac_digest)
|
||||
self.assert_no_warning_logged("session file rejected")
|
||||
|
||||
|
||||
class TestTruncatedHeader(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""A file shorter than the header length is silently discarded."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sid = "10101010-1010-1010-1010-101010101010"
|
||||
with open(os.path.join(self.tmpdir, sid), 'wb') as f:
|
||||
f.write(b"a" * (HMAC_HEX_LEN - 1)) # 1 byte short
|
||||
|
||||
result = self.manager.get(sid, "anything")
|
||||
|
||||
self.assertIsNone(result.hmac_digest)
|
||||
# Truncation is indistinguishable from "old format" for our purposes;
|
||||
# no warning to avoid noise during upgrade rollout.
|
||||
self.assert_no_warning_logged("session file rejected")
|
||||
|
||||
|
||||
class TestValidHeaderEmptyBody(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""A file with header bytes but empty body is silently discarded."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sid = "20202020-2020-2020-2020-202020202020"
|
||||
# Exactly HMAC_HEX_LEN bytes of header, zero bytes of body.
|
||||
with open(os.path.join(self.tmpdir, sid), 'wb') as f:
|
||||
f.write(b"a" * HMAC_HEX_LEN)
|
||||
|
||||
result = self.manager.get(sid, "anything")
|
||||
|
||||
self.assertIsNone(result.hmac_digest)
|
||||
self.assert_no_warning_logged("session file rejected")
|
||||
|
||||
|
||||
class TestCookieHmacMismatchWithValidFile(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Spec T8: a legitimate file but a cookie that doesn't bind to it.
|
||||
|
||||
The file-HMAC verifies the file is server-written; the cookie-HMAC
|
||||
verifies the cookie matches THIS session. Both checks remain.
|
||||
"""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sid = "30303030-3030-3030-3030-303030303030"
|
||||
body = make_pickle_body("rv", "real_digest", {"k": "v"})
|
||||
write_session_file(self.tmpdir, sid, body) # valid file-HMAC
|
||||
|
||||
# Caller presents the wrong cookie digest.
|
||||
result = self.manager.get(sid, "wrong_digest")
|
||||
|
||||
# New session returned — but no file-HMAC warning, because the file
|
||||
# itself was fine.
|
||||
self.assertNotEqual(result.hmac_digest, "real_digest")
|
||||
self.assert_no_warning_logged("session file rejected")
|
||||
|
||||
|
||||
class TestUnsafeSidReturnsNewSession(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""A sid containing path-traversal characters yields safe_join None."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
# safe_join refuses these — get() must return new_session, not crash.
|
||||
for unsafe_sid in ("../../etc/passwd", "/absolute/path", ""):
|
||||
with self.subTest(sid=unsafe_sid):
|
||||
result = self.manager.get(unsafe_sid, "anything")
|
||||
self.assertIsNone(result.hmac_digest)
|
||||
|
||||
|
||||
class TestEmptySecretRaises(BaseTestGenerator):
|
||||
"""Constructing a manager with empty SECRET_KEY must fail loudly."""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def setUp(self):
|
||||
unittest.TestCase.setUp(self)
|
||||
self.tmpdir = tempfile.mkdtemp(prefix="pga_session_test_")
|
||||
|
||||
def tearDown(self):
|
||||
shutil.rmtree(self.tmpdir, ignore_errors=True)
|
||||
|
||||
def runTest(self):
|
||||
for empty in ("", None):
|
||||
with self.subTest(secret=empty):
|
||||
with self.assertRaises(RuntimeError):
|
||||
FileBackedSessionManager(
|
||||
path=self.tmpdir,
|
||||
secret=empty,
|
||||
disk_write_delay=0,
|
||||
)
|
||||
|
||||
|
||||
class TestRealisticSessionShapeRoundTrip(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Round-trip a session with realistic pgAdmin contents (MFA, OAuth2, etc.).
|
||||
|
||||
Spec test 7: MFA fields are JSON-safe but exercise them through put/get
|
||||
to confirm the format change preserves the data structures pgAdmin
|
||||
actually stashes in flask.session. Full MFA login flow is covered by
|
||||
§4.3.3 manual verification (separate from this unit test).
|
||||
"""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sess = self.manager.new_session()
|
||||
sess.sid = "50505050-5050-5050-5050-505050505050"
|
||||
# Mirror the shape of fields seen across pgadmin/authenticate/mfa/*,
|
||||
# pgadmin/authenticate/oauth2.py, pgadmin/__init__.py, etc.
|
||||
sess.update({
|
||||
'_user_id': '1',
|
||||
'_csrf_token': 'a' * 40,
|
||||
'mfa_authenticated': True,
|
||||
'mfa_email_id': 'user@example.com',
|
||||
'mfa_email_code': '123456',
|
||||
'mfa_authenticator_opt': 'totp',
|
||||
'oauth2_token': {
|
||||
'access_token': 'a' * 60,
|
||||
'token_type': 'Bearer',
|
||||
'expires_in': 3600,
|
||||
'id_token': 'b' * 200,
|
||||
},
|
||||
'auth_source_manager': {
|
||||
'current_source': 'internal',
|
||||
'sources': ['internal'],
|
||||
},
|
||||
'allow_save_password': True,
|
||||
'pass_enc_key': 'c' * 32,
|
||||
'fileManagerData': {
|
||||
'last_path': '/var/lib/pgadmin/storage/alice/',
|
||||
},
|
||||
})
|
||||
sess.sign(SECRET)
|
||||
self.manager.put(sess)
|
||||
|
||||
loaded = self.manager.get(sess.sid, sess.hmac_digest)
|
||||
|
||||
self.assertEqual(loaded['_user_id'], '1')
|
||||
self.assertTrue(loaded['mfa_authenticated'])
|
||||
self.assertEqual(loaded['mfa_email_id'], 'user@example.com')
|
||||
self.assertEqual(
|
||||
loaded['oauth2_token']['access_token'], 'a' * 60)
|
||||
self.assertEqual(
|
||||
loaded['auth_source_manager']['current_source'], 'internal')
|
||||
self.assertEqual(
|
||||
loaded['fileManagerData']['last_path'],
|
||||
'/var/lib/pgadmin/storage/alice/')
|
||||
|
||||
|
||||
class TestServerModeFalseDirectUpload(_SessionTestSetupMixin, BaseTestGenerator):
|
||||
"""Spec T10: Scenario A chain closed at the session-read layer.
|
||||
|
||||
Even if SERVER_MODE=False permits an upload directly into the sessions
|
||||
directory, the file-HMAC check rejects the resulting file and pickle is
|
||||
never deserialized.
|
||||
"""
|
||||
|
||||
scenarios = [('default', dict())]
|
||||
|
||||
def runTest(self):
|
||||
sentinel = os.path.join(self.tmpdir, "SCENARIO_A_SENTINEL")
|
||||
_MaliciousPayload.sentinel_path = sentinel
|
||||
# Simulate the file the attacker would have written via Scenario A:
|
||||
# legacy-format pickle (no HMAC header).
|
||||
sid = "40404040-4040-4040-4040-404040404040"
|
||||
write_legacy_session_file(
|
||||
self.tmpdir, sid,
|
||||
("rv", "fake", {"x": _MaliciousPayload()}),
|
||||
)
|
||||
|
||||
result = self.manager.get(sid, "fake")
|
||||
|
||||
self.assertFalse(os.path.exists(sentinel))
|
||||
self.assertIsNone(result.get("x"))
|
||||
self.assert_warning_logged("session file rejected")
|
||||
Reference in new issue
Block a user