-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
gateway: harden path prefix #1988
Conversation
log.Debugf("X-Ipfs-Gateway-Prefix: %s", prefixHdr[0]) | ||
prefix = prefixHdr[0] | ||
prfx := prefixHdr[0] | ||
if strings.HasPrefix(prfx, "/") { |
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.
Maybe path.Clean
? Not sure
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.
Clean("http://foobar.com/") = "http:/foobar.com"
:(
I think starts-with-slash should be good enough, and is also simpler to specify (as in api spec) than the specifics of a golang stdlib function.
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.
sounds good
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.
what about paths like:
//foo.com/bar/baz
valid http path that might point elsewhere
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.
doing this in a header is particularly dangerous because this could come from a client, so malicious request could cause links to be served back to a user that point to some evil place. maybe this should be a config? not sure what the right + secure way to do this is.
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.
//foo.com/bar/baz
shit I didn't think about that :/
maybe this should be a config
A whitelist of allowed prefixes would work. Setting only one fixed prefix defeats the purpose, since the basic idea behind all this is to have the daemon exposed at multiple arbitrary URIs: ipfs.io/, ipfs.io/refs, etc.
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.
ah yeah.
@lgierth update here? |
No update but let's keep it open so that I don't forget. It's important (see labels) but I keep postponing it... |
@lgierth if you come to seattle i'm not going to let you leave until you merge this one :P |
dd7cddc
to
714b391
Compare
Updated to use a configurable allowlist of path prefixes: gateway: enforce allowlist for path prefixes The gateway accepts an X-Ipfs-Path-Prefix header, We don't want a potential attacker to be able to Previously, we'd accept any X-Ipfs-Path-Prefix header. Example: We mount blog.ipfs.io (a dnslink page) at ipfs.io/blog. nginx_ipfs.conf:
.ipfs/config:
dnslink:
|
Okay it does work as expected, I tested it the wrong manual way the other day.
|
The gateway accepts an X-Ipfs-Path-Prefix header, and assumes that it is mounted in a reverse proxy like nginx, at this path. Links in directory listings, as well as trailing-slash redirects need to be rewritten with that prefix in mind. We don't want a potential attacker to be able to pass in arbitrary path prefixes, which would end up in redirects and directory listings, which is why every prefix has to be explicitly allowed in the config. Previously, we'd accept *any* X-Ipfs-Path-Prefix header. Example: We mount blog.ipfs.io (a dnslink page) at ipfs.io/blog. nginx_ipfs.conf: location /blog/ { rewrite "^/blog(/.*)$" $1 break; proxy_set_header Host blog.ipfs.io; proxy_set_header X-Ipfs-Gateway-Prefix /blog; proxy_pass http://127.0.0.1:8080; } .ipfs/config: "Gateway": { "PathPrefixes": ["/blog"], // ... }, dnslink: > dig TXT _dnslink.blog.ipfs.io dnslink=/ipfs/QmWcBjXPAEdhXDATV4ghUpkAonNBbiyFx1VmmHcQe9HEGd License: MIT Signed-off-by: Lars Gierth <larsg@systemli.org>
714b391
to
09937f8
Compare
Fixed the tests -- filed #2528 about the CircleCI failure |
Yay circleci is green |
Looks good to me, nice to finally merge this :) |
gateway: harden path prefix This commit was moved from ipfs/kubo@c6e6bb0
See #1971 (comment) ff.
Only allow paths, so we stay local to this gateway,
and don't get tricked into linking to a different host,
or even redirecting to one.
I can't come up with a concrete attack right off the bat,
but who knows how things are going to change.
Not restricting it would be silly.
Edit: