Security fixes & hardening: injection, open redirect, some DoS mitigations - #216
Open
fschwebel wants to merge 8 commits into
Open
Security fixes & hardening: injection, open redirect, some DoS mitigations#216fschwebel wants to merge 8 commits into
fschwebel wants to merge 8 commits into
Conversation
Lines longer than HIGHLIGHT_LINE_LENGTH_CUTOFF take a fast path that
skips highlighting. That path returned the raw source line, which is
then concatenated into the page and rendered with `{{ html|safe }}`, so
a paste containing one line over the cutoff injected arbitrary markup
into the page every viewer loads.
Escaping is what the highlighter's normal path does via
line_tokens_to_classed_spans; the cutoff was only ever meant to skip
tokenising, not escaping. Run the line through the existing escape()
helper instead.
The Content Security Policy prevents this from becoming XSS, but it does
not cover `meta refresh`, so the practical impact was navigating every
viewer of the paste to an arbitrary site, plus full control of the page
via a `data:` stylesheet.
As a side effect this also stops a long line containing literal `<span`
or `</span>` text from unbalancing the surrounding markup, since
open_span_prefix now never sees tags it did not emit.
Both templates were declared `escape = "none"`, and askama resolves
inheritance into the child, so escaping was also off for every block
they inherit from paste.html and base.html. Three sinks followed:
* /qr/{id} rendered the paste title unescaped (stored);
* /burn/{id}.{ext} put the unvalidated URL "extension" inside an href,
and renders before touching the database, so a made-up id worked;
* /qr/{id}.{ext} put the same value into data-paste-id.
qr.html only interpolates integers and UI strings, so it just gets
escaping back. burn.html genuinely needs raw markup for one translated
literal, so it follows the pattern burn-confirmation.html already uses:
escaping on, `safe` applied to the trusted string only. Since that
string interpolates an untrusted value, t_with() now escapes what it
substitutes.
The Content Security Policy kept this short of XSS, but `meta refresh`
is not covered by any directive, so a single crafted /burn/ link
redirected the visitor anywhere.
Adds regression tests for all three sinks and for t_with().
The `Uids` extractor used `type Rejection = ()`, and in axum `()` is a valid response: an empty `200 OK`. So a DELETE whose `uid` cookie was missing, forged, or signed with a key the server no longer holds never reached the handler and the caller was told the delete succeeded — while the paste stayed online. With `WASTEBIN_SIGNING_KEY` unset (the default) a fresh key is generated at every restart, so ownership cookies stop verifying after each restart, making this the common case rather than an edge one. Make the extractor yield an empty uid list instead of failing. An empty list already means "no ownership proven": `Database::delete_for` returns `Error::Delete` for it, which both delete routes render as 403 in their own format (JSON for the API route, an HTML error page for the form route). The optional `Uids` extractor used by the read handlers is unaffected — an empty list yields `can_delete = false` exactly as a missing cookie did before. Adds regression tests covering a missing cookie and a forged cookie on both delete routes.
Syntax highlighting wraps every line and token in markup, so a pathological paste (e.g. a megabyte of newlines) renders to tens of megabytes of HTML, and that result is cached. With the default cache of 128 entries, worst-case resident memory was in the multiple-gigabyte range — reachable with a handful of ordinary-sized requests. Refuse to cache any render larger than 1 MiB, bounding worst-case cache memory to roughly WASTEBIN_CACHE_SIZE * 1 MiB. A paste whose highlighted form exceeds the default request-body limit is unusual enough to re-render on each view rather than let one entry dominate the cache; correctness is unchanged either way. This does not address a related lower-severity issue: because the cache key includes the URL extension, one paste fetched under many extensions still occupies many slots. With per-entry size now bounded that is a cache-eviction concern, not a memory-exhaustion one, and is left as is. Adds `Html::len`/`is_empty` and a test that an oversized render is not cached.
Every password attempt derives an Argon2 key, and the configuration reserves 64 MiB and keeps several cores busy per hash. Each derivation runs on the tokio blocking pool via `spawn_blocking`, which defaults to 512 threads, so a burst of unauthenticated attempts (e.g. wrong-password reads of an encrypted paste) could pin the machine's memory and CPU with no bound of its own. Gate the hashing behind a semaphore sized to the core count, acquired before the blocking task and released after it, so concurrency is capped independently of the blocking pool. This covers both encryption and decryption, the only two Argon2 call sites. Adds a concurrency test that more round-trips than there are permits all complete, guarding against a leaked permit or deadlock.
The magic-link handoff redirected to `format!("/{id}")` using the raw,
percent-decoded path parameter before the id was validated. A value such
as `%5Cevil.example.com` decodes to `/\evil.example.com`, which a browser
normalises to `//evil.example.com` — an off-site redirect. An attacker
mints the required owner token by creating a paste of their own.
Parse the id first and build the redirect from the canonical `Key`. A
value that is not a valid paste id no longer triggers the handoff at all;
a valid one always begins with an id character (never `/` or `\`), so the
target stays same-origin. Adds a regression test.
SafeReferer guarded the relative-referer branch against `//` but not `/\`, and the absolute-URL branch handed `url.path()` straight to Redirect even though a path can itself begin with `//`. Either produced a scheme-relative Location that leaves the origin: a page at a `//`-path with `Referrer-Policy: unsafe-url` linking to `/theme` would bounce the visitor off-site. Route both branches through a single `safe_local_redirect` that only accepts a single-slash absolute path (rejecting `//` and `/\`) and otherwise falls back to `/`. Adds tests for the absolute `//`-path and the `/\` relative referer.
WASTEBIN_PASTE_EXPIRATIONS only populated the index-page dropdown; both insert paths accepted any NonZeroU32, or none at all, so an operator who configured a mandatory retention policy did not actually get one. Add Page::allows_expiration and check the requested value (0 meaning "no expiration") against the configured set on both the JSON and form insert handlers, rejecting anything outside it with 400 via a new Error::IllegalExpiration. Adds tests on both routes.
Author
|
Follow-up: three more fixes pushed, addressing open redirects and enforcing the expiry
Also Fable. |
fschwebel
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes five security issues, one commit each with regression tests. 10 files,
+348/-17, 11 new tests.
cargo test,clippy -Dwarnings, andfmt --checkpass.highlight.rs) — lines over 2048 bytesskipped escaping and were rendered via
{{ html|safe }}, injecting stored markupinto every viewer's page. Now escaped like every other line.
qr.rs,burn.rs,i18n.rs,burn.html) — both templates usedescape = "none", exposing the paste title andthe unvalidated URL extension (the
/burn/{id}.{ext}sink needs no real paste).Escaping restored;
t_withnow escapes its substituted value.extract.rs,delete/*) — theUidsextractor rejected with
(), i.e. an empty200 OK, so a DELETE with a missing orforged cookie reported success while the paste survived (the default random signing
key makes this the common case after any restart). Now a proper 403 via
delete_for.cache.rs) — a pathological paste rendersto tens of MB and was cached across 128 slots (multi-GB worst case). Oversized
renders are no longer cached.
crypto.rs) — each password attemptreserves 64 MiB on the 512-thread blocking pool; concurrency is now capped at the
core count, covering both encrypt and decrypt.
Made with Claude Fable 5