-
Notifications
You must be signed in to change notification settings - Fork 1
fix: bound request bodies and validate UnifiedPush endpoints against SSRF #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1e5b3ec
1700267
a1e1aae
8d3232c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,7 @@ Request: | |
| | Field | Type | Description | | ||
| |-----------------|--------|------------------------------------------------------------------------------------------------------------------------| | ||
| | `trade_pubkey` | string | 64 hex characters | | ||
| | `token` | string | FCM device token, or UnifiedPush endpoint URL | | ||
| | `token` | string | FCM device token, or UnifiedPush endpoint URL. Non-empty, at most 4096 bytes. If it parses as an `http`/`https` URL it must be `https` and point at a public address (see below). | | ||
| | `platform` | string | `"android"` or `"ios"` | | ||
| | `mostro_pubkey` | string | 64 hex characters. Optional on the wire; required when the trusted-instance whitelist is non-empty (see below). | | ||
|
|
||
|
|
@@ -226,10 +226,86 @@ curl -i -X POST http://localhost:8080/api/notify \ | |
| |--------|-------------------------------------------------------------------------------| | ||
| | 200 | `/api/health`, `/api/info`, `/api/status`, `/api/register`, `/api/unregister` | | ||
| | 202 | `/api/notify` on parse-valid input | | ||
| | 400 | Malformed body, invalid `trade_pubkey`, invalid `platform`, empty `token` | | ||
| | 400 | Malformed body, body over the size limit, invalid `trade_pubkey`, invalid `platform`, empty or oversized `token` | | ||
| | 429 | `/api/register`, `/api/unregister`, `/api/notify` rate limits | | ||
| | 500 | Rate-limited endpoints fail closed when the per-IP key cannot be extracted | | ||
|
|
||
| ### Push endpoint validation | ||
|
|
||
| The `token` field is overloaded: for FCM it is an opaque registration token, | ||
| for UnifiedPush it is the URL the server will POST to. The request carries no | ||
| field saying which, so the server inspects the value instead. | ||
|
|
||
| A token that parses as an `http` or `https` URL is treated as a push endpoint | ||
| and must satisfy all of: | ||
|
|
||
| - scheme is `https` | ||
| - the host is not a private, loopback, link-local, CGNAT, or otherwise | ||
| non-routable address, including the IPv4-mapped IPv6 spellings of those | ||
| (`https://[::ffff:169.254.169.254]/`) | ||
|
|
||
| Anything that does not parse as an `http`/`https` URL is treated as an opaque | ||
| backend token and passed through untouched, so FCM registrations are | ||
| unaffected. A short list of clearly unusable schemes (`file`, `ftp`, `gopher`, | ||
| `data`, `dict`, `ldap`) is refused outright. | ||
|
Comment on lines
+247
to
+250
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Make the opaque-token exception explicit. Lines 247-250 state that every value which does not parse as HTTP(S) passes through unchanged, but the next sentence rejects 🤖 Prompt for AI Agents |
||
|
|
||
| Rejection — `400 Bad Request`: | ||
|
|
||
| ```json | ||
| { | ||
| "success": false, | ||
| "message": "Invalid push endpoint" | ||
| } | ||
| ``` | ||
|
|
||
| The message is identical for every rejection reason on purpose. A caller must | ||
| not be able to use the response to distinguish "unsupported scheme" from | ||
| "internal address" and map the server's network. | ||
|
|
||
| Registration performs the checks above without touching the network. The | ||
| authoritative check runs again immediately before the outbound POST and | ||
| additionally resolves domain hosts, refusing the endpoint if any resolved | ||
| address is non-public. | ||
|
|
||
| ## Request size limits | ||
|
|
||
| Every endpoint that accepts a body caps it. Actix's own default is 2 MB, which | ||
| on unauthenticated endpoints is a free memory-amplification primitive. | ||
|
|
||
| | Endpoint | Max body | Notes | | ||
| |-------------------|----------|--------------------------------------------------------------| | ||
| | `/api/register` | 8 KiB | Sized to fit a 4096-byte `token` plus the other fields | | ||
| | `/api/unregister` | 1 KiB | Body carries a single 64-char hex pubkey | | ||
| | `/api/notify` | 1 KiB | Body carries a single 64-char hex pubkey | | ||
|
|
||
| The `token` field of a registration is bounded separately at **4096 bytes**. The | ||
| body cap stops an enormous request; the field cap stops a merely large one from | ||
| being retained in the in-memory token store for its whole TTL. | ||
|
|
||
| Exceeding either limit is reported as `400 Bad Request`, **not** `413 Payload | ||
| Too Large`: | ||
|
|
||
| ```json | ||
| { | ||
| "success": false, | ||
| "message": "Request body too large" | ||
| } | ||
| ``` | ||
|
|
||
| ```json | ||
| { | ||
| "success": false, | ||
| "message": "Token exceeds maximum length" | ||
| } | ||
| ``` | ||
|
|
||
| Returning `400` rather than `413` is deliberate. The response bodies of | ||
| `/api/register` and `/api/unregister` are frozen against pre-1.1 fixtures, and | ||
| `/api/notify` is contractually restricted to a single failure status, so the | ||
| size cap reuses the shape those endpoints already emit instead of introducing a | ||
| new one. Only the payload-overflow case is remapped; every other body-parsing | ||
| failure keeps its previous behaviour. | ||
|
|
||
| ## Rate limiting | ||
|
|
||
| `/api/register` and `/api/unregister` share a per-IP limit to protect the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,11 +38,39 @@ The endpoint store is loaded once at startup. Failures to read or parse the file | |
|
|
||
| If `UNIFIEDPUSH_ENABLED=false`, the service is not added to the dispatcher slice. Existing entries in `data/unifiedpush_endpoints.json` are ignored at runtime but not deleted. | ||
|
|
||
| ## Endpoint validation | ||
|
|
||
| The registered device token *is* the URL the server POSTs to, which makes it a | ||
| request-forgery surface reachable from the unauthenticated `/api/register` and | ||
| `/api/notify` pair. `src/push/endpoint_guard.rs` is the single place that | ||
| decides whether an endpoint may be contacted. | ||
|
|
||
| Two passes: | ||
|
|
||
| 1. **Registration** — static, no network. Refuses non-`https` schemes and hosts | ||
| that are IP literals outside the public internet. | ||
| 2. **Dispatch** — runs immediately before the outbound POST, repeats the static | ||
| checks, and resolves domain hosts, refusing if *any* resolved address is | ||
| non-public. This is the authoritative gate; registration is defence in | ||
| depth and fast feedback. | ||
|
|
||
| The guard only ever inspects the **first hop**, which is why this backend does | ||
| not use the shared HTTP client. `reqwest` follows up to 10 redirects by | ||
| default, so a registered endpoint answering `302 Location: http://169.254.169.254/` | ||
| would walk the request past the guard entirely. `UnifiedPushService::build_client` | ||
| refuses redirects outright: a push endpoint has no legitimate reason to issue | ||
| one. A regression test asserts the second hop is never requested. | ||
|
|
||
| Known limitation: `reqwest` resolves the host again when it connects, so a DNS | ||
| record with a very short TTL can change between validation and connection. | ||
| Closing that race requires pinning the validated address into the connection; | ||
| tracked in [#39](https://github.com/MostroP2P/mostro-push-server/issues/39). | ||
|
|
||
| ## Operational notes | ||
|
|
||
| - UnifiedPush has no per-payload distinction between silent and visible push. `send_silent_to_token` falls back to `send_to_token`, which is the same code path the Nostr listener uses. | ||
| - There is no rate limiting on outbound UnifiedPush calls beyond what the server-wide `reqwest::Client` timeouts provide (2 s connect, 5 s total). | ||
| - The endpoint URL is fully attacker-controlled in the sense that the distributor can be any HTTP server. The shared `reqwest::Client` enforces TLS and the timeouts; the server does not pin certificates or restrict hostnames. | ||
| - The endpoint URL is fully attacker-controlled in the sense that the distributor can be any HTTP server. A dedicated `reqwest::Client` (`UnifiedPushService::build_client`) enforces TLS, the timeouts, and a no-redirect policy; the server does not pin certificates. | ||
|
Comment on lines
72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -A25 -B10 'build_client|Client::builder|connect_timeout|timeout|redirect' \
src/push/unifiedpush.rs src/main.rs
rg -n -A20 -B10 'validate_endpoint|https|is_non_public' \
src/push/endpoint_guard.rsRepository: MostroP2P/mostro-push-server Length of output: 30183 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- docs/unifiedpush.md ---'
sed -n '58,80p' docs/unifiedpush.md
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/mostrop2p-mostro-push-server-3db7dddf -maxdepth 2 -type f -name '*.md' -print \
| sort \
| while read -r f; do
case "$f" in
*/learnings/*) ;;
*) printf '%s\n' "### $f"; cat "$f" ;;
esac
done
printf '%s\n' '--- relevant learnings ---'
find /tmp/coderabbit-repo-knowledge/mostrop2p-mostro-push-server-3db7dddf/learnings -maxdepth 1 -type f -name '*.md' -print 2>/dev/null \
| sort \
| while read -r f; do
printf '%s\n' "### $f"
cat "$f"
doneRepository: MostroP2P/mostro-push-server Length of output: 2102 Describe timeout ownership accurately.
🤖 Prompt for AI Agents |
||
|
|
||
| ## Reference | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the invalid-endpoint rejection to the 400 summary.
Line 229 omits the new
"Invalid push endpoint"response documented at Lines 252-259. Add this case to the summary and to the registration validation-error list so all documented 400 outcomes are discoverable.🤖 Prompt for AI Agents