Skip to content
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

Merged
merged 1 commit into from
Apr 4, 2016
Merged

gateway: harden path prefix #1988

merged 1 commit into from
Apr 4, 2016

Conversation

ghost
Copy link

@ghost ghost commented Nov 21, 2015

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:

  • make this a configurable set of allowed prefixes instead

@jbenet jbenet added the status/in-progress In progress label Nov 21, 2015
@ghost ghost added topic/gateway Topic gateway topic/security Topic security RFCR labels Nov 21, 2015
@daviddias daviddias mentioned this pull request Nov 21, 2015
53 tasks
log.Debugf("X-Ipfs-Gateway-Prefix: %s", prefixHdr[0])
prefix = prefixHdr[0]
prfx := prefixHdr[0]
if strings.HasPrefix(prfx, "/") {
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

Copy link
Member

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

Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah.

@rht
Copy link
Contributor

rht commented Nov 24, 2015

Here is a gateway yanked from the current dev0.4.0 (as of b4ab684).

@ghost ghost mentioned this pull request Nov 26, 2015
42 tasks
@ghost ghost added RFM and removed RFCR labels Nov 29, 2015
@ghost ghost removed the RFM label Dec 22, 2015
@ghost ghost mentioned this pull request Jan 11, 2016
@whyrusleeping
Copy link
Member

@lgierth update here?

@whyrusleeping whyrusleeping assigned ghost Jan 29, 2016
@ghost
Copy link
Author

ghost commented Feb 25, 2016

No update but let's keep it open so that I don't forget. It's important (see labels) but I keep postponing it...

@whyrusleeping
Copy link
Member

@lgierth if you come to seattle i'm not going to let you leave until you merge this one :P

@ghost ghost force-pushed the ipfs-prefix-hardening branch from dd7cddc to 714b391 Compare March 29, 2016 00:30
@ghost
Copy link
Author

ghost commented Mar 29, 2016

Updated to use a configurable allowlist of path prefixes:

gateway: enforce allowlist for path prefixes

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

@ghost
Copy link
Author

ghost commented Apr 4, 2016

Okay it does work as expected, I tested it the wrong manual way the other day.

  1. set Gateway.PathPrefixes = ['/blog'] in the config
  2. curl -v localhost:8080/static/ -H 'Host: blog.ipfs.io' -H 'X-Ipfs-Gateway-Prefix: /blog'
  3. Expect links of the form /blog/static/... in the resulting directory listing
  4. curl -v localhost:8080/static/ -H 'Host: blog.ipfs.io' -H 'X-Ipfs-Gateway-Prefix: //evil.com/blog'
  5. Expect untouched links of the form /static/...
  • fix unrelated tests

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>
@ghost ghost force-pushed the ipfs-prefix-hardening branch from 714b391 to 09937f8 Compare April 4, 2016 20:32
@ghost
Copy link
Author

ghost commented Apr 4, 2016

  • fix unrelated tests

Fixed the tests -- filed #2528 about the CircleCI failure

@ghost
Copy link
Author

ghost commented Apr 4, 2016

Yay circleci is green

@whyrusleeping
Copy link
Member

Looks good to me, nice to finally merge this :)

@whyrusleeping whyrusleeping merged commit c6e6bb0 into master Apr 4, 2016
@whyrusleeping whyrusleeping deleted the ipfs-prefix-hardening branch April 4, 2016 23:08
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
gateway: harden path prefix

This commit was moved from ipfs/kubo@c6e6bb0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress In progress topic/gateway Topic gateway topic/security Topic security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants