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.

Sign up for GitHub

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

Jump to bottom

Security: stop gossiping failure and attempt times as last_seen times #2273

Merged
merged 4 commits into from
Jun 15, 2021
Merged

Security: stop gossiping failure and attempt times as last_seen times #2273

merged 4 commits into from
Jun 15, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 10, 2021 •
edited by jvff
Loading

Motivation

Previously, Zebra had a single time field for each peer, which was updated every time a peer was attempted, sent a message, or failed.

This is a security issue, because the last_seen time should be "the last time [a peer] connected to that node", so that
"nodes can use the time field to avoid relaying old 'addr' messages".

So Zebra was sending incorrect peer times to other nodes, which is malicious behaviour.

Specifications

addr.time field description:

Nodes advertising IP addresses they’ve connected to set this to the last time they connected to that node. Other nodes just relaying the IP address should not change the time. Nodes can use the time field to avoid relaying old “addr” messages. Malicious nodes may change times or even set them in the future.

https://developer.bitcoin.org/reference/p2p_networking.html#addr

Designs

This PR uses the DateTime32 type introduced in PR #2210, see ticket #2171 for more details.

Solution

Security Changes

  • Keep success times separate from failure and attempt times
  • Only send success times to peers

Other Required Changes

Split the last_seen time into the following fields:

  • untrusted_last_seen: time gossiped from other peers - serialized DateTime32
  • last_response: time we got a response from a directly connected peer - serialized DateTime32
  • last_attempt: time we attempted to connect to a peer - monotonic Instant
  • last_failure: time a connection with a peer failed - monotonic Instant

Send peer changes using a new MetaAddrChange type, so that senders don't accidentally overwrite peer fields with outdated data. (Due to concurrency, changes can be applied to the address book out of order.)

Review

@jvff can review this PR. It's an important security fix, but it's not particularly urgent.

This PR should close #1868.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

This PR is part of a series of MetaAddr refactors.

After #1867 is fixed, the network will be able to remove the unreachable addresses introduced by #2120.

@teor2345
Security: stop gossiping failure and attempt times as last_seen times
c8667b3 
Previously, Zebra had a single time field for peer addresses, which was
updated every time a peer was attempted, sent a message, or failed.

This is a security issue, because the `last_seen` time should be
"the last time [a peer] connected to that node", so that
"nodes can use the time field to avoid relaying old 'addr' messages".
So Zebra was sending incorrect peer information to other nodes.

As part of this change, we split the `last_seen` time into the
following fields:
- untrusted_last_seen: gossiped from other peers
- last_response: time we got a response from a directly connected peer
- last_attempt: time we attempted to connect to a peer
- last_failure: time a connection with a peer failed
@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code P-High C-security Category: Security issues I-slow Problems with performance or responsiveness I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes labels Jun 10, 2021
@teor2345 teor2345 added this to the 2021 Sprint 11 - Zcon2 milestone Jun 10, 2021
@teor2345 teor2345 requested a review from jvff June 10, 2021 04:44
@teor2345 teor2345 self-assigned this Jun 10, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Jun 10, 2021 •
edited
Loading

I still need to write proptests to ensure:

  • the untrusted last seen time is not updated by any change
  • the services are only updated after a handshake
  • the only times that get included in serialized MetaAddrs are the untrusted last seen and responded times

@teor2345 teor2345 mentioned this pull request Jun 10, 2021
Merged
3 tasks
teor2345 added 2 commits June 11, 2021 15:20
@teor2345
Implement Arbitrary and strategies for MetaAddrChange
2c24dab 
Also replace the MetaAddr Arbitrary impl with a derive.
@teor2345
Write proptests for MetaAddr and MetaAddrChange
0183b1d 
MetaAddr:
- the only times that get included in serialized MetaAddrs are
  the untrusted last seen and responded times

MetaAddrChange:
- the untrusted last seen time is never updated
- the services are only updated if there has been a handshake
@teor2345 teor2345 marked this pull request as ready for review June 11, 2021 05:23
jvff
jvff reviewed Jun 11, 2021
View reviewed changes
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

It's looking good! I added a few suggestions and questions.

zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr.rs Show resolved Hide resolved
zebra-network/src/meta_addr/arbitrary.rs Show resolved Hide resolved
zebra-network/src/meta_addr/arbitrary.rs Outdated Show resolved Hide resolved
@jvff
Copy link
Contributor

