fix(net): Rate-limit MetaAddrChange::Responded from peers

[teor2345]

Motivation

There is no limit to the amount of MetaAddrChange::Responded updates a peer can send to the shared address book updater channel.

This is a remotely-triggerable security issue, likely resulting in a hang or very slow network performance.

Complex Code or Requirements

This code runs concurrently for every peer, then writes to a channel shared between all peers. Writing on the channel blocks if it is full.

Solution

• Stop allowing peers to send a Ping or Pong message to generate a MetaAddrChange::Responded
• Instead, generate a MetaAddrChange::Responded after every successful heartbeat, which is already rate-limited by Zebra

Review

This is a security fix that we might want to block the stable release for.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

[teor2345]

@mpguerra this seems serious enough to block the stable release, but it's a small change so it should be easy to review.

[codecov]

Codecov Report

Merging #6738 (74ac9f4) into main (a972144) will increase coverage by 0.19%.
The diff coverage is 66.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6738      +/-   ##
==========================================
+ Coverage   77.92%   78.11%   +0.19%     
==========================================
  Files         308      308              
  Lines       40874    40879       +5     
==========================================
+ Hits        31851    31933      +82     
+ Misses       9023     8946      -77     

[teor2345]

Temporary network failure:

Error: buildx failed with: ERROR: failed to solve: failed to push us-docker.pkg.dev/zfnd-dev-zebra/zebra/zebrad-test:pr-6738: failed to copy: failed to do request: Put "https://us-docker.pkg.dev/artifacts-uploads/namespaces/zfnd-dev-zebra/repositories/zebra/uploads/...": write tcp 172.17.0.2:47038->142.251.116.82:443: write: connection reset by peer

https://github.com/ZcashFoundation/zebra/actions/runs/5039819774/jobs/9038203151?pr=6738#step:10:926

[mpguerra]

@mpguerra this seems serious enough to block the stable release, but it's a small change so it should be easy to review.

Is there a corresponding issue for it? It's ok if not, I just want to make sure we track it somehow.

[teor2345]

@mpguerra this seems serious enough to block the stable release, but it's a small change so it should be easy to review.

Is there a corresponding issue for it? It's ok if not, I just want to make sure we track it somehow.

I didn't open a separate ticket.

This is related to #6672, because it was one of the ways that remote peers could easily manipulate their address state inside Zebra. (If we'd fixed this bug earlier, the audit finding might have been low severity, because peer state changes would all have been rate-limited or triggered by the local Zebra node.)

Also happy to call it a separate bug if you want.

[mpguerra]

@mpguerra this seems serious enough to block the stable release, but it's a small change so it should be easy to review.

Is there a corresponding issue for it? It's ok if not, I just want to make sure we track it somehow.

I didn't open a separate ticket.

This is related to #6672, because it was one of the ways that remote peers could easily manipulate their address state inside Zebra. (If we'd fixed this bug earlier, the audit finding might have been low severity, because peer state changes would all have been rate-limited or triggered by the local Zebra node.)

Also happy to call it a separate bug if you want.

Nope, I see I already linked it to the relevant audit issue, it's all I need :)

[arya2]

Looks good!