Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved double-encoded traversal risk and missing regression coverage must be addressed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens IPFS/IPNS gateway rewriting against decoded .. path traversal.
Changes:
- Validates decoded path segments.
- Rejects traversal attempts with
CURLE_URL_MALFORMAT.
File summaries
| File | Summary |
|---|---|
src/tool_ipfs.c |
Adds dot-segment validation during IPFS/IPNS gateway rewriting. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| /* reject a decoded ".." segment so the request cannot escape the gateway's | ||
| content namespace (result stays CURLE_URL_MALFORMAT) */ | ||
| if(inputpath && has_dotdot_segment(inputpath)) |
|
|
||
| /* reject a decoded ".." segment so the request cannot escape the gateway's | ||
| content namespace (result stays CURLE_URL_MALFORMAT) */ | ||
| if(inputpath && has_dotdot_segment(inputpath)) |
|
it struck me that curl should probably just not URL decode the path (nor any other parts) when extracted from the URL, so that it builds the new URL with already URL encoded parts. |
7061b86 to
3d5752c
Compare
|
went with your suggestion. it doesn't decode the path parts anymore, just builds the new url from the still-encoded cid, gateway path and user path and sets the path without re-encoding, so a %2f stays %2f and never becomes a separator. that also kills the double-encoded %252e%252f variant copilot pointed at, and a plain ../ still gets collapsed by the parser at parse time so it resolves inside the namespace like before. i left scheme/host/port alone since they come from the gateway and get set as their own url parts, no attacker input rides through them, but can make it uniform if you'd rather. checked single and double encoded ipfs+ipns stay under the /ipfs|ipns// prefix, normal paths and a literal admin/config are unchanged, 20 ipfs tests still pass. no test in the PR yet, couldn't find a free number near the 72x ones, tell me which to use and i'll add one that checks the encoded path survives. |
3d5752c to
834af0f
Compare
1rhino2
changed the title
|
added tests 1688 and 1689 (ipfs and ipns) since those slots were free, they both fail on master with the request going to /admin/config and pass with the change. also updated the title and description, the old ones still described the reject-dotdot version. |
|
Ah, it looks as if something else stole those test numbers in the mean time. Can you move 1688 and 1689 to other numbers? |
The IPFS/IPNS rewrite fetched the user path (and the CID and gateway path) with CURLU_URLDECODE and then re-encoded the assembled path with CURLU_URLENCODE. The decode turned an encoded slash (%2f) into a real separator that the URL parser's earlier dot-segment cleanup never saw, so a ".." could climb back out of the /<protocol>/<cid> namespace when the final URL was parsed. A double-encoded form (%252e%252f) slipped through the same way. Build the new URL from the still-encoded parts instead: get the CID, the gateway path and the user path without decoding, and set the combined path without re-encoding. A percent-encoded slash then stays encoded, so no new separators appear and the later normalization has nothing extra to remove. A literal "../" in the user path is still collapsed by the parser when the ipfs:// URL is first parsed, so it resolves within the namespace as before. Add tests 1691 (ipfs) and 1692 (ipns) that send an encoded-slash dotdot path through a gateway with a base path and verify the request stays under /<gateway>/ipfs|ipns/<cid>/. Both fail on the previous code with a request for /admin/config. Reported-by: 1rhino2 on hackerone
834af0f to
1b10ed2
Compare
|
moved them to 1691 and 1692, next free ones after the block. rebased on master too so the conflict is gone. both still fail on the old code (request goes to /admin/config) and pass with the fix, and 1688/1689 from master are untouched. |
|
Thanks! |
The IPFS/IPNS rewrite pulls the user path (and the CID and the gateway path)
with
CURLU_URLDECODE, drops it after the gateway base and the/ipfs/<cid>(or
/ipns/<name>) part, and re-encodes the result withCURLU_URLENCODE.The decode happens after the URL parser already did its own dot-segment
cleanup, so an encoded slash (
%2f) turns into a segment separator too late tobe cleaned, and a
..then climbs back out of the namespace when the final URLis parsed.
So e.g.
ipfs://<cid>/%2f..%2f..%2f..%2f..%2f..%2fadmin/configwith a gatewayof
http://host/private/gatewayends up requesting/admin/configon thatorigin instead of staying under
/private/gateway/ipfs/<cid>/.Fix (bagder's suggestion below): do not decode at all. Get the CID, gateway
path and user path still encoded and set the combined path without
re-encoding, so a
%2fstays%2fand never becomes a separator. That alsocovers the double-encoded
%252e%252fvariant, and a plain../is stillcollapsed by the parser when the ipfs:// URL is first parsed, so it resolves
inside the namespace like before.
Tests 1688 (ipfs) and 1689 (ipns) send the encoded dotdot path through a
gateway with a base path and check the request stays under
/private/gateway/ipfs|ipns/<cid>/. Both fail on master with a request for/admin/config. The existing IPFS tests 722-741 still pass, checksrc is clean.Reported via HackerOne (#4027511).