eth: preserve signed integer encodings

api/firmware/eth.go

@@ -620,15 +620,26 @@ func parseTypedMessage(jsonMsg []byte) (*ethTypedMessage, []*messages.ETHSignTyp
 
 // Golang's stdlib doesn't support serializing signed integers in big endian (two's complement).
 // -x = ~x+1.
+// Return the shortest encoding that includes a sign bit, including one byte for zero.
 func bigendianInt(integer *big.Int) []byte {
 	if integer.Sign() >= 0 {
-		return integer.Bytes()
+		bytes := integer.Bytes()
+		// Add a sign byte if needed; encode zero as a single zero byte.
+		if len(bytes) == 0 || bytes[0]&0x80 != 0 {
+			return append([]byte{0}, bytes...)
+		}
+		return bytes
 	}
 	bytes := append([]byte{0}, integer.Bytes()...)
 	for i, v := range bytes {
 		bytes[i] = ^v
 	}
-	return new(big.Int).Add(new(big.Int).SetBytes(bytes), big.NewInt(1)).Bytes()
+	bytes = new(big.Int).Add(new(big.Int).SetBytes(bytes), big.NewInt(1)).Bytes()
+	// Drop redundant sign extension, e.g. ff80 -> 80 for -128.
+	if len(bytes) > 1 && bytes[0] == 0xff && bytes[1]&0x80 != 0 {
+		bytes = bytes[1:]
+	}
+	return bytes
 }
 
 // encodeValue encodes a json decoded typed data value to send to the BitBox02 as part of the

api/firmware/eth_test.go

@@ -4,6 +4,7 @@ package firmware
 
 import (
 	"bytes"
+	"fmt"
 	"math/big"
 	"testing"
 
@@ -300,7 +301,7 @@ func TestEncodeValue(t *testing.T) {
 
 	encoded, err = encodeValue(parseTypeNoErr(t, "int64", nil), float64(2983742332))
 	require.NoError(t, err)
-	require.Equal(t, []byte("\xb1\xd8\x4b\x7c"), encoded)
+	require.Equal(t, []byte("\x00\xb1\xd8\x4b\x7c"), encoded)
 
 	encoded, err = encodeValue(parseTypeNoErr(t, "int64", nil), float64(-2983742332))
 	require.NoError(t, err)
@@ -344,6 +345,49 @@ func TestEncodeValue(t *testing.T) {
 	}
 }
 
+func TestEncodeValueSignedIntegers(t *testing.T) {
+	for _, test := range []struct {
+		value int64
+		hex   string
+	}{
+		{0, "00"}, {1, "01"}, {-1, "ff"},
+		{127, "7f"}, {128, "0080"}, {129, "0081"},
+		{-127, "81"}, {-128, "80"}, {-129, "ff7f"},
+		{255, "00ff"}, {256, "0100"}, {-255, "ff01"}, {-256, "ff00"},
+		{32767, "7fff"}, {32768, "008000"},
+		{-32768, "8000"}, {-32769, "ff7fff"},
+		{65535, "00ffff"}, {65536, "010000"},
+	} {
+		decimal := big.NewInt(test.value).String()
+		t.Run(decimal, func(t *testing.T) {
+			for _, input := range []interface{}{decimal, float64(test.value)} {
+				encoded, err := encodeValue(parseTypeNoErr(t, "int64", nil), input)
+				require.NoError(t, err)
+				require.Equal(t, test.hex, fmt.Sprintf("%x", encoded))
+			}
+		})
+	}
+}
+
+func TestBigendianIntSignedWidths(t *testing.T) {
+	for size := 1; size <= 32; size++ {
+		limit := new(big.Int).Lsh(big.NewInt(1), uint(8*size-1))
+		maximum := new(big.Int).Sub(limit, big.NewInt(1))
+		minimum := new(big.Int).Neg(limit)
+		for _, value := range []*big.Int{minimum, maximum} {
+			original := new(big.Int).Set(value)
+			encoded := bigendianInt(value)
+			require.Len(t, encoded, size)
+			decoded := new(big.Int).SetBytes(encoded)
+			if encoded[0]&0x80 != 0 {
+				decoded.Sub(decoded, new(big.Int).Lsh(big.NewInt(1), uint(8*size)))
+			}
+			require.Zero(t, decoded.Cmp(value))
+			require.Zero(t, original.Cmp(value), "encoding must not mutate its input")
+		}
+	}
+}
+
 func TestHandleETHDataStreamingOutOfBounds(t *testing.T) {
 	device := &Device{}
 	_, err := device.nonAtomicHandleETHDataStreaming(
@@ -559,6 +603,44 @@ func TestSimulatorETHSignTypedMessage(t *testing.T) {
 	})
 }
 
+func TestSimulatorETHSignTypedMessageSignedIntegers(t *testing.T) {
+	testInitializedSimulators(t, func(t *testing.T, device *Device, stdOut *simulatorStdout) {
+		t.Helper()
+		keypath := []uint32{44 + hardenedKeyStart, 60 + hardenedKeyStart, hardenedKeyStart, 0, 10}
+		pubKey := simulatorPub(t, device, keypath...)
+		sig, err := device.ETHSignTypedMessage(1, keypath, []byte(`{
+			"types": {
+				"EIP712Domain": [{"name": "name", "type": "string"}],
+				"SignedIntegers": [
+					{"name": "positive", "type": "int16"},
+					{"name": "negative", "type": "int8"},
+					{"name": "zero", "type": "int8"}
+				]
+			},
+			"primaryType": "SignedIntegers",
+			"domain": {"name": "Signed integers"},
+			"message": {"positive": 128, "negative": -128, "zero": 0}
+		}`), true)
+		require.NoError(t, err)
+		require.Len(t, sig, 65)
+
+		// Compute the EIP-712 digest independently of the wire encoder. The device
+		// must sign +128, -128 and zero, not reinterpret or reject their sign bytes.
+		domainHash := hashKeccak(bytes.Join([][]byte{
+			hashKeccak([]byte("EIP712Domain(string name)")),
+			hashKeccak([]byte("Signed integers")),
+		}, nil))
+		messageHash := hashKeccak(bytes.Join([][]byte{
+			hashKeccak([]byte("SignedIntegers(int16 positive,int8 negative,int8 zero)")),
+			append(make([]byte, 31), 0x80),
+			append(bytes.Repeat([]byte{0xff}, 31), 0x80),
+			make([]byte, 32),
+		}, nil))
+		digest := hashKeccak(bytes.Join([][]byte{{0x19, 0x01}, domainHash, messageHash}, nil))
+		require.True(t, parseECDSASignature(t, sig[:64]).Verify(digest, pubKey))
+	})
+}
+
 func TestSimulatorETHSignTypedMessageAntikleptoEnabled(t *testing.T) {
 	testInitializedSimulators(t, func(t *testing.T, device *Device, stdOut *simulatorStdout) {
 		t.Helper()