Validate received chunks before conversion to indexes

https://github.com/anacrolix/torrent/issues/791

peerconn.go

@@ -596,6 +596,10 @@ type messageWriter func(pp.Message) bool
 // This function seems to only used by Peer.request. It's all logic checks, so maybe we can no-op it
 // when we want to go fast.
 func (cn *Peer) shouldRequest(r RequestIndex) error {
+	err := cn.t.checkValidReceiveChunk(cn.t.requestIndexToRequest(r))
+	if err != nil {
+		return err
+	}
 	pi := cn.t.pieceIndexOfRequestIndex(r)
 	if cn.requestState.Cancelled.Contains(r) {
 		return errors.New("request is cancelled and waiting acknowledgement")
@@ -1425,8 +1429,13 @@ func (c *Peer) receiveChunk(msg *pp.Message) error {
 	chunksReceived.Add("total", 1)
 
 	ppReq := newRequestFromMessage(msg)
-	req := c.t.requestIndexFromRequest(ppReq)
 	t := c.t
+	err := t.checkValidReceiveChunk(ppReq)
+	if err != nil {
+		err = log.WithLevel(log.Warning, err)
+		return err
+	}
+	req := c.t.requestIndexFromRequest(ppReq)
 
 	if c.bannableAddr.Ok {
 		t.smartBanCache.RecordBlock(c.bannableAddr.Value, req, msg.Piece)
@@ -1508,7 +1517,7 @@ func (c *Peer) receiveChunk(msg *pp.Message) error {
 		p.cancel(req)
 	}
 
-	err := func() error {
+	err = func() error {
 		cl.unlock()
 		defer cl.lock()
 		concurrentChunkWrites.Add(1)

torrent.go

@@ -2541,3 +2541,22 @@ type requestState struct {
 	peer *Peer
 	when time.Time
 }
+
+// Returns an error if a received chunk is out of bounds in someway.
+func (t *Torrent) checkValidReceiveChunk(r Request) error {
+	if !t.haveInfo() {
+		return errors.New("torrent missing info")
+	}
+	if int(r.Index) >= t.numPieces() {
+		return fmt.Errorf("chunk index %v, torrent num pieces %v", r.Index, t.numPieces())
+	}
+	pieceLength := t.pieceLength(pieceIndex(r.Index))
+	if r.Begin >= pieceLength {
+		return fmt.Errorf("chunk begins beyond end of piece (%v >= %v)", r.Begin, pieceLength)
+	}
+	// We could check chunk lengths here, but chunk request size is not changed often, and tricky
+	// for peers to manipulate as they need to send potentially large buffers to begin with. There
+	// should be considerable checks elsewhere for this case due to the network overhead. We should
+	// catch most of the overflow manipulation stuff by checking index and begin above.
+	return nil
+}