fix(protocol): always expect & validate block hash in requests (#10738)

Verify that block requests have a hash and that it's correct. This helps
prevent certain races and ensure that only expected data is ever
returned in response to a request.

(In Syncthing prior to 1.28.1 the block hash was omitted for encrypted
requests from trusted devices. This breaks compatibility with that
specific config on those versions.)

---------

Signed-off-by: Jakob Borg <jakob@kastelo.net>

lib/model/model.go

@@ -2079,7 +2079,7 @@ func (m *model) Request(conn protocol.Connection, req *protocol.Request) (out pr
 		return nil, protocol.ErrGeneric
 	}
 
-	if folderCfg.Type != config.FolderTypeReceiveEncrypted && len(req.Hash) > 0 && !scanner.Validate(res.data[:n], req.Hash) {
+	if folderCfg.Type != config.FolderTypeReceiveEncrypted && !scanner.Validate(res.data[:n], req.Hash) {
 		m.recheckFile(deviceID, req.Folder, req.Name, req.Offset, req.Hash)
 		l.Debugf("%v REQ(in) failed validating data: %s: %q / %q o=%d s=%d", m, deviceID.Short(), req.Folder, req.Name, req.Offset, req.Size)
 		return nil, protocol.ErrNoSuchFile

lib/model/model_test.go

@@ -9,6 +9,7 @@ package model
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -93,8 +94,10 @@ func TestRequest(t *testing.T) {
 
 	m.ScanFolder("default")
 
+	foobarHash := sha256.Sum256([]byte("foobar"))
+
 	// Existing, shared file
-	res, err := m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 6})
+	res, err := m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 6, Hash: foobarHash[:]})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -104,35 +107,42 @@ func TestRequest(t *testing.T) {
 	}
 
 	// Existing, nonshared file
-	_, err = m.Request(device2Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 6})
+	_, err = m.Request(device2Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 6, Hash: foobarHash[:]})
 	if err == nil {
 		t.Error("Unexpected nil error on insecure file read")
 	}
 
 	// Nonexistent file
-	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "nonexistent", Size: 6})
+	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "nonexistent", Size: 6, Hash: foobarHash[:]})
 	if err == nil {
 		t.Error("Unexpected nil error on insecure file read")
 	}
 
 	// Shared folder, but disallowed file name
-	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "../walk.go", Size: 6})
+	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "../walk.go", Size: 6, Hash: foobarHash[:]})
 	if err == nil {
 		t.Error("Unexpected nil error on insecure file read")
 	}
 
 	// Negative size
-	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: -4})
+	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: -4, Hash: foobarHash[:]})
 	if err == nil {
 		t.Error("Unexpected nil error on insecure file read")
 	}
 
-	// Larger block than available
+	// Missing hash
+	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 6})
+	if err == nil {
+		t.Error("Unexpected nil error on request without hash")
+	}
+
+	// Larger block than available, with a mismatched hash
 	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 42, Hash: []byte("hash necessary but not checked")})
 	if err == nil {
 		t.Error("Unexpected nil error on read past end of file")
 	}
-	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 42})
+	// Larger block than available, with the matching hash of the short read
+	_, err = m.Request(device1Conn, &protocol.Request{Folder: "default", Name: "foo", Size: 42, Hash: foobarHash[:]})
 	if err != nil {
 		t.Error("Unexpected error when large read should be permitted")
 	}
