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

Add RSA signature protection for DHT records #187

Merged
merged 15 commits into from
Apr 5, 2021
Merged

Add RSA signature protection for DHT records #187

merged 15 commits into from
Apr 5, 2021

Conversation

borzunov
Copy link
Member

@borzunov borzunov commented Mar 19, 2021

This PR introduces a notion of protected DHT records whose key/subkey contains substring [owner:ssh-rsa ...] (the format can be changed) with an RSA public key of the owner. Changes to such records always must be signed with the corresponding private key (so only the owner can change them). This protects from malicious nodes trying to spoil the DHT contents (see the list of attack vectors).

TODO:

  • Save the signature and make the DHT readers check it (so malicious DHT nodes won't be able to fake the contents of such records)
  • Write unit tests
  • Check time of signing/signature validation (if it is > 100 ms, consider moving it to a separate thread)
  • Check whether the cryptography module has heavy dependencies and whether we can choose a better alternative

@borzunov borzunov added security Security issues or improvements dht labels Mar 19, 2021
@borzunov borzunov requested a review from justheuristic March 19, 2021 00:01
@borzunov borzunov linked an issue Mar 19, 2021 that may be closed by this pull request
@borzunov borzunov removed a link to an issue Mar 19, 2021
@borzunov
Copy link
Member Author

borzunov commented Mar 29, 2021

For RSASignatureValidator (backed by the cryptography module) and a record of size > 1 KiB, sign_value(), validate(), and strip_value() take < 1 ms each:

In [21]: validator = RSASignatureValidator()

In [23]: record = DHTRecord(b'key', b'subkey' + validator.ownership_marker, b'value' + b'X' * 1000, get_dht_time())

In [30]: %timeit protected_record = dataclasses.replace(record, value=validator.sign_value(record))
852 µs ± 8.94 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [35]: %timeit validator.validate(protected_record)
180 µs ± 3.06 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [36]: %timeit record = dataclasses.replace(protected_record, value=validator.strip_value(protected_record))
10.9 µs ± 105 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

So, using the validator won't interfere with the asyncio event loop.

Generating a private-public key pair (happens once before starting the node) takes ~101 ms:

In [38]: %timeit validator = RSASignatureValidator()
101 ms ± 10.9 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

@borzunov borzunov marked this pull request as ready for review March 29, 2021 13:52
@dataclasses.dataclass(init=True, repr=True, frozen=True)
class DHTRecord:
key: bytes
subkey: bytes
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, this field is optional

Copy link
Member Author

@borzunov borzunov Mar 31, 2021

Choose a reason for hiding this comment

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

It is not optional here, at the protocol layer. In case of a non-dictionary value or a whole dictionary, the subkey contains a special bytes value.

@borzunov borzunov requested review from justheuristic and mryab April 2, 2021 07:42

serialized_public_key = self._private_key.public_key().public_bytes(
encoding=serialization.Encoding.OpenSSH, format=serialization.PublicFormat.OpenSSH)
self._our_marker = marker_format.replace(b'_key_', serialized_public_key)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._our_marker = marker_format.replace(b'_key_', serialized_public_key)
self._marker = marker_format.replace(b'_key_', serialized_public_key)

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed it to self._ownership_marker, is it okay?

Comment on lines +484 to +486
store_ok = await mallory.store(key, b'updated_fake_value',
hivemind.get_dht_time() + 10, subkey=subkey)
assert not store_ok
assert (await alice.get(key, latest=True)).value[subkey].value == b'updated_true_value'
Copy link
Member

Choose a reason for hiding this comment

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

I still think we need the following test: writing in a protected key with a different signature should be possible after the previous record expires

Copy link
Member

@justheuristic justheuristic Apr 2, 2021

Choose a reason for hiding this comment

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

[based on discussion with @borzunov ]
Alas, this functionality is not supported and we will likely have to do without it.

The validation of (key, subkey, value, expiration) tuple does not depend on whether or not there is a previous value under that key.

Unfortunately, conditioning the validator on currently stored entries would not be insecure:

  • not all DHT peers that are supposed to have a given value will actually have it at any given moment in time. An attacker can use this to bypass the protection.
  • DHT peers sometimes need to pass information to new nodes e.g. welcome_if_new.

Fortunately, it seems that there is a workaround that will allow us to secure all hivemind components with the current validator interface.

  • Averager/metadata: all secret keys are unique and tied to peer identity. No two peers will even need (or be allowed to write) to the same (key, subkey) pair
  • Experts / beam search: an authority (server or blockchain) will lease a separate security key for each expert that gives the use permission to declare this expert. This security key will be tied to (expert uuid, peer_id/endpoint, max expiration).

@borzunov borzunov requested a review from mryab April 3, 2021 15:00
@mryab mryab changed the title Protect some DHT records with RSA signature Add RSA signature protection for DHT records Apr 5, 2021
@borzunov borzunov merged commit 1deab01 into learning-at-home:master Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dht security Security issues or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants