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

Ignore and sanitize untrusted MetaAddr peer services #2324

Closed
1 of 8 tasks
Tracked by #2311
teor2345 opened this issue Jun 16, 2021 · 2 comments
Closed
1 of 8 tasks
Tracked by #2311

Ignore and sanitize untrusted MetaAddr peer services #2324

teor2345 opened this issue Jun 16, 2021 · 2 comments
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 16, 2021

Motivation

We have a higher level of trust in peer services that we learn directly from the peer itself.

But if we learn those services from other peers, we don't trust them very much, so we might want to:

  • sanitize untrusted services
  • ignore untrusted services when choosing the next peer to connect to

Specifications

The services the node advertised in its “version” message.

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

(The bitcoin reference and bitcoin wiki don't say much about services.)

Solution

Refactor

  • make services optional (PR Make services field in MetaAddr optional #2976)
  • split peer services into optional untrusted and direct fields
  • update those fields correctly with each service change
  • create a "services implemented by Zebra" constant

Security

  • ignore untrusted services when choosing the next peer to connect to in impl Ord for MetaAddr
    • order trusted services by usefulness when selecting peers, not numeric values
      • make sure Option<PeerServices> ordering handles None and Some comparisons correctly
  • consider sanitizing untrusted service fields to known bits (?)

Alternatives

This change doesn't make any difference to security until Zebra supports multiple service bits.

Even then, this change might not make much difference to security.

Related Work

The MetaAddrChange code in PRs #2273 and #2275 already has some untrusted service fields.

@teor2345 teor2345 added A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage P-Low C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes labels Jun 16, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jun 21, 2021
@ftm1000
Copy link

ftm1000 commented Jan 26, 2022

redistributing issues that can be separately worked from this epic #2311

@mpguerra mpguerra mentioned this issue Jan 27, 2022
40 tasks
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 1, 2022

Zcash doesn't really use this field, so this change is not required.

@teor2345 teor2345 closed this as completed Mar 1, 2022
@mpguerra mpguerra closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data
Projects
None yet
Development

No branches or pull requests

3 participants