[Mega tracking] - Audit Resolutions

[terencechain]

Opening this mega tracking issue to track the overall resolution progresses of the latest Quantstamp Audit report.
For all medium & above risk issues. Each issue will have its own tracking issue.
For the rest of the issues (e.g. low risk, informational, undetermined). The tracking issues will be grouped by component, and consolidated under one tracking issue under that component.

🥇Medium and above risk issues

• QSP-4 Insecure gRPC connection by default (rpc service QSP-4 Use Self-Signed, Secure gRPC Connection by Default #6428)
• QSP-6 Use of pseudo random number generator where true random number generator would be needed (sync service), use crypto/rand instead of math/rand (sync service) QSP-6: Enforces crypto-secure PRNGs #6401
• QSP-7 Second pre-image attack on Merkle Trees (will be complete once we deprecate custom HTR and instead use fastssz codegen) Update fastssz: Attempt 2 #7115
• QSP-10 Possible loss of data integrity (state trie mgmt) QSP10, QSP11 - Add proper locking #6350
• QSP-11 Checks before the lock (state trie mgmt) QSP10, QSP11 - Add proper locking #6350
• QSP-14 Opening BoltDB and backup file varies on the permission (db) QSP-14 consistent file permissions #6378
• QSP-8 Function ActivationEligibilityEpoch() doesn't check v == nil || v.validator == nil (core processing) QSP8 - ActivationEligibilityEpoch nil case #6347
• QSP-9 Cast from uint64 to int (core processing) QSP-9 Prevent Casting to Int if Possible #6349
• QSP-12 DDoS attack vector through creating a mapping between public keys and validators’ IPs (Spec issue - out of scope of our resolution)
• QSP-13 Message encoding is changeable: that could lead to network partitions (p2p) QSP-13 Only Allow Snappy P2P Encoding #6415
• QSP-16 No support for IPv6 (p2p) QSP-16 Fix Support for IPv6 #6363
• QSP-15 Lack of unit tests (general)

🥈 Low risk issues grouped by components

Sync

• QSP-19 Memory resources are never freed in newBlocksFetcher() Reclaims leakybucket resources in sync service #6339
• QSP-20 No meaningful code in removeDisconnectedPeerStatus() QPS-20 implement removeDisconnectedPeerStatus #6370
• QSP-31 Peer descoring and disconnect on failed validation, while being optional in the spec, should be considered
  (even more encompassing solution in [Tracking] Peer scoring service #6622 - it involves peer scoring on GossipSub, p2p req/resp, and synchronization level)
• QSP-35 finalized_root not checked in Status topic Validate Finalized Root in Status RPC Requests #5811 Fix Finalized Epoch Checks #7364
• QSP-42 Double unsubscribe QSP-42 Remove Double Unsubscribe in Initial Sync #6368 Revert "QSP-42 Remove Double Unsubscribe in Initial Sync" #6376 and finally resolved in Init sync refactor state initialization check + tests #7285
• QSP-52 Signature validation for block attestations is conditional on feature flag Sync: Verify all block and attestations signatures on finalized blocks by default #6499
• QSP-61 Function processPendingAtts() finishes prematurely QSP-61 Clarify Why Process Pending Attestations May Exit Early #6371
• QSP-62 Corrupted clock QSP-62 Corrupted Clock In Stream Deadlines #6404

Beacon state trie management

• QSP-44 Duplicated dirty indices (marking obsolete, as using a map as recommended by quantstamp is far too big of a performance hit to be worth it. Appending indices as it stands today is not a security hole)
• QSP-64 Inconsistent rebuildTrie updates QSP-64 Conditional for Rebuilding BlockRoots Trie in State Setters #6390

Core processing

• QSP-39 areEth1DataEqual() returns false when both a and b are nil QSP-39 AreEth1DataEqual Should Return True If Both Nil #6372
• QSP-43 Unintended mutability (not relevant because input argument is an array, so it will not cause mutability issues)
• QSP-49 One-time calibration of Roughtime QSP-49 Recalibrate Roughtime #6393
• QSP-65 Using UnshuffleList() instead of ShuffleList() contradicts the specification QSP-65 Clarify Why UnshuffleList is Used #6381

DB

• QSP-21 Disk space exhaustion attack Possible disk space exhaustion when finality cannot be reached #6215
• QSP-38 Running out of disk space (BoltDB Freelist is not cleared up)
• QSP-41 Missing implementation for filtering blocks QSP-41 Clarify Block Filters Passthrough In DB #6397
• QSP-54 DefaultDataDir() may return empty string QSP-54 Handle Default Datadir Empty String #6394
• QSP-56 Deprecated jsonpb dependency QSP-56 Remove Deprecated Kafka JSONPB dependency #6383 (Won't fix.)
• QSP-57 Undocumented Kafka topics
• QSP-58 Inconsistent spans QSP58, QSP59 - Fix DB spans #6352
• QSP-59 Wrong StartSpan QSP58, QSP59 - Fix DB spans #6352
• QSP-60 Functions do not add summary to the cache QSP-60 Save State Summaries to Cache in DB Functions #6384

P2P networking

• QSP-18 No header length validation QSP18: Add Varint Header Validation #6577
• QSP-23 Disconnected, stale, or bad peer status records are not cleaned up QSP 23: Prune Peers From Peer Handler #6614
• QSP-24 Connection manager options are not always initialized correctly (no longer relevant in current codebase)
• QSP-25 Relay option is enabled in libp2p even when unused in Prysm QSP-25 Only Enable Relay if Flag Provided #6386
• QSP-26 Presence of the test-only code (blocked until mainnet release...while we have a relay, we need this current code, see Remove Relay Handshake Code for Mainnet #6425)
• QSP-27 Potential discrepancy between NextForkVersion and ForkVersionSchedule
• QSP-28 Subnet-based whitelisting not working (no longer relevant, this is already done)
• QSP-29 Rate limiting not implemented for some node communication QSP 29: Add Rate Limiter For All Topics #6606
• QSP-30 Incorrect (insecure) hash function is being used for determining message Ids (collisions using highway hash are acceptable for this use case, as it will not lead to a security issue if they occur)
• QSP-32 Incorrect deadline for responses QSP 32: Add Appropriate Stream Deadlines for RPC Requests #6583
• QSP-33 Maximum response chunk size not checked for all topics (done by QSP-33 Check Max Response For All Topics #6424)
• QSP-40 Return error when encoding nil buffer (not possible, see [Mega tracking] - Audit Resolutions #6327 (comment))
• QSP-45 Undefined behaviour of BestFinalized() when no peers are connected or some peers have no finalized epochs QSP-45 Add Check for No Connected Peers BestFinalized #6402
• QSP-46 Eclipse attacks are still possible (this is an issue with discv5 in general, out of scope of our code)
• QSP-47 Potentially meaningless check QSP-47 Remove Meaningless Relay Addr Check #6388
• QSP-48 Connection manager does not work properly (we no longer use connection manager)
• QSP-50 Possibly unintentional fallback to TCP encryption QSP 50, QSP 51: Make Noise Default  #6440
• QSP-51 NOISE support has no fallback QSP 50, QSP 51: Make Noise Default  #6440
• QSP-53 Snappy “frame” (streaming) format does not seem to be supported Align code base to v0.11 #5127 Fix Network Encoder #5435
• QSP-55 Support for extra pubsub protocols QSP-55 Remove Support for Other Pubsub Protocols #6419

Infra set up:

• QSP-22 Boot nodes availability and centralization risks (this is more about just having more than one bootnode as a default, rather than any changes to Prysm itself)

UX:

• QSP-36 Weak passwords allowed for validator accounts (completely revamped v2 of accounts is implemented - many closed PRs)
• QSP-37 Minimal system requirements

Others:

• Adherence to spec feedback Audit adherence to spec feedbacks #6335
• Documentation feedback Audit documentation feedbacks #6336
• Best practice feedback Audit best practice feedback #6337

[0xKiwi]

I'll be working on QSP-6

[terencechain]

Taking QSP-8

[terencechain]

Taking QSP-10 and QSP-11

[rauljordan]

Working on QSP-9

[terencechain]

Taking QSP-58 and QSP-59

[nisdas]

Taking QSP 13 and QSP 16

[shayzluf]

taking QSP-20

[rauljordan]

Working on 42

[rauljordan]

Workin on 61

[shayzluf]

working on QSP-14

[rauljordan]

Working on 35

[farazdagi]

Working on QSP-45 and QSP-6 (adding static analysis checker to enforce crypto/rand)

[rauljordan]

QSP-40 is not a possible condition, as the buffer can never be nil. For example:

func (e SszNetworkEncoder) EncodeGossip(w io.Writer, msg interface{}) (int, error) {
	if msg == nil {
		return 0, nil
	}
	b, err := e.doEncode(msg)
	if err != nil {
		return 0, err
	}
	if uint64(len(b)) > MaxGossipSize {
		return 0, errors.Errorf("gossip message exceeds max gossip size: %d bytes > %d bytes", len(b), MaxGossipSize)
	}
	if e.UseSnappyCompression {
		b = snappy.Encode(nil /*dst*/, b)
	}
+	if b == nil {
+		return 0, errors.New("attempting to write to nil buffer")
+	}
	return w.Write(b)
}

Even with a test that may seem like it could trigger the conditional check for b == nil, the code path is actually never reached. Worst case scenario, the result of snappy.Encode will be a buffer with length 1.

func TestSszNetworkEncoder_EncodeGossip_NilBuffer(t *testing.T) {
	buf := new(bytes.Buffer)
	type emptyContainer struct{}
	msg := &emptyContainer{}
	e := &encoder.SszNetworkEncoder{UseSnappyCompression: true}
	_, err := e.EncodeGossip(buf, msg)
	if err != nil {
		t.Error(err)
	}
}

The test above passes.

[rauljordan]

@nisdas for QSP-32, how do you think we should handle it? Basically:

The P2P specification says that: “The requester MUST wait a maximum of TTFB_TIMEOUT for the first response byte to arrive (time to first byte—or TTFB—timeout). On that happening, the requester allows a further RESP_TIMEOUT for each subsequent response_chunk received.” According to specification TTFB_TIMEOUT is 5s. It is the maximum time to wait for the first byte of request response (time-to-first-byte).

However, the deadline is set to:
on L24 in beacon-chain/p2p/sender.go.

// Send a message to a specific peer. The returned stream may be used for reading, but has been
// closed for writing.
func (s *Service) Send(ctx context.Context, message interface{}, baseTopic string, pid peer.ID) (network.Stream, error) {
	ctx, span := trace.StartSpan(ctx, "p2p.Send")
	defer span.End()
	topic := baseTopic + s.Encoding().ProtocolSuffix()
	span.AddAttributes(trace.StringAttribute("topic", topic))

	// TTFB_TIME (5s) + RESP_TIMEOUT (10s).
	var deadline = params.BeaconNetworkConfig().TtfbTimeout + params.BeaconNetworkConfig().RespTimeout

Seems like we'll have to (a) be aware of how many chunks there are and (b) use that to extend the context deadline appropriately by the RESP_TIMEOUT value for each. How can we the number of chunks here?

[rauljordan]

• QSP-4 Insecure gRPC connection by default: won't fix, this is only a concern for users that are running on different machines, and it will be a significant UX constraint to force normal users to handle and deal with TLS certs when they might not need to
• QSP-57 Undocumented Kafka topics: won't fix, as it is too low priority and not much importance given barely anyone is using our kafka support
• QSP-37 Minimal system requirements: will fix via a log
• QSP-38 Running out of disk space (BoltDB Freelist is not cleared up): needs to look into, will open as a separate tracking issue