fix: block cover downloads from private hosts and cap their size #19
Loading…
Reference in a new issue
No description provided.
Delete branch "jtk/cover-url-ssrf"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #6 (high): cover URLs from request params (
add_from_lookup,scan_barcode,batch_add) went straight toReq.get, so a user could make the server fetchlocalhost,169.254.169.254or LAN admin pages; anything that looked like an image was stored and served publicly.Stacked on #18 (critical #5). This branch includes #18's commit; the new change is the second commit,
7c140c8. Merge #18 first.Changes (
lib/fuzzy_catalog/storage.ex)::,::1, ULA (fc00::/7), ff00::/8, or an IPv4-mapped / NAT64 form of those. Returns{:error, :private_address}.redirect: false; redirects are followed by hand, at most 3, with the same check on every hop.retry: false,compressed: false, and the body is streamed with a 10 MB cap ({:error, :too_large}).store_synced_cover/1) passallow_private: true: those URLs are built from admin-configured Audiobookshelf/BookLore servers, which are usually on the LAN.:cover_download_req_optionsapp env lets tests plug inReq.Test.Known limit: DNS is resolved for the check and again by the HTTP client, so a DNS-rebinding host could still slip through. Pinning the connection to the checked IP needs custom connect options with SNI; I left that out for this release.
Tests
test/fuzzy_catalog/storage_test.exs: public host stored; 12 internal URLs refused without any request; redirect to the metadata IP refused; redirect chain followed and loop stopped; >10 MB body refused; synced LAN cover allowed;public_address?/1cases.mix precommit: 179 tests pass.Review by Nye (PR review / audit) — APPROVE
compressed: false, streamed 10 MB cap, 5s/10s timeouts andretry: falseall look right.allow_privateis only used bystore_synced_cover(admin-configured sync hosts), never on request input.0x7f000001etc.) are judged the same way they're connected.Non-blocking follow-ups (filed as one low issue):
public_address?treats IPv4-compatible::a.b.c.d(::/96, e.g.::7f00:1) as public; add{0,0,0,0,0,0,_,_}to the blocked set.a == 192 and b == 0blocks all of 192.0.0.0/16; only 192.0.0.0/24 and 192.0.2.0/24 are special. Harmless over-block.