search

prysmaticlabs / prysm

Go implementation of Ethereum proof of stake
https://www.offchainlabs.com
GNU General Public License v3.0
3.46k stars 985 forks source link

[Mega tracking] - Audit Resolutions #6327

Closed terencechain closed 2 years ago

terencechain commented 4 years ago

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

🥈 Low risk issues grouped by components

Sync

Beacon state trie management

Core processing

DB

P2P networking

Infra set up:

UX:

Others:

0xKiwi commented 4 years ago

I'll be working on QSP-6

terencechain commented 4 years ago

Taking QSP-8

terencechain commented 4 years ago

Taking QSP-10 and QSP-11

rauljordan commented 4 years ago

Working on QSP-9

terencechain commented 4 years ago

Taking QSP-58 and QSP-59

nisdas commented 4 years ago

Taking QSP 13 and QSP 16

shayzluf commented 4 years ago

taking QSP-20

rauljordan commented 4 years ago

Working on 42

rauljordan commented 4 years ago

Workin on 61

shayzluf commented 4 years ago

working on QSP-14

rauljordan commented 4 years ago

Working on 35

farazdagi commented 4 years ago

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

rauljordan commented 4 years ago

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 commented 4 years ago

@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 commented 3 years ago