jvff commented Jun 11, 2021

I updated the PR description, changing After we fix #1867 to After #1867 is fixed, because GitHub parsed the fix #1867 and thought that this PR would close that issue once it is merged.

@teor2345 teor2345 mentioned this pull request Jun 14, 2021
Closed
7 tasks
@teor2345
Copy link
Contributor Author

I updated the PR description, changing After we fix #1867 to After #1867 is fixed, because GitHub parsed the fix #1867 and thought that this PR would close that issue once it is merged.

Thanks for catching that!

@mpguerra mpguerra removed this from the 2021 Sprint 11 - Zcon2 milestone Jun 14, 2021
jvff
jvff previously approved these changes Jun 14, 2021
View reviewed changes
@teor2345
Merge branch 'main' into stop-failure-as-last-seen
775d2d0 
- make ready_outbound_strategy generate changes
- update existing strategies based on the refactor
- update newly added proptests based on the refactor
@teor2345 teor2345 requested a review from jvff June 15, 2021 02:38
@teor2345
Copy link
Contributor Author

I merged the arbitrary and proptest changes from #2278.

All the changes were in test code, and they were minor. So I'll admin-merge once CI passes, which lets me make progress on #2275 without more conflicts.

@teor2345 teor2345 assigned teor2345 and unassigned teor2345 Jun 15, 2021
@teor2345 teor2345 enabled auto-merge (squash) June 15, 2021 02:48
@teor2345 teor2345 disabled auto-merge June 15, 2021 03:30
@teor2345 teor2345 merged commit 3f7410d into main Jun 15, 2021
@teor2345 teor2345 deleted the stop-failure-as-last-seen branch June 15, 2021 03:31
dconnolly pushed a commit that referenced this pull request Jun 15, 2021
@teor2345 @dconnolly
Security: stop gossiping failure and attempt times as last_seen times (…
5e3d686 
…#2273)

* Security: stop gossiping failure and attempt times as last_seen times

Previously, Zebra had a single time field for peer addresses, which was
updated every time a peer was attempted, sent a message, or failed.

This is a security issue, because the `last_seen` time should be
"the last time [a peer] connected to that node", so that
"nodes can use the time field to avoid relaying old 'addr' messages".
So Zebra was sending incorrect peer information to other nodes.

As part of this change, we split the `last_seen` time into the
following fields:
- untrusted_last_seen: gossiped from other peers
- last_response: time we got a response from a directly connected peer
- last_attempt: time we attempted to connect to a peer
- last_failure: time a connection with a peer failed

* Implement Arbitrary and strategies for MetaAddrChange

Also replace the MetaAddr Arbitrary impl with a derive.

* Write proptests for MetaAddr and MetaAddrChange

MetaAddr:
- the only times that get included in serialized MetaAddrs are
  the untrusted last seen and responded times

MetaAddrChange:
- the untrusted last seen time is never updated
- the services are only updated if there has been a handshake
dconnolly pushed a commit that referenced this pull request Jun 16, 2021
@teor2345 @dconnolly
Security: stop gossiping failure and attempt times as last_seen times (…
517ddf4 
…#2273)

* Security: stop gossiping failure and attempt times as last_seen times

Previously, Zebra had a single time field for peer addresses, which was
updated every time a peer was attempted, sent a message, or failed.

This is a security issue, because the `last_seen` time should be
"the last time [a peer] connected to that node", so that
"nodes can use the time field to avoid relaying old 'addr' messages".
So Zebra was sending incorrect peer information to other nodes.

As part of this change, we split the `last_seen` time into the
following fields:
- untrusted_last_seen: gossiped from other peers
- last_response: time we got a response from a directly connected peer
- last_attempt: time we attempted to connect to a peer
- last_failure: time a connection with a peer failed

* Implement Arbitrary and strategies for MetaAddrChange

Also replace the MetaAddr Arbitrary impl with a derive.

* Write proptests for MetaAddr and MetaAddrChange

MetaAddr:
- the only times that get included in serialized MetaAddrs are
  the untrusted last seen and responded times

MetaAddrChange:
- the untrusted last seen time is never updated
- the services are only updated if there has been a handshake
This was referenced Jun 16, 2021
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvff jvff Awaiting requested review from jvff

Assignees

@teor2345 teor2345

Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Security: Zebra should stop gossiping failure times as last_seen times
3 participants
@teor2345 @jvff @mpguerra