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

[Security] Even more strict on address bar for IPFS #13873

Closed
bbondy opened this issue Feb 1, 2021 · 8 comments · Fixed by brave/brave-core#8328
Closed

[Security] Even more strict on address bar for IPFS #13873

bbondy opened this issue Feb 1, 2021 · 8 comments · Fixed by brave/brave-core#8328

Comments

@bbondy
Copy link
Member

bbondy commented Feb 1, 2021

This issue #13872 makes it so we only use ipns:// and ipfs:// for configured gateways.

This issue is to track being even more strict and only replacing ipfs:// when there is a valid CID.

This is about the reverse lookup code which will show ipfs://
We only want to do that when it's a valid CID.
For .ipfs.localhost:

If is not in the right format, we shouldn't show ipfs:// reverse mapping

@lidel
Copy link

lidel commented Mar 23, 2021

CID spec is at:

TLDR is that we have CIDv0 (legacy, implicit base, version and codec) and CIDv1 (future-proof, with explicit base encoding, version number and codec).

CIDv0 is not safe for use in browser context due to being case-sensitive (base58 breaks when force-lowercased on subdomains), so our gateway converts it to CIDv1 in Base32 (/ipfs/{cid}) or base36 (/ipns/{libp2p-key}) before loading it via subdomain.

How to implement validation? I see two ways:

  • (A) Brave implements CID spec and checks if string is a valid CID
    • downside is that you would have to implement support for base58, base32 and base36 at minimum (those are the most popular "safe" defaults used by go-ipfs in various contexts)
  • (B) Brave implements naive heuristic, some ideas:
    • check if characters are from multibase pool
      • eg. if first character of CID is Qm (CIDv0) or a valid multibase prefix ("code" character from multibase.csv)
    • this should be enough for weeding-off false-positives. full CID validation would happen at go-ipfs anyway
  • (C) Brave delegates validation to go-ipfs by sending HTTP HEAD request to /ipfs/{cid} and checking if its not an error >=400
    • requires running go-ipfs node or using public gateway

I suggest doing (B) first. Good for now, does not require maintenance and validation function can always update to full one if we decide its necessary.

@bbondy
Copy link
Member Author

bbondy commented Mar 23, 2021

agreed on B) for now

@stephendonner
Copy link

@spylogsster mind adding a testplan for this? Thanks!

@spylogsster
Copy link

Test Plan:

@stephendonner
Copy link

Thanks, @spylogsster - quick question:

For the last bullet point there, I did the following:

  • grabbed the CID from ipfs://QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Tokyo_National_Museum.html (which retrieves and redirects to ipfs://bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq/wiki/Tokyo_National_Museum.html) as QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco
  • tried to load https://QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco.ipns.localhost:48081/

I got an error, though; am I grabbing the right [CID]? Thanks in advance!

Screen Shot 2021-03-26 at 1 10 37 PM

@spylogsster
Copy link

@stephendonner QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco should be resolved first, use this as ipfs://QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco

@stephendonner
Copy link

stephendonner commented Mar 30, 2021

Verified PASSED using the first portion of the inline testplan.

Steps:

  1. new profile
  2. loaded ipns://en.wikipedia-on-ipfs.org to get interstitial page
  3. clicked on Use local node
  4. waited for installation
  5. loaded http://brantly.eth.ipns.localhost:48081

Verified the URL bar retained it as such.

Screen Shot 2021-03-29 at 10 16 10 PM

Screen Shot 2021-03-29 at 10 16 25 PM


Verification passed on

Brave 1.24.66 Chromium: 90.0.4430.72 (Official Build) beta (64-bit)
Revision b6172ef8d07ef486489a4b11b66b2eaeed50d132-refs/branch-heads/4430@{#1233}
OS Ubuntu 18.04 LTS

Verified the above test plan
image


Verified PASSED using the first portion of the inline testplan, with build

Brave 1.24.82 Chromium: 90.0.4430.93 (Official Build) (64-bit)
Revision 4df112c29cfe9a2c69b14195c0275faed4e997a7-refs/branch-heads/4430@{#1348}
OS Windows 10 OS Version 2009 (Build 21370.1)

Steps:

  1. new profile
  2. loaded ipns://en.wikipedia-on-ipfs.org to get interstitial page
  3. clicked on Use local node
  4. waited for installation
  5. loaded http://brantly.eth.ipns.localhost:48084

Verified the URL bar retained it as such.

example example
interstitial almonit

@lidel
Copy link

lidel commented Mar 30, 2021

@stephendonner FYSA http://Qm.. fail because Qm.. CIDv0 are case-sensitive, and the authority part of URL is case-insensitive in some web contexts (eg. DNS / Firefox force-lowercase)
This is not an issue with Brave, but a quirk of representing CIDs on the web.
That is why we are moving towards case-insensitive base32 CIDv1 (bafy...) identifiers (and why there is a redirect returned by go-ipfs, that does it automatically under the hood).

@LaurenWags LaurenWags changed the title Even more strict on address bar for IPFS [Security] Even more strict on address bar for IPFS May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment