Skip to content

Commit cce03fd

Browse files
authored
Validate freeDB record structure in mdb_page_alloc on both engines (#488)
mdb_page_alloc() trusted a free-database record read from disk completely; no patch in either bundled series touched the allocator. Three structural defects, confirmed by reproducer on both engines and both writemap modes: - Forged element count read past the node (SIGBUS) and merged garbage page numbers into me_pghead. - A page number from a record was never range-checked: the reuse path jumps past the me_maxpg bound guarding only the fresh-page path, so under MDB_WRITEMAP the store lands outside the map (SIGSEGV) and without it mdb_page_flush() pwrites at an arbitrary 64-bit file offset, committing silently and growing the file without bound. - A duplicate page number tripped mdb_tassert() in mdb_page_dirty() -> abort(). New validate-freedb-record patch on both trees adds three constant-time, point-of-use checks (no new loop over file data): the node size must equal (count+1)*sizeof(pgno); a reused page number must fall in [NUM_METAS, next_pgno) with room for the run; and mdb_page_dirty() returns MDB_CORRUPTED instead of asserting, propagated by its callers. Structural ("Tier 0") checks only -- a coherent forgery naming a genuinely live, in-range page is left to an offline reachability verifier. This removes the memory-safety half of issue #484's freeDB caveat, not the semantic one. Regression tests in tests/cve_test.py (FreeDbRecordTest), engine-agnostic, including a positive no-false-rejection test. Full suite green on both engines; mypy, pyright, and stubtest clean.
1 parent 0d181ce commit cce03fd

9 files changed

Lines changed: 706 additions & 0 deletions

File tree

‎ChangeLog‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,27 @@ Bug fixes
6262
data written by an unaffected build still read back correctly on an
6363
affected one — only the write path consulted the faulty check.
6464

65+
- New ``validate-freedb-record`` hardening patch for both bundled engines:
66+
``mdb_page_alloc()`` trusted a free-database record's structure
67+
completely. Its element count was never checked against the record's
68+
node size, so a forged count read past the end of the node
69+
(``mdb_midl_xmerge`` SIGBUS) and merged garbage page numbers into the
70+
in-memory free list. A page number taken from a record was never
71+
range-checked either — the reuse path jumps past the bound the
72+
fresh-page path applies — so a crafted number could be handed out
73+
directly: under ``MDB_WRITEMAP`` the store lands outside the map
74+
(SIGSEGV), and without it ``mdb_page_flush()`` later pwrites at an
75+
arbitrary file offset, committing silently and growing the file
76+
without limit. The record's node size must now equal
77+
``(count+1)*sizeof(pgno)``, and a reused page number must fall within
78+
``[NUM_METAS, next_pgno)`` with room for the whole run. A forged
79+
record listing the same page twice also tripped an assertion in
80+
``mdb_page_dirty()`` (``abort()``); it now returns ``MDB_CORRUPTED``.
81+
As with the rest of the series this matters only if you open a database
82+
file you do not trust. These checks are structural only: a record that
83+
lists a genuinely live page in range still passes, which is left to an
84+
offline verifier.
85+
6586
- New ``cve-2019-16225-validate-mp-txnid`` hardening patch for the bundled
6687
LMDB 1.0 engine, restoring a protection 0.9 has had since CVE-2019-16225.
6788
0.9 rejects a mapped page claiming ``P_DIRTY``; 1.0 removed that flag and

‎docs/freedb-record-validation.md‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
# Detecting a forged freeDB record
2+
3+
Status: **open**. Two confirmed defects in `mdb_page_alloc()` (Tier 0 below)
4+
are unfixed on both engines. They are more severe than issue #484, cheaper to
5+
fix, and independent of it — they should be filed separately and fixed first.
6+
7+
Written up from an investigation into issue #484's residual freeDB caveat: the
8+
1.0 `mp_txnid` fix proposed there proves a page is "mine" via the allocation
9+
invariant, which reaches back into freeDB records that a hostile file also
10+
controls. The question this document answers is how such a record could be
11+
detected.
12+
13+
## The short answer: detection has to start one level lower
14+
15+
Looking for a way to detect a *semantically* forged freeDB record — one that
16+
lists a live page — turned up the fact that nothing validates freeDB records
17+
**structurally** either. No patch in either series touches `mdb_page_alloc`. A
18+
record read from disk is trusted completely, and this is exploitable well
19+
beyond the MVCC problem in #484.
20+
21+
Two independent defects, both engines, confirmed by reproducer:
22+
23+
**The pgno values are unbounded.** The reuse path does
24+
`pgno = mop[i]; goto search_done;` (`build/lib09/mdb.c:2339`,
25+
`build/lib10/mdb.c:2833`) — and that `goto` jumps *past* the
26+
`pgno + num >= env->me_maxpg` bound at `lib09:2422` / `lib10:2924`, which
27+
guards only the fresh-page path. So a pgno taken from the freeDB is never
28+
range-checked at all. Building a normal database, shifting every pgno in its
29+
freeDB record up by 100000 against a 10 MB map (`me_maxpg` = 2560), and
30+
reopening:
31+
32+
| | result |
33+
| --- | --- |
34+
| `writemap=True` | **SIGSEGV** — the store `np->mp_pgno = pgno` lands at `me_map + 4096×100002` |
35+
| `writemap=False` | **completes silently**; `mdb_page_flush` pwrites at `pgno×psize`, growing `data.mdb` from 237,568 to **409,653,248 bytes** |
36+
37+
Identical on 0.9 and 1.0. Note the non-writemap case: an attacker-controlled
38+
64-bit file offset, with `map_size` bypassed entirely, and *no error raised*.
39+
40+
**The element count is unvalidated.** `i = idl[0]` (`lib09:2396`,
41+
`lib10:2898`) is never checked against `data.mv_size`, and
42+
`mdb_midl_xmerge` then reads `idl[1..idl[0]]`. Forging count 54 → 200000 on a
43+
440-byte record gives **SIGBUS** (reading past EOF inside the mapping) and
44+
merges garbage pgnos into `me_pghead`. This one is independent of the first —
45+
it needs no bad pgno.
46+
47+
As a side effect, forging duplicate pgnos trips `mdb_tassert(txn, rc == 0)` in
48+
`mdb_page_dirty()`, which calls `abort()`. That is a denial of service where an
49+
`MDB_CORRUPTED` return belongs.
50+
51+
## How to detect a forged record, in four tiers
52+
53+
**Tier 0 — structural validation, essentially free, always-on.** Right after
54+
`mdb_node_read` returns the record in `mdb_page_alloc`, one linear pass over
55+
the IDL before it reaches `mdb_midl_xmerge`: `data.mv_size` must match
56+
`(idl[0]+1) * sizeof(MDB_ID)`; the entries must be strictly descending (which
57+
gives duplicate detection within the record for free); and every pgno must lie
58+
in `[NUM_METAS, txn->mt_next_pgno)`. That pass is O(n) over an array the merge
59+
is about to walk anyway, so it roughly doubles a cost already being paid, on a
60+
path that runs once per freeDB record consumed rather than per page. This
61+
closes both confirmed defects and needs no new flag or API.
62+
63+
**Tier 1 — cheap semantic invariants, still always-on, partial coverage.**
64+
After the merge, `me_pghead` must remain strictly descending — adjacent equals
65+
mean two different records freed the same page without an intervening
66+
reallocation, which cannot happen in a sane file. On 1.0 only, a page listed in
67+
the record keyed by txnid K must satisfy `mp_txnid <= K`, since the page's last
68+
writer necessarily precedes its freeing; that catches an attacker trying to
69+
free a recently-written page. 0.9 cannot do this — its `MDB_page` header has no
70+
txnid field, only `mp_pad` and `mp_flags`. Also convert the `mdb_page_dirty`
71+
assertion to a `MDB_CORRUPTED` return.
72+
73+
**Tier 2 — page accounting.** This already exists: `mdb_audit()` at
74+
`lib09:1831` / `lib10:2224` checks exactly
75+
`freecount + count + NUM_METAS == mt_next_pgno`, summing freeDB record counts
76+
against each DB's `md_branch_pages + md_leaf_pages + md_overflow_pages`. Adding
77+
a live page's pgno to a freeDB record without removing it from the tree makes
78+
that sum overshoot. But it is `#if (MDB_DEBUG) > 2`, it only `fprintf`s rather
79+
than failing, it needs all named DBs open to be correct, and — the real limit —
80+
it trusts `md_*_pages`, which the attacker also controls and can simply
81+
decrement to rebalance. It catches lazy forgeries, not deliberate ones.
82+
83+
**Tier 3 — reachability traversal, the only sound detector.** Walk from the
84+
meta page through every DB root (main, freeDB, and every `F_SUBDATA` sub-DB),
85+
marking a bitmap of live pages, then assert that the freeDB and the live set
86+
are disjoint and together cover `[NUM_METAS, next_pgno)`. This trusts nothing
87+
but page structure, so it defeats even a fully coherent forgery. Cost is
88+
O(database pages) plus a bitmap of `next_pgno` bits — 32 MB for a 1 TB database
89+
at 4 K pages, which is fine. It cannot be a per-transaction check.
90+
91+
## On the "no new API" constraint
92+
93+
Tiers 0 through 2 are unconditional validation, consistent with how the rest of
94+
the hardening series works, and introduce no caller-visible surface. Tier 3 is
95+
inherently opt-in, but it does not have to be the "declare this file untrusted"
96+
open-time flag that #484 wanted to avoid — it fits naturally as a verb rather
97+
than a policy: a `verify` subcommand in `lmdb/tool.py`, or an
98+
`Environment.verify()` method. That is an offline audit someone runs on a file
99+
they do not trust, not a mode that changes how every open behaves.
100+
101+
## What this does to issue #484
102+
103+
The freeDB caveat on the proposed 1.0 `mp_txnid` fix survives, but shrinks a
104+
lot. With Tier 0 in place, a pgno arriving from the freeDB is at least
105+
guaranteed to be a real, in-range page, so the residual is narrowed to a
106+
coherent forgery naming a genuinely live page — a consistency and durability
107+
failure, no longer a memory-safety one. Detecting that specific case still
108+
requires Tier 3.
109+
110+
## Upstream
111+
112+
Per #484's framing, upstream does not treat a crafted `data.mdb` as an attack
113+
surface, so this likely stays a py-lmdb patch. An unbounded write with no error
114+
raised is arguably severe enough to report anyway; undecided.
115+
116+
## Reproducers
117+
118+
`misc/freedb_repro.py` — self-contained, engine-selectable
119+
(`--engine 0` / `--engine 1`), following the `misc/md_pad_repro.py` house
120+
style. It builds a normal database, locates its freeDB IDL, forges only the
121+
record bytes, and exercises each case in its own subprocess so a crash is
122+
isolated and its signal is visible. On current master both engines report 4 of
123+
5 cases not refused:
124+
125+
| case | result |
126+
| --- | --- |
127+
| out-of-range pgno, `writemap=False` | committed silently; `data.mdb` grows 237,568 -> 409,653,248 bytes |
128+
| out-of-range pgno, `writemap=True` | SIGSEGV |
129+
| unvalidated IDL count, `writemap=False` | SIGBUS |
130+
| unvalidated IDL count, `writemap=True` | `MDB_CORRUPTED` (incidental downstream wrong-page-type check) |
131+
| duplicate pgno, `writemap=False` | SIGABRT (`mdb_page_dirty` assertion) |
132+
133+
A minimal C reproducer and an OpenLDAP-format bug report have not been prepared.

‎docs/verifier-plan.md‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
# Plan: offline verifier (verify-then-trust)
2+
3+
Status: **planned**. This records a decision already reached; it introduces no
4+
new analysis. It is the counterpart to the runtime-hardening work tracked
5+
separately — see `freedb-record-validation.md` for the Tier 0 defects that
6+
hardening closes.
7+
8+
## Decision: reframe the threat model
9+
10+
No per-transaction check is 100% forge-proof short of revalidating the entire
11+
database on every open. Against an adversary, detection coverage is
12+
weakest-link: a 99% semantic detector buys nothing, because the attacker simply
13+
routes through the uncovered 1%. Chasing the freeDB residual — or any semantic
14+
forgery — inside a write transaction is therefore a losing game.
15+
16+
So we stop chasing per-transaction semantic detection and adopt **verify-then-trust**:
17+
18+
- Tell users, in user-facing documentation, not to open files they do not
19+
control.
20+
- Provide an offline tool that fully verifies a file. Once a file passes, the
21+
user may treat it as trusted.
22+
23+
Everything undetectable per-transaction is trivially detectable at rest, because
24+
at rest there is no legitimate dirty state to be ambiguous about: a reachable
25+
page carrying a dirty marker, a live page listed in the freeDB, an `mp_txnid`
26+
above the committed meta's — each is simply invalid for a verifier with a global
27+
view. The online problem is hard because of ambiguity that does not exist
28+
offline.
29+
30+
This retires the in-engine semantic #484 fixes previously proposed (the 0.9
31+
dirty-list sort and the 1.0 allocation invariant). Those are exactly the
32+
per-transaction semantic-detection layer this reframe replaces; the 1.0 one was
33+
already carrying the freeDB caveat that motivated the reframe.
34+
35+
## Two layers, crisp identities
36+
37+
- **Engine (runtime hardening, worked separately):** guarantees no crash, no
38+
out-of-bounds access, no unbounded write — in O(1) per guard, constant-time
39+
and side-effect-free, at point of use. Anything requiring iteration over
40+
file-derived data does *not* belong here.
41+
- **`verify` (this plan):** owns every global invariant and everything that
42+
requires looping over the file.
43+
44+
The read-hardening series is the floor `verify` stands on: opening an arbitrary
45+
file to inspect or verify it must itself be survivable. The reframe does not
46+
make that series a mistake — it is the precondition for the tool.
47+
48+
## Deliverable shape: a verb, not a policy
49+
50+
- `python -m lmdb verify <path>` subcommand in `lmdb/tool.py`, and/or an
51+
`Environment.verify()` method.
52+
- An offline audit someone runs on a file they do not trust — **not** an
53+
open-time flag that changes how every caller behaves. This is the "untrusted
54+
file" surface #484 wanted to avoid, reframed as an action rather than a mode.
55+
- Being Python-side, it covers `LMDB_PURE`, system-lib, and CFFI builds, which
56+
the patch series never reaches.
57+
- Precedent: SQLite's `PRAGMA integrity_check`, BerkeleyDB's `db_verify`.
58+
Verify-then-trust is the established model for file-format trust.
59+
60+
## What "fully verify" must check (invariant catalog)
61+
62+
"Fully verify" must mean *every invariant the write paths assume*, or the tool
63+
licenses trust the engine does not actually get — and the 99% problem reappears
64+
one level up, now stamped as approved. At minimum:
65+
66+
- Meta-page selection matching each engine's open semantics.
67+
- Full reachability: main DB, every `F_SUBDATA` sub-DB, dup sub-trees, overflow
68+
chains, and the freeDB itself — marking a bitmap of live pages.
69+
- The structural bounds the read patches already enforce (`mp_lower`/`mp_upper`,
70+
LEAF2 pad, etc.).
71+
- Key ordering and `md_entries` / `md_*_pages` consistency. The write paths
72+
assume a sorted tree; an unsorted tree that otherwise "verifies" is still
73+
hostile.
74+
- No reachable dirty markers: on 0.9, no reachable `P_DIRTY`; on 1.0, no
75+
reachable page with `mp_txnid` greater than the committed meta's txnid (this is
76+
the #484 residual, trivially caught offline).
77+
- freeDB rules: the structural pass (strictly descending, in range, count
78+
matches record size) plus the global check — the freeDB set and the live set
79+
must be disjoint and together cover `[NUM_METAS, next_pgno)`.
80+
- Crash-consistency aware: a post-crash file legitimately contains unreachable
81+
garbage and freeDB entries for pages a dead transaction touched, and must
82+
still pass against the committed meta.
83+
84+
## Operational caveats (document these, or the model is unsound)
85+
86+
- **TOCTOU:** verify a private copy and ensure no untrusted writer can touch the
87+
file afterward, or verification proves nothing.
88+
- **Lock file:** discard and regenerate `lock.mdb` rather than trusting one that
89+
arrived with the data file.
90+
- **Co-writers:** "trusted" extends to every process that holds write access
91+
from then on.
92+
93+
## Design decisions to make deliberately
94+
95+
- **Verifier trust base:** a pure-Python byte-walker cannot be owned by hostile
96+
input but is slow on huge maps; an engine-backed walk is fast but spends the
97+
read-hardening budget as its safety margin. Pick with eyes open.
98+
- **Fuzz harness:** mutate files, filter them through `verify`, then exercise the
99+
engine — to validate the central claim ("passes verify ⇒ engine will not crash
100+
or corrupt on it") and keep the verifier honest against upstream drift over
101+
time.
102+
- **Cost:** O(database pages) plus a bitmap of `next_pgno` bits — ~32 MB for a
103+
1 TB database at 4 K pages. Inherently not a per-transaction check.
104+
105+
## Documentation changes
106+
107+
- **Express the threat model in user-facing documentation for the first time.**
108+
It is currently only implicit in patch notes. State it as a layered scope:
109+
- Reads of arbitrary files will not crash or corrupt the process (hardened,
110+
best-effort).
111+
- The semantic honesty of a file is a trust property, established by provenance
112+
or by `verify`.
113+
- The read-hardening series is the floor this stands on.
114+
- Document the operational caveats above alongside the `verify` tool.
115+
- Update `freedb-record-validation.md`: Tier 3 becomes this plan; note Tier 0 is
116+
handled in the runtime-hardening path.
117+
- `ChangeLog` entry when it lands.
118+
119+
## Relationship to issue #484
120+
121+
Close #484 by documentation plus a pointer to `verify`, and drop the in-engine
122+
#484 fixes. The freeDB residual that survived the online approach collapses
123+
offline: a live page named in a freeDB record is simply a failed
124+
disjoint-and-cover assertion.
125+
126+
## Out of scope for this plan
127+
128+
- **Tier 0 runtime hardening** — the other path, worked now (loop-free,
129+
constant-time guards; see `freedb-record-validation.md`).
130+
- **Tier 2 page accounting** — skipped; it catches lazy forgeries, not
131+
deliberate ones, and trusts `md_*_pages`, which the attacker controls.

‎lib/py-lmdb/PATCH-STATUS.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,57 @@ byte-identical to pristine 0.9.36 across `mtest` through `mtest5`.
129129
130130
## Resolved
131131
132+
### `validate-freedb-record` — unvalidated free-database record structure
133+
134+
`mdb_page_alloc()` reads a free-DB record with `mdb_node_read` and trusts it
135+
whole. Two structural fields were never checked, on either engine:
136+
137+
- **Element count.** `i = idl[0]` is taken as the number of page numbers in
138+
the record without comparing it to the node size. A forged count drives
139+
`mdb_midl_need()` and `mdb_midl_xmerge()` past the end of the node —
140+
SIGBUS reading past the mapping, and garbage page numbers merged into
141+
`me_pghead`.
142+
- **Page numbers.** The reuse path does `pgno = mop[i]; goto search_done;`,
143+
jumping *past* the `pgno + num >= me_maxpg` bound that guards only the
144+
fresh-page path. So a page number taken from a record is never
145+
range-checked: under `MDB_WRITEMAP` the `me_map + psize*pgno` store lands
146+
outside the map (SIGSEGV); without it, `mdb_page_flush()` pwrites at
147+
`pgno*psize`, an attacker-controlled 64-bit file offset, committing
148+
silently and growing the file without bound.
149+
150+
A third, milder case: a record listing the same page twice makes
151+
`mdb_mid2l_insert` return a duplicate error, which `mdb_page_dirty()`
152+
funneled into `mdb_tassert` → `abort()` — a DoS where `MDB_CORRUPTED`
153+
belongs.
154+
155+
The patch adds three constant-time checks, no new loop over file data:
156+
157+
1. In `mdb_page_alloc`, before `idl[0]` is trusted, require
158+
`data.mv_size` to be a non-zero multiple of `sizeof(MDB_ID)` equal to
159+
`(idl[0]+1)*sizeof(MDB_ID)`. `mv_size` is read before `idl[0]` so the
160+
count read itself cannot run off a short node.
161+
2. At `search_done`, for a reused page number (`i != 0`), require
162+
`pgno` in `[NUM_METAS, mt_next_pgno)` with `num` pages of room; the
163+
clause order keeps the subtraction from wrapping. `mt_next_pgno` is a
164+
tighter and always-valid bound than `me_maxpg` — a freed page is by
165+
definition already allocated.
166+
3. `mdb_page_dirty()` returns `MDB_CORRUPTED` instead of asserting, and its
167+
two callers (`mdb_page_alloc`, `mdb_page_unspill`) propagate it.
168+
169+
These are **structural** checks only (the "Tier 0" of the freeDB analysis).
170+
A coherent forgery that lists a genuinely live page *in range* still passes
171+
here and is left to an offline reachability verifier; that residual is what
172+
narrows, but does not close, issue #484's free-DB caveat. Because a freed
173+
page number now reaches the allocator guaranteed real and in-range, it can
174+
no longer be the memory-safety half of that caveat.
175+
176+
Regression tests are in `tests/cve_test.py` (`FreeDbRecordTest`), engine
177+
agnostic so they run on both trees, and including
178+
`test_normal_freelist_reuse_still_works`, a positive test that passes with
179+
the patch removed — it guards against false rejection of a valid record.
180+
Each of the four forgery tests crashes (SIGBUS/SIGSEGV/SIGABRT) or commits
181+
silently with the patch removed, and returns `MDB_CORRUPTED` with it.
182+
132183
### `validate-ovpage-free` — unbounded overflow extent on the delete path
133184
134185
`mdb_ovpage_free()` takes the extent from the page header

0 commit comments

Comments
 (0)