@@ -2979,15 +2989,16 @@ func TestRequestLimit(t *testing.T) {
 	defer cleanupModel(m)
 	m.ScanFolder("default")
 
+	emptyHash := sha256.Sum256(nil)
 	befReq := time.Now()
-	first, err := m.Request(conn, &protocol.Request{Folder: "default", Name: file, Size: 2000})
+	first, err := m.Request(conn, &protocol.Request{Folder: "default", Name: file, Size: 2000, Hash: emptyHash[:]})
 	if err != nil {
 		t.Fatalf("First request failed: %v", err)
 	}
 	reqDur := time.Since(befReq)
 	returned := make(chan struct{})
 	go func() {
-		second, err := m.Request(conn, &protocol.Request{Folder: "default", Name: file, Size: 2000})
+		second, err := m.Request(conn, &protocol.Request{Folder: "default", Name: file, Size: 2000, Hash: emptyHash[:]})
 		if err != nil {
 			t.Errorf("Second request failed: %v", err)
 		}

lib/protocol/encryption.go

@@ -93,31 +93,25 @@ func (e encryptedModel) Request(req *Request) (RequestResponse, error) {
 	}
 	realSize := req.Size - blockOverhead
 	realOffset := req.Offset - int64(req.BlockNo*blockOverhead)
+	if realOffset < 0 {
+		panic("bug: realOffset underflow")
+	}
 
 	if req.Size < minPaddedSize {
 		return nil, errors.New("short request")
 	}
 
-	// Attempt to decrypt the block hash; it may be nil depending on what
-	// type of device the request comes from. Trusted devices with
-	// encryption enabled know the hash but don't bother to encrypt & send
-	// it to us. Untrusted devices have the hash from the encrypted index
-	// data and do send it. The model knows to only verify the hash if it
-	// actually gets one.
-
-	var realHash []byte
+	// Decrypt the block hash.
 	fileKey := e.keyGen.FileKey(realName, folderKey)
-	if len(req.Hash) > 0 {
-		var additional [8]byte
-		binary.BigEndian.PutUint64(additional[:], uint64(realOffset))
-		realHash, err = decryptDeterministic(req.Hash, fileKey, additional[:])
-		if err != nil {
-			// "Legacy", no offset additional data?
-			realHash, err = decryptDeterministic(req.Hash, fileKey, nil)
-		}
-		if err != nil {
-			return nil, fmt.Errorf("decrypting block hash: %w", err)
-		}
+	var additional [8]byte
+	binary.BigEndian.PutUint64(additional[:], uint64(realOffset))
+	realHash, err := decryptDeterministic(req.Hash, fileKey, additional[:])
+	if err != nil {
+		// "Legacy", no offset additional data?
+		realHash, err = decryptDeterministic(req.Hash, fileKey, nil)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("decrypting block hash: %w", err)
 	}
 
 	// Perform that request and grab the data.

lib/protocol/protocol.go

@@ -489,6 +489,12 @@ func (c *rawConnection) dispatcherLoop() (err error) {
 			if msg.Size > MaxRequestSize {
 				return newProtocolError(fmt.Errorf("request size %d exceeds maximum allowed", msg.Size), msgContext)
 			}
+			if len(msg.Hash) == 0 {
+				// Syncthing versions older than v1.28.1 omit the hash in
+				// encrypted requests from trusted devices (a rare config)
+				// and will run into this.
+				return newProtocolError(errors.New("request missing block hash"), msgContext)
+			}
 			go c.handleRequest(requestFromWire(msg))
 
 		case *bep.Response:

lib/protocol/protocol_test.go

@@ -560,6 +560,7 @@ func TestRequestMaxSize(t *testing.T) {
 				Id:   1,
 				Name: "valid",
 				Size: MaxRequestSize,
+				Hash: []byte{42},
 			}
 
 			res := <-c.outbox
@@ -573,6 +574,7 @@ func TestRequestMaxSize(t *testing.T) {
 				Id:   2,
 				Name: "invalid",
 				Size: int32(s),
+				Hash: []byte{42},
 			}
 
 			select {
@@ -606,6 +608,7 @@ func TestRequestZeroSize(t *testing.T) {
 		Id:   1,
 		Name: "valid",
 		Size: 0,
+		Hash: []byte{42},
 	}
 
 	select {
@@ -632,6 +635,7 @@ func TestRequestInvalidFilename(t *testing.T) {
 		Id:   1,
 		Name: "../escape",
 		Size: 1024,
+		Hash: []byte{42},
 	}
 
 	select {

lib/scanner/blocks.go

@@ -120,25 +120,12 @@ func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, cou
 	return blocks, nil
 }
 
-// Validate validates the hash, if len(hash)>0.
+// Validate validates the hash.
 func Validate(buf, hash []byte) bool {
-	if len(hash) > 0 {
-		hbuf := sha256.Sum256(buf)
-		return bytes.Equal(hbuf[:], hash)
-	}
-
-	return true
+	hbuf := sha256.Sum256(buf)
+	return bytes.Equal(hbuf[:], hash)
 }
 
-type noopHash struct{}
-
-func (noopHash) Sum32() uint32             { return 0 }
-func (noopHash) BlockSize() int            { return 0 }
-func (noopHash) Size() int                 { return 0 }
-func (noopHash) Reset()                    {}
-func (noopHash) Sum([]byte) []byte         { return nil }
-func (noopHash) Write([]byte) (int, error) { return 0, nil }
-
 type noopCounter struct{}
 
 func (*noopCounter) Update(_ int64) {}