Are IPNS record signatures really validated?

[wigy-opensource-developer]

Version information:

go-ipfs version: 0.4.3-
Repo version: 4
System version: amd64/linux
Golang version: go1.7

Type:

Bug

Priority:

P2

Description:

As I understand the current state of IPNS is alpha. Still, I do not see the code that prevents malicious nodes to poison the DHT recordstore with a record that has a higher sequence number, but invalid signature.

Some pointers to the code that record my journey of debugging the problem:

• /~https://github.com/ipfs/go-ipfs/blob/1c6e84ce7c9e8b6deb6a221e6379640de882d6fd/core/core.go#L649-L661
  • Configures validity criteria for the IPNS records.
• /~https://github.com/ipfs/go-ipfs/blob/1c6e84ce7c9e8b6deb6a221e6379640de882d6fd/namesys/publisher.go#L251
  • Selects the record with highest sequence number among those whose validity is not expired yet, ignores signature.
• /~https://github.com/ipfs/go-ipfs/blob/1c6e84ce7c9e8b6deb6a221e6379640de882d6fd/namesys/publisher.go#L307
  • Validates time again, but ignores signature.
• /~https://github.com/ipfs/go-ipfs/blob/1c6e84ce7c9e8b6deb6a221e6379640de882d6fd/routing/supernode/server.go#L208
  • This verify function is currently not used, but probably that should have done the trick checking the signature.
• /~https://github.com/libp2p/go-libp2p-record/blob/ffbd25bc61154d676b7c8148e2e767daeaa8108d/validation.go#L101-L104
  • It is worth noting that the /pk/ record schema does not need signing, because the hashing cannot be easily forged.
• /~https://github.com/ipfs/go-ipfs/blob/1c6e84ce7c9e8b6deb6a221e6379640de882d6fd/namesys/publisher.go#L154-L160
  • As far as I can tell, the validation of the /ipns/ scheme depends on a successful publishing of the /pk/ record. This is done in parallel and in reverse order, so this might have caused issues in testing.

If the Author field of the IPRS records could be a public key, not the hash of the public key, the IPNS record could be a self-signed certificate of the author + the IPFS content hash and would not depend on the /pk/ IPRS schema. Is there any advantage of storing the Author field as an ID and keep the /pk/ record store to resolve it?

[wigy-opensource-developer]

Bump. This is a security issue. Should I spend more time producing a fix, or should I give a demo defacing the site ipfs.io? 😘

[whyrusleeping]

First off, IMPORTANT:
If you suspect you've found a security issue please disclose things responsibly as detailed in the readme.

Hey @wigy-opensource-developer, thanks for diving into this. The records are validated in the dht code here: /~https://github.com/libp2p/go-libp2p-kad-dht/blob/master/dht.go#L182
And also validates signature of records stored here: /~https://github.com/libp2p/go-libp2p-kad-dht/blob/master/handlers.go#L160

Is there any advantage of storing the Author field as an ID and keep the /pk/ record store to resolve it?

Other than reducing the size of records we're asking other nodes to store for us, no. Adding an extra 256 or 512 bytes to each ~100 byte record is a significant overhead in storage space for the network. When we look at migrating to ed25519 keys and implementing the actual iprs spec we may consider embedding keys into the record itself.

[wigy-opensource-developer]

Hi Jeromy, thanks for the answer. I had a suspicion that it was an oversight on my side, because IPFS development seemed to be more serious than leaving such a gaping hole in there.

As for the future, I spent about 15 minutes reading about how to report a potential issue to go-ipfs. The issue template points to /~https://github.com/ipfs/community/blob/master/contributing.md#reporting-issues and /~https://github.com/ipfs/go-ipfs/blob/master/docs/github-issue-guide.md but those do not mention anything about security issues. Could you please either copy that paragraph from the /~https://github.com/ipfs/go-ipfs/blob/master/README.md#security-issues into either of that, or just mention this in the issue template?

And thanks for the pointers for the signature validation. Since it is done in the DHT implementation of a record store, it means it will need to be duplicated in the coming alternative implementations. But that is fine.

[whyrusleeping]

@wigy-opensource-developer I will make sure to make a note of the security reporting info in the issue template, thanks!

Also, another place that verification happens is here: /~https://github.com/ipfs/go-ipfs/blob/master/namesys/routing.go#L181