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

repo.gc - potentially insecure hash functions not allowed #5067

Open
alanshaw opened this issue Jun 1, 2018 · 9 comments
Open

repo.gc - potentially insecure hash functions not allowed #5067

alanshaw opened this issue Jun 1, 2018 · 9 comments
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@alanshaw
Copy link
Member

alanshaw commented Jun 1, 2018

Version information:

0.4.14

Type:

Bug

Description:

Low priority but I can't gc if I have a "insecure" hash:

$ ipfs repo gc
Error: "potentially insecure hash functions not allowed"
Please run 'ipfs pin verify' to list insecure hashes. If you want to read them, please downgrade your go-ipfs to 0.4.13
; garbage collection aborted: could not retrieve some links
$ ipfs pin verify
zDvnoLvmiom broken
  zDvnoLvmiom: potentially insecure hash functions not allowed
@Stebalien Stebalien added kind/bug A bug in existing code (including security flaws) status/duplicate This issue or pull request already exists labels Jun 1, 2018
@Stebalien
Copy link
Member

Yep, this is a known issue. The current solution is to downgrade, remove them, and then re-upgrade. @kevina will write a migration for handling insecure hashes (not sure how at the moment) which should automate this.

#4766 is currently tracking this so I'll close this issue.

@kevina kevina removed the status/duplicate This issue or pull request already exists label Jun 2, 2018
@kevina kevina self-assigned this Jun 2, 2018
@kevina
Copy link
Contributor

kevina commented Jun 2, 2018

I don't consider this a duplicate of #4766 and I am not 100% sure that will solve help as it may just detect and refuse to do anything about it.

I am also reopening this issue because I want to get a feel for how much pain it will be to tell users to use an older version of ipfs as oppose to writing a special tool to deal with the problem.

If anyone else reading this has this problem please add a comment with the specifics.

@kevina kevina reopened this Jun 2, 2018
@kevina
Copy link
Contributor

kevina commented Jun 2, 2018

@alanshaw do you happen to know what this hash contains, you will unfortunately need to use an older version of IPFS to examine it.

@Stebalien
Copy link
Member

Fair enough. I see two conflicting concerns:

On one hand, our gateways will want a migration that just deletes nodes with bad CIDs. This'll be the case for many IPFS nodes.

On the other hand, deleting user data is not good.

IMO, the most user friendly thing to do would be to:

  1. If not pinned, just delete it.
  2. If pinned, fix the hash and then propagate the change up to the pin root (changing the root's hash).

For all modified pin roots, create (and pin) a new IPLD object mapping the old CIDs to the new CIDs.

However, this is probably overkill.


One thing we can almost certainly do is (1) from above. That is, when sweeping the datastore, if we come across a node with a bad CID that we could garbage collect, we can simply do so. This will cover the common case "gateway" case.

@kevina
Copy link
Contributor

kevina commented Jun 2, 2018

IMO, the most user friendly thing to do would be to: (1) If not pinned, just delete it.

The problem with deleting anything be default is that it is not easily reversible.

Instead what I plan to do is to just abort if there are any insure refs and instead tell the user to set an environmental variable say IPFS_REPO_MIGRATION_UNSAFE=delete_unpinned_insecure, and rerun the migration.

(2) If pinned, fix the hash and then propagate the change up to the pin root (changing the root's hash).

That may be overkill. I am not sure what to do about this case.

@kevina
Copy link
Contributor

kevina commented Jun 13, 2018

IMO, the most user friendly thing to do would be to:

  • If not pinned, just delete it.

I was about to implemented but then realized that it is more complicated than that, the more correct thing to do is "if it would normally be gc, delete it". This will avoid deleting anything hanging off the MFS root which uses the so called "best-effort" pin.

@Stebalien
Copy link
Member

I was about to implemented but then realized that it is more complicated than that, the more correct thing to do is "if it would normally be gc, delete it". This will avoid deleting anything hanging off the MFS root which uses the so called "best-effort" pin.

That's approximately what I meant. We definitely shouldn't delete anything "pinned" my MFS.

That may be overkill. I am not sure what to do about this case.

Really, bailing probably isn't that much of an issue. Now that I think about it, we don't want to try to fix it because, by definition, they're using an insecure hash function so we can't be sure that the hash we calculate is the correct one (the object may be forged).

@kevina
Copy link
Contributor

kevina commented Jun 15, 2018

Yeah, what I am now heavy leaning towards is removing anything that would ordinarily be GC if an environmental variable is defined (because even if it not pinned, the GC is not something users expect to run automatically right now), and if there is something that would not be GC bail, and likely tell the user they need to downgrade to IPFS 0.4.13 and manually unpin and possible remove any insecure hashes. That will be a pain, but I doubt there may be many users with this problem.

@Stebalien
Copy link
Member

Hm. Yeah, that like a good solution. In the common case, it should be fairly simple but you'll never lose data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

No branches or pull requests

3 participants