From 0349e7e463521b63fa6e618d80770fbe6176bb02 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Thu, 28 Nov 2024 16:16:04 +0100 Subject: [PATCH 01/13] feat(meta): secretbox encryption in place of aes-gcm --- pkg/meta/internal/crypto/aes.go | 132 ------------------ pkg/meta/internal/crypto/secretbox.go | 89 ++++++++++++ .../crypto/{aes_test.go => secretbox_test.go} | 62 +++++--- pkg/meta/meta.go | 8 +- 4 files changed, 134 insertions(+), 157 deletions(-) delete mode 100644 pkg/meta/internal/crypto/aes.go create mode 100644 pkg/meta/internal/crypto/secretbox.go rename pkg/meta/internal/crypto/{aes_test.go => secretbox_test.go} (53%) diff --git a/pkg/meta/internal/crypto/aes.go b/pkg/meta/internal/crypto/aes.go deleted file mode 100644 index 482402e..0000000 --- a/pkg/meta/internal/crypto/aes.go +++ /dev/null @@ -1,132 +0,0 @@ -package crypto - -import ( - "crypto/aes" - "crypto/cipher" - "crypto/rand" - "errors" - "fmt" - "io" -) - -// KeySize represents valid AES key sizes -type KeySize int - -const ( - KeySize128 KeySize = 16 // AES-128 - KeySize192 KeySize = 24 // AES-192 - KeySize256 KeySize = 32 // AES-256 (recommended) -) - -// IsValid returns true if the key size is valid for AES -func (ks KeySize) IsValid() bool { - switch ks { - case KeySize128, KeySize192, KeySize256: - return true - default: - return false - } -} - -var ErrShortCipherText = errors.New("ciphertext too short") -var ErrNoEncryptionKey = errors.New("encryption key is required") -var ErrInvalidKeySize = errors.New("invalid key size: must be 16, 24, or 32 bytes") -var ErrZeroKey = errors.New("encryption key cannot be all zeros") - -// GenerateKey generates a random AES key of default size KeySize256 (32 bytes). -// Returns an error if the specified size is invalid or if key generation fails. -func GenerateKey() ([]byte, error) { - return GenerateKeyWithSize(KeySize256) -} - -// GenerateKeyWithSize generates a random AES key of the specified size. -// Returns an error if the specified size is invalid or if key generation fails. -func GenerateKeyWithSize(size KeySize) ([]byte, error) { - if !size.IsValid() { - return nil, ErrInvalidKeySize - } - - key := make([]byte, size) - if _, err := io.ReadFull(rand.Reader, key); err != nil { - return nil, fmt.Errorf("failed to generate AES key: %w", err) - } - - return key, nil -} - -// EncryptWithAESKey encrypts data using AES-GCM with the provided key. -// The key must be 16, 24, or 32 bytes long (for AES-128, AES-192, or AES-256). -// Returns the encrypted data with the nonce prepended, or an error if encryption fails. -func EncryptWithAESKey(data, key []byte) ([]byte, error) { - if err := validateAESKey(key); err != nil { - return nil, err - } - - block, err := aes.NewCipher(key) - if err != nil { - return nil, err - } - - gcm, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - - nonce := make([]byte, gcm.NonceSize()) - if _, err = io.ReadFull(rand.Reader, nonce); err != nil { - return nil, err - } - - return gcm.Seal(nonce, nonce, data, nil), nil -} - -// DecryptStringWithAESKey decrypts data that was encrypted with EncryptWithAESKey. -// The key must match the one used for encryption. -// Expects the input to have a prepended nonce. -// Returns the decrypted data or an error if decryption fails. -func DecryptStringWithAESKey(data, key []byte) ([]byte, error) { - if err := validateAESKey(key); err != nil { - return nil, err - } - - block, err := aes.NewCipher(key) - if err != nil { - return nil, err - } - - gcm, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - - if len(data) < gcm.NonceSize() { - return nil, ErrShortCipherText - } - - nonce, ciphertext := data[:gcm.NonceSize()], data[gcm.NonceSize():] - decrypted, err := gcm.Open(nil, nonce, ciphertext, nil) - if err != nil { - return nil, err - } - - return decrypted, nil -} - -func validateAESKey(key []byte) error { - if key == nil { - return ErrNoEncryptionKey - } - - if !KeySize(len(key)).IsValid() { - return ErrInvalidKeySize - } - - // check if key is all zeros - for _, b := range key { - if b != 0 { - return nil - } - } - - return ErrZeroKey -} diff --git a/pkg/meta/internal/crypto/secretbox.go b/pkg/meta/internal/crypto/secretbox.go new file mode 100644 index 0000000..70d223c --- /dev/null +++ b/pkg/meta/internal/crypto/secretbox.go @@ -0,0 +1,89 @@ +package crypto + +import ( + "crypto/rand" + "errors" + "fmt" + "io" + + "golang.org/x/crypto/nacl/secretbox" +) + +const keySize = 32 // secretbox allows only 32-byte keys + +var ErrShortCipherText = errors.New("ciphertext too short") +var ErrNoEncryptionKey = errors.New("encryption key is required") +var ErrInvalidKeySize = errors.New("invalid key size: must be 32 bytes") +var ErrZeroKey = errors.New("encryption key cannot be all zeros") + +// GenerateKey generates a random 32-byte key to be used by EncryptWithKey and DecryptWithKey +func GenerateKey() ([]byte, error) { + key := make([]byte, keySize) + if _, err := io.ReadFull(rand.Reader, key); err != nil { + return nil, fmt.Errorf("failed to generate key: %w", err) + } + return key, nil +} + +// EncryptWithKey encrypts data using secretbox with the provided key +func EncryptWithKey(data, key []byte) ([]byte, error) { + if err := validateKey(key); err != nil { + return nil, err + } + + var secretKey [keySize]byte + copy(secretKey[:], key) + + // Generate 24 bytes of random data as nonce + var nonce [24]byte + if _, err := io.ReadFull(rand.Reader, nonce[:]); err != nil { + return nil, err + } + + // Encrypt and authenticate data + encrypted := secretbox.Seal(nonce[:], data, &nonce, &secretKey) + return encrypted, nil +} + +// DecryptStringWithKey decrypts data using secretbox with the provided key +func DecryptStringWithKey(data, key []byte) ([]byte, error) { + if err := validateKey(key); err != nil { + return nil, err + } + + if len(data) < 24 { + return nil, ErrShortCipherText + } + + var secretKey [keySize]byte + copy(secretKey[:], key) + + var nonce [24]byte + copy(nonce[:], data[:24]) + + decrypted, ok := secretbox.Open(nil, data[24:], &nonce, &secretKey) + if !ok { + return nil, errors.New("decryption failed") + } + + return decrypted, nil +} + +func validateKey(key []byte) error { + if key == nil { + return ErrNoEncryptionKey + } + + if len(key) != keySize { + return ErrInvalidKeySize + } + + // check if key is all zeros + for _, b := range key { + if b != 0 { + return nil + } + } + + return ErrZeroKey +} diff --git a/pkg/meta/internal/crypto/aes_test.go b/pkg/meta/internal/crypto/secretbox_test.go similarity index 53% rename from pkg/meta/internal/crypto/aes_test.go rename to pkg/meta/internal/crypto/secretbox_test.go index 1d0d3e4..d87f860 100644 --- a/pkg/meta/internal/crypto/aes_test.go +++ b/pkg/meta/internal/crypto/secretbox_test.go @@ -8,10 +8,10 @@ import ( "github.com/stretchr/testify/require" ) -func TestAESEncryption(t *testing.T) { +func TestSecretBoxEncryption(t *testing.T) { t.Parallel() - key := make([]byte, 32) // generated random 32-byte key + key := make([]byte, keySize) // generate random 32-byte key _, errKey := rand.Read(key) require.NoError(t, errKey) @@ -40,13 +40,13 @@ func TestAESEncryption(t *testing.T) { { name: "invalid key size", data: []byte("hello world"), - key: make([]byte, 31), + key: make([]byte, 16), // Only 32 bytes allowed now wantErr: ErrInvalidKeySize, }, { name: "zero key returns error", data: []byte("hello world"), - key: make([]byte, 32), + key: make([]byte, keySize), wantErr: ErrZeroKey, }, } @@ -56,24 +56,22 @@ func TestAESEncryption(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - encrypted, err := EncryptWithAESKey(tt.data, tt.key) + encrypted, err := EncryptWithKey(tt.data, tt.key) if tt.wantErr != nil { require.ErrorIs(t, err, tt.wantErr) - return } require.NoError(t, err) - decrypted, err := DecryptStringWithAESKey(encrypted, tt.key) - require.NoError(t, err) - - if tt.key == nil { - require.Equal(t, tt.data, encrypted) - require.Equal(t, tt.data, decrypted) - } else { - require.NotEqual(t, tt.data, encrypted) - require.True(t, bytes.Equal(tt.data, decrypted)) + // Verify encrypted data is different and includes nonce + require.Greater(t, len(encrypted), 24) // At least nonce size + if len(tt.data) > 0 { + require.NotEqual(t, tt.data, encrypted[24:]) // Ignore nonce prefix } + + decrypted, err := DecryptStringWithKey(encrypted, tt.key) + require.NoError(t, err) + require.True(t, bytes.Equal(tt.data, decrypted)) }) } } @@ -81,10 +79,15 @@ func TestAESEncryption(t *testing.T) { func TestDecryptionErrors(t *testing.T) { t.Parallel() - key := make([]byte, 32) + key := make([]byte, keySize) _, err := rand.Read(key) require.NoError(t, err) + // Create valid encrypted data for tampering tests + validData := []byte("test message") + encrypted, err := EncryptWithKey(validData, key) + require.NoError(t, err) + tests := []struct { name string data []byte @@ -93,19 +96,25 @@ func TestDecryptionErrors(t *testing.T) { }{ { name: "short ciphertext", - data: []byte("short"), + data: make([]byte, 23), // Less than nonce size key: key, errMsg: "ciphertext too short", }, { name: "invalid ciphertext", - data: make([]byte, 16), // just nonce size + data: make([]byte, 24), // Just nonce size key: key, - errMsg: "message authentication failed", + errMsg: "decryption failed", + }, + { + name: "tampered ciphertext", + data: tamperWithBytes(encrypted), + key: key, + errMsg: "decryption failed", }, { name: "missing key", - data: []byte("�`M���l\u001AIF�\u0012���=h�?�c� ��\u0012����\u001C�\u0018Ƽ(g"), + data: encrypted, key: nil, errMsg: "encryption key is required", }, @@ -116,9 +125,20 @@ func TestDecryptionErrors(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := DecryptStringWithAESKey(tt.data, tt.key) + _, err := DecryptStringWithKey(tt.data, tt.key) require.Error(t, err) require.Contains(t, err.Error(), tt.errMsg) }) } } + +// tamperWithBytes modifies a byte in the encrypted data to simulate tampering +func tamperWithBytes(data []byte) []byte { + if len(data) < 25 { // Need at least nonce + 1 byte + return data + } + tampered := make([]byte, len(data)) + copy(tampered, data) + tampered[24] ^= 0x01 // Modify first byte after nonce + return tampered +} diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 3c54738..21d56a6 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -63,7 +63,7 @@ func (m *Meta) GetEncryptedString(key string, encryptionKey []byte) (string, err return "", err } - decrypted, err := crypto.DecryptStringWithAESKey(v, encryptionKey) + decrypted, err := crypto.DecryptStringWithKey(v, encryptionKey) if err != nil { return "", err } @@ -111,7 +111,7 @@ func (m *Meta) GetEncryptedBytes(key string, encryptionKey []byte) ([]byte, erro return nil, err } - decrypted, err := crypto.DecryptStringWithAESKey(v, encryptionKey) + decrypted, err := crypto.DecryptStringWithKey(v, encryptionKey) if err != nil { return nil, err } @@ -156,12 +156,12 @@ func (m *Meta) AddEncrypted(key string, val any, encryptionKey []byte) error { switch val := val.(type) { case string: - encrypted, err = crypto.EncryptWithAESKey([]byte(val), encryptionKey) + encrypted, err = crypto.EncryptWithKey([]byte(val), encryptionKey) if err != nil { return err } case []byte: - encrypted, err = crypto.EncryptWithAESKey(val, encryptionKey) + encrypted, err = crypto.EncryptWithKey(val, encryptionKey) if err != nil { return err } From 200d6a8ae25563ffb3250021ad9a1a2310ba8dbe Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Thu, 28 Nov 2024 17:17:10 +0100 Subject: [PATCH 02/13] benchmarks vs aes-gcm --- pkg/meta/internal/crypto/secretbox_test.go | 149 +++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/pkg/meta/internal/crypto/secretbox_test.go b/pkg/meta/internal/crypto/secretbox_test.go index d87f860..09d3831 100644 --- a/pkg/meta/internal/crypto/secretbox_test.go +++ b/pkg/meta/internal/crypto/secretbox_test.go @@ -2,7 +2,12 @@ package crypto import ( "bytes" + "crypto/aes" + "crypto/cipher" "crypto/rand" + "errors" + "fmt" + "io" "testing" "github.com/stretchr/testify/require" @@ -142,3 +147,147 @@ func tamperWithBytes(data []byte) []byte { tampered[24] ^= 0x01 // Modify first byte after nonce return tampered } + +func encryptWithAESKey(data, key []byte) ([]byte, error) { + if err := validateKey(key); err != nil { + return nil, err + } + + block, err := aes.NewCipher(key) + if err != nil { + return nil, err + } + + aesGCM, err := cipher.NewGCM(block) + if err != nil { + return nil, err + } + + nonce := make([]byte, aesGCM.NonceSize()) + if _, err := io.ReadFull(rand.Reader, nonce); err != nil { + return nil, err + } + + return aesGCM.Seal(nonce, nonce, data, nil), nil +} + +func decryptWithAESKey(data, key []byte) ([]byte, error) { + if err := validateKey(key); err != nil { + return nil, err + } + + block, err := aes.NewCipher(key) + if err != nil { + return nil, err + } + + aesGCM, err := cipher.NewGCM(block) + if err != nil { + return nil, err + } + + nonceSize := aesGCM.NonceSize() + if len(data) < nonceSize { + return nil, ErrShortCipherText + } + + nonce, ciphertext := data[:nonceSize], data[nonceSize:] + plaintext, err := aesGCM.Open(nil, nonce, ciphertext, nil) + if err != nil { + return nil, errors.New("decryption failed") + } + + return plaintext, nil +} + +func BenchmarkEncryption(b *testing.B) { + key := make([]byte, keySize) + _, err := rand.Read(key) + require.NoError(b, err) + + sizes := []int{16, 64, 256, 1024, 4096} // Test different payload sizes + for _, size := range sizes { + data := make([]byte, size) + _, err := rand.Read(data) + require.NoError(b, err) + + b.Run(fmt.Sprintf("Secretbox-%dB", size), func(b *testing.B) { + for i := 0; i < b.N; i++ { + encrypted, err := EncryptWithKey(data, key) + require.NoError(b, err) + b.SetBytes(int64(len(encrypted))) + } + }) + + b.Run(fmt.Sprintf("AES-GCM-%dB", size), func(b *testing.B) { + for i := 0; i < b.N; i++ { + encrypted, err := encryptWithAESKey(data, key) + require.NoError(b, err) + b.SetBytes(int64(len(encrypted))) + } + }) + } +} + +func BenchmarkDecryption(b *testing.B) { + key := make([]byte, keySize) + _, err := rand.Read(key) + require.NoError(b, err) + + sizes := []int{16, 64, 256, 1024, 4096} + for _, size := range sizes { + data := make([]byte, size) + _, err := rand.Read(data) + require.NoError(b, err) + + secretboxEncrypted, err := EncryptWithKey(data, key) + require.NoError(b, err) + + aesGCMEncrypted, err := encryptWithAESKey(data, key) + require.NoError(b, err) + + b.Run(fmt.Sprintf("Secretbox-%dB", size), func(b *testing.B) { + for i := 0; i < b.N; i++ { + decrypted, err := DecryptStringWithKey(secretboxEncrypted, key) + require.NoError(b, err) + b.SetBytes(int64(len(decrypted))) + } + }) + + b.Run(fmt.Sprintf("AES-GCM-%dB", size), func(b *testing.B) { + for i := 0; i < b.N; i++ { + decrypted, err := decryptWithAESKey(aesGCMEncrypted, key) + require.NoError(b, err) + b.SetBytes(int64(len(decrypted))) + } + }) + } +} + +// TestCiphertextSizeComparison shows that Secretbox encryption entails +// a slightly larger ciphertext overhead of 40 bytes, compared to AES-GCM, +// whose overhead is just 28 bytes. +func TestCiphertextSizeComparison(t *testing.T) { + key := make([]byte, keySize) + _, err := rand.Read(key) + require.NoError(t, err) + + sizes := []int{0, 16, 64, 256, 1024, 4096} + for _, size := range sizes { + t.Run(fmt.Sprintf("size-%d", size), func(t *testing.T) { + data := make([]byte, size) + _, err := rand.Read(data) + require.NoError(t, err) + + sbCiphertext, err := EncryptWithKey(data, key) + require.NoError(t, err) + + aesCiphertext, err := encryptWithAESKey(data, key) + require.NoError(t, err) + + t.Logf("Input size: %d bytes", size) + t.Logf("Secretbox size: %d bytes (overhead: %d bytes)", len(sbCiphertext), len(sbCiphertext)-size) + t.Logf("AES-GCM size: %d bytes (overhead: %d bytes)", len(aesCiphertext), len(aesCiphertext)-size) + }) + } +} From 3997a8618482b1e6b159e21c0ab57dc311176e73 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Fri, 29 Nov 2024 13:00:00 +0100 Subject: [PATCH 03/13] fix: prevent overflow of int values --- pkg/policy/literal/literal.go | 13 ++++++- pkg/policy/literal/literal_test.go | 56 +++++++++++++++++++++++++++- pkg/policy/match.go | 24 ++++++++++++ pkg/policy/match_test.go | 36 ++++++++++++++++++ pkg/policy/selector/parsing.go | 11 ++++++ pkg/policy/selector/parsing_test.go | 21 +++++++++++ pkg/policy/selector/selector.go | 22 +++++++++-- pkg/policy/selector/selector_test.go | 55 +++++++++++++++++++++++++++ 8 files changed, 232 insertions(+), 6 deletions(-) diff --git a/pkg/policy/literal/literal.go b/pkg/policy/literal/literal.go index 33b0904..b96619d 100644 --- a/pkg/policy/literal/literal.go +++ b/pkg/policy/literal/literal.go @@ -12,6 +12,7 @@ import ( "github.com/ipld/go-ipld-prime/fluent/qp" cidlink "github.com/ipld/go-ipld-prime/linking/cid" "github.com/ipld/go-ipld-prime/node/basicnode" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) var Bool = basicnode.NewBool @@ -168,9 +169,17 @@ func anyAssemble(val any) qp.Assemble { case reflect.Bool: return qp.Bool(rv.Bool()) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return qp.Int(rv.Int()) + i := rv.Int() + if i > limits.MaxInt53 || i < limits.MinInt53 { + panic(fmt.Sprintf("integer %d exceeds safe bounds", i)) + } + return qp.Int(i) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return qp.Int(int64(rv.Uint())) + u := rv.Uint() + if u > limits.MaxInt53 { + panic(fmt.Sprintf("unsigned integer %d exceeds safe bounds", u)) + } + return qp.Int(int64(u)) case reflect.Float32, reflect.Float64: return qp.Float(rv.Float()) case reflect.String: diff --git a/pkg/policy/literal/literal_test.go b/pkg/policy/literal/literal_test.go index 45d7c6c..d2282d0 100644 --- a/pkg/policy/literal/literal_test.go +++ b/pkg/policy/literal/literal_test.go @@ -8,6 +8,7 @@ import ( cidlink "github.com/ipld/go-ipld-prime/linking/cid" "github.com/ipld/go-ipld-prime/printer" "github.com/stretchr/testify/require" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) func TestList(t *testing.T) { @@ -214,7 +215,7 @@ func TestAny(t *testing.T) { require.NoError(t, err) require.True(t, asLink.(cidlink.Link).Equals(cid.MustParse("bafzbeigai3eoy2ccc7ybwjfz5r3rdxqrinwi4rwytly24tdbh6yk7zslrm"))) - v, err = Any(data["func"]) + _, err = Any(data["func"]) require.Error(t, err) } @@ -254,6 +255,59 @@ func BenchmarkAny(b *testing.B) { }) } +func TestAnyAssembleIntegerOverflow(t *testing.T) { + tests := []struct { + name string + input interface{} + shouldErr bool + }{ + { + name: "valid int", + input: 42, + shouldErr: false, + }, + { + name: "max safe int", + input: limits.MaxInt53, + shouldErr: false, + }, + { + name: "min safe int", + input: limits.MinInt53, + shouldErr: false, + }, + { + name: "overflow int", + input: int64(limits.MaxInt53 + 1), + shouldErr: true, + }, + { + name: "underflow int", + input: int64(limits.MinInt53 - 1), + shouldErr: true, + }, + { + name: "overflow uint", + input: uint64(limits.MaxInt53 + 1), + shouldErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.shouldErr { + require.Panics(t, func() { + anyAssemble(tt.input) + }) + } else { + require.NotPanics(t, func() { + anyAssemble(tt.input) + }) + } + }) + } +} + func must[T any](t T, err error) T { if err != nil { panic(err) diff --git a/pkg/policy/match.go b/pkg/policy/match.go index 59316ed..81a5431 100644 --- a/pkg/policy/match.go +++ b/pkg/policy/match.go @@ -3,10 +3,13 @@ package policy import ( "cmp" "fmt" + "math" "github.com/ipld/go-ipld-prime" "github.com/ipld/go-ipld-prime/datamodel" "github.com/ipld/go-ipld-prime/must" + + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) // Match determines if the IPLD node satisfies the policy. @@ -249,10 +252,26 @@ func matchStatement(cur Statement, node ipld.Node) (_ matchResult, leafMost Stat panic(fmt.Errorf("unimplemented statement kind: %s", cur.Kind())) } +// isOrdered compares two IPLD nodes and returns true if they satisfy the given ordering function. +// It supports comparison of integers and floats, returning false for: +// - Nodes of different or unsupported kinds +// - Integer values outside JavaScript's safe integer bounds (±2^53-1) +// - Non-finite floating point values (NaN or ±Inf) +// +// The satisfies parameter is a function that interprets the comparison result: +// - For ">" it returns true when order is 1 +// - For ">=" it returns true when order is 0 or 1 +// - For "<" it returns true when order is -1 +// - For "<=" it returns true when order is -1 or 0 func isOrdered(expected ipld.Node, actual ipld.Node, satisfies func(order int) bool) bool { if expected.Kind() == ipld.Kind_Int && actual.Kind() == ipld.Kind_Int { a := must.Int(actual) b := must.Int(expected) + + if a > limits.MaxInt53 || a < limits.MinInt53 || b > limits.MaxInt53 || b < limits.MinInt53 { + return false + } + return satisfies(cmp.Compare(a, b)) } @@ -265,6 +284,11 @@ func isOrdered(expected ipld.Node, actual ipld.Node, satisfies func(order int) b if err != nil { panic(fmt.Errorf("extracting selector float: %w", err)) } + + if math.IsInf(a, 0) || math.IsNaN(a) || math.IsInf(b, 0) || math.IsNaN(b) { + return false + } + return satisfies(cmp.Compare(a, b)) } diff --git a/pkg/policy/match_test.go b/pkg/policy/match_test.go index 108037a..89bf02d 100644 --- a/pkg/policy/match_test.go +++ b/pkg/policy/match_test.go @@ -11,6 +11,7 @@ import ( "github.com/ipld/go-ipld-prime/node/basicnode" "github.com/stretchr/testify/require" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/pkg/policy/literal" ) @@ -901,3 +902,38 @@ func TestPartialMatch(t *testing.T) { }) } } + +func TestIntegerOverflow(t *testing.T) { + tests := []struct { + name string + expected ipld.Node + actual ipld.Node + want bool + }{ + { + name: "valid integers", + expected: literal.Int(42), + actual: literal.Int(43), + want: true, // for gt comparison + }, + { + name: "exceeds MaxInt53", + expected: literal.Int(limits.MaxInt53 + 1), + actual: literal.Int(42), + want: false, + }, + { + name: "below MinInt53", + expected: literal.Int(limits.MinInt53 - 1), + actual: literal.Int(42), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isOrdered(tt.expected, tt.actual, gt) + require.Equal(t, tt.want, result) + }) + } +} diff --git a/pkg/policy/selector/parsing.go b/pkg/policy/selector/parsing.go index 05ab092..5ccc771 100644 --- a/pkg/policy/selector/parsing.go +++ b/pkg/policy/selector/parsing.go @@ -6,6 +6,8 @@ import ( "regexp" "strconv" "strings" + + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) var ( @@ -56,6 +58,9 @@ func Parse(str string) (Selector, error) { if err != nil { return nil, newParseError("invalid index", str, col, tok) } + if idx > limits.MaxInt53 || idx < limits.MinInt53 { + return nil, newParseError(fmt.Sprintf("index %d exceeds safe integer bounds", idx), str, col, tok) + } sel = append(sel, segment{str: tok, optional: opt, index: idx}) // explicit field, ["abcd"] @@ -77,6 +82,9 @@ func Parse(str string) (Selector, error) { if err != nil { return nil, newParseError("invalid slice index", str, col, tok) } + if i > limits.MaxInt53 || i < limits.MinInt53 { + return nil, newParseError(fmt.Sprintf("slice index %d exceeds safe integer bounds", i), str, col, tok) + } rng[0] = i } if splt[1] == "" { @@ -86,6 +94,9 @@ func Parse(str string) (Selector, error) { if err != nil { return nil, newParseError("invalid slice index", str, col, tok) } + if i > limits.MaxInt53 || i < limits.MinInt53 { + return nil, newParseError(fmt.Sprintf("slice index %d exceeds safe integer bounds", i), str, col, tok) + } rng[1] = i } sel = append(sel, segment{str: tok, optional: opt, slice: rng[:]}) diff --git a/pkg/policy/selector/parsing_test.go b/pkg/policy/selector/parsing_test.go index b84ad52..2810227 100644 --- a/pkg/policy/selector/parsing_test.go +++ b/pkg/policy/selector/parsing_test.go @@ -1,10 +1,12 @@ package selector import ( + "fmt" "math" "testing" "github.com/stretchr/testify/require" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) func TestParse(t *testing.T) { @@ -572,4 +574,23 @@ func TestParse(t *testing.T) { _, err := Parse(".[foo]") require.Error(t, err) }) + + t.Run("integer overflow", func(t *testing.T) { + sel, err := Parse(fmt.Sprintf(".[%d]", limits.MaxInt53+1)) + require.Error(t, err) + require.Nil(t, sel) + + sel, err = Parse(fmt.Sprintf(".[%d]", limits.MinInt53-1)) + require.Error(t, err) + require.Nil(t, sel) + + // Test slice overflow + sel, err = Parse(fmt.Sprintf(".[%d:42]", limits.MaxInt53+1)) + require.Error(t, err) + require.Nil(t, sel) + + sel, err = Parse(fmt.Sprintf(".[1:%d]", limits.MaxInt53+1)) + require.Error(t, err) + require.Nil(t, sel) + }) } diff --git a/pkg/policy/selector/selector.go b/pkg/policy/selector/selector.go index 149078d..249cd44 100644 --- a/pkg/policy/selector/selector.go +++ b/pkg/policy/selector/selector.go @@ -266,19 +266,32 @@ func resolveSliceIndices(slice []int64, length int64) (start int64, end int64) { case slice[0] == math.MinInt: start = 0 case slice[0] < 0: - start = length + slice[0] + // Check for potential overflow before adding + if -slice[0] > length { + start = 0 + } else { + start = length + slice[0] + } } + switch { case slice[1] == math.MaxInt: end = length case slice[1] < 0: - end = length + slice[1] + // Check for potential overflow before adding + if -slice[1] > length { + end = 0 + } else { + end = length + slice[1] + } } // backward iteration is not allowed, shortcut to an empty result if start >= end { start, end = 0, 0 + return } + // clamp out of bound if start < 0 { start = 0 @@ -286,11 +299,14 @@ func resolveSliceIndices(slice []int64, length int64) (start int64, end int64) { if start > length { start = length } + if end < 0 { + end = 0 + } if end > length { end = length } - return start, end + return } func kindString(n datamodel.Node) string { diff --git a/pkg/policy/selector/selector_test.go b/pkg/policy/selector/selector_test.go index 184b7b3..fdd18ec 100644 --- a/pkg/policy/selector/selector_test.go +++ b/pkg/policy/selector/selector_test.go @@ -2,6 +2,7 @@ package selector import ( "errors" + "math" "strings" "testing" @@ -356,3 +357,57 @@ func FuzzParseAndSelect(f *testing.F) { } }) } + +func TestResolveSliceIndices(t *testing.T) { + tests := []struct { + name string + slice []int64 + length int64 + wantStart int64 + wantEnd int64 + }{ + { + name: "normal case", + slice: []int64{1, 3}, + length: 5, + wantStart: 1, + wantEnd: 3, + }, + { + name: "negative indices", + slice: []int64{-2, -1}, + length: 5, + wantStart: 3, + wantEnd: 4, + }, + { + name: "overflow protection negative start", + slice: []int64{math.MinInt64, 3}, + length: 5, + wantStart: 0, + wantEnd: 3, + }, + { + name: "overflow protection negative end", + slice: []int64{0, math.MinInt64}, + length: 5, + wantStart: 0, + wantEnd: 0, + }, + { + name: "max bounds", + slice: []int64{0, math.MaxInt64}, + length: 5, + wantStart: 0, + wantEnd: 5, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + start, end := resolveSliceIndices(tt.slice, tt.length) + require.Equal(t, tt.wantStart, start) + require.Equal(t, tt.wantEnd, end) + }) + } +} From ff79bbb44335f05da51b9141cd00a0b9b8130d3f Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Fri, 29 Nov 2024 13:03:48 +0100 Subject: [PATCH 04/13] go fmt --- pkg/policy/literal/literal.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/policy/literal/literal.go b/pkg/policy/literal/literal.go index b96619d..618f33d 100644 --- a/pkg/policy/literal/literal.go +++ b/pkg/policy/literal/literal.go @@ -12,6 +12,7 @@ import ( "github.com/ipld/go-ipld-prime/fluent/qp" cidlink "github.com/ipld/go-ipld-prime/linking/cid" "github.com/ipld/go-ipld-prime/node/basicnode" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) From bff482f73b98f18bfb7a6fb8c0b9a86af5697f03 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Fri, 29 Nov 2024 13:04:14 +0100 Subject: [PATCH 05/13] add constants.go --- pkg/policy/limits/constants.go | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 pkg/policy/limits/constants.go diff --git a/pkg/policy/limits/constants.go b/pkg/policy/limits/constants.go new file mode 100644 index 0000000..c2d1fd5 --- /dev/null +++ b/pkg/policy/limits/constants.go @@ -0,0 +1,8 @@ +package limits + +const ( + // MaxInt53 represents the maximum safe integer in JavaScript (2^53 - 1) + MaxInt53 = 9007199254740991 + // MinInt53 represents the minimum safe integer in JavaScript (-2^53 + 1) + MinInt53 = -9007199254740991 +) From a854389e328087f165f5c59563389008bee455a3 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 11:37:03 +0100 Subject: [PATCH 06/13] validate token timestamps integer limits --- token/delegation/delegation.go | 21 +++++++++ token/delegation/delegation_test.go | 71 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/token/delegation/delegation.go b/token/delegation/delegation.go index 959a40a..5548477 100644 --- a/token/delegation/delegation.go +++ b/token/delegation/delegation.go @@ -18,6 +18,7 @@ import ( "github.com/ucan-wg/go-ucan/pkg/command" "github.com/ucan-wg/go-ucan/pkg/meta" "github.com/ucan-wg/go-ucan/pkg/policy" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/token/internal/nonce" "github.com/ucan-wg/go-ucan/token/internal/parse" ) @@ -176,6 +177,18 @@ func (t *Token) validate() error { errs = errors.Join(errs, fmt.Errorf("token nonce too small")) } + if t.notBefore != nil { + if err := validateTimestamp(t.notBefore.Unix(), "nbf"); err != nil { + errs = errors.Join(errs, err) + } + } + + if t.expiration != nil { + if err := validateTimestamp(t.expiration.Unix(), "exp"); err != nil { + errs = errors.Join(errs, err) + } + } + return errs } @@ -224,3 +237,11 @@ func tokenFromModel(m tokenPayloadModel) (*Token, error) { return &tkn, nil } + +func validateTimestamp(ts int64, field string) error { + if ts > limits.MaxInt53 || ts < limits.MinInt53 { + return fmt.Errorf("token %s timestamp %d exceeds safe integer bounds", field, ts) + } + + return nil +} diff --git a/token/delegation/delegation_test.go b/token/delegation/delegation_test.go index 8da08b4..7450bea 100644 --- a/token/delegation/delegation_test.go +++ b/token/delegation/delegation_test.go @@ -11,6 +11,7 @@ import ( "github.com/ucan-wg/go-ucan/did/didtest" "github.com/ucan-wg/go-ucan/pkg/command" "github.com/ucan-wg/go-ucan/pkg/policy" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/token/delegation" ) @@ -207,3 +208,73 @@ func TestEncryptedMeta(t *testing.T) { } }) } + +func TestTokenTimestampBounds(t *testing.T) { + t.Parallel() + + cmd, err := command.Parse("/foo/bar") + require.NoError(t, err) + pol, err := policy.FromDagJson("[]") + require.NoError(t, err) + + tomorrow := time.Now().Add(24 * time.Hour).Unix() + + tests := []struct { + name string + nbf int64 + exp int64 + wantErr bool + }{ + { + name: "valid timestamps", + nbf: tomorrow, + exp: tomorrow + 3600, + wantErr: false, + }, + { + name: "max safe integer", + nbf: tomorrow, + exp: limits.MaxInt53, + wantErr: false, + }, + { + name: "exceeds max safe integer", + nbf: tomorrow, + exp: limits.MaxInt53 + 1, + wantErr: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + _, err = delegation.New(didtest.PersonaAlice.DID(), didtest.PersonaBob.DID(), + cmd, pol, + delegation.WithNotBefore(time.Unix(tt.nbf, 0)), + delegation.WithExpiration(time.Unix(tt.exp, 0)), + ) + + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), "exceeds safe integer bounds") + } else { + require.NoError(t, err) + } + }) + } + + t.Run("nbf overflow", func(t *testing.T) { + t.Parallel() + + futureExp := time.Now().Add(48 * time.Hour).Unix() + _, err := delegation.New(didtest.PersonaAlice.DID(), didtest.PersonaBob.DID(), + cmd, pol, + delegation.WithNotBefore(time.Unix(limits.MaxInt53+1, 0)), + delegation.WithExpiration(time.Unix(futureExp, 0)), + ) + require.Error(t, err) + require.Contains(t, err.Error(), "exceeds safe integer bounds") + }) +} From 28272e6900d37a204121332b1be28416bcef4d8c Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 11:59:16 +0100 Subject: [PATCH 07/13] move int validation to where a error can be returned --- pkg/policy/ipld.go | 5 ++ pkg/policy/limits/constants.go | 8 --- pkg/policy/limits/int.go | 49 ++++++++++++++++++ pkg/policy/limits/int_test.go | 82 ++++++++++++++++++++++++++++++ pkg/policy/literal/literal.go | 12 ++++- pkg/policy/literal/literal_test.go | 10 ++-- pkg/policy/match.go | 6 --- pkg/policy/match_test.go | 36 ------------- pkg/policy/selector/selector.go | 5 ++ 9 files changed, 156 insertions(+), 57 deletions(-) delete mode 100644 pkg/policy/limits/constants.go create mode 100644 pkg/policy/limits/int.go create mode 100644 pkg/policy/limits/int_test.go diff --git a/pkg/policy/ipld.go b/pkg/policy/ipld.go index 9d52d4d..752dd96 100644 --- a/pkg/policy/ipld.go +++ b/pkg/policy/ipld.go @@ -9,10 +9,15 @@ import ( "github.com/ipld/go-ipld-prime/must" "github.com/ipld/go-ipld-prime/node/basicnode" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/pkg/policy/selector" ) func FromIPLD(node datamodel.Node) (Policy, error) { + if err := limits.ValidateIntegerBoundsIPLD(node); err != nil { + return nil, fmt.Errorf("policy contains integer values outside safe bounds: %w", err) + } + return statementsFromIPLD("/", node) } diff --git a/pkg/policy/limits/constants.go b/pkg/policy/limits/constants.go deleted file mode 100644 index c2d1fd5..0000000 --- a/pkg/policy/limits/constants.go +++ /dev/null @@ -1,8 +0,0 @@ -package limits - -const ( - // MaxInt53 represents the maximum safe integer in JavaScript (2^53 - 1) - MaxInt53 = 9007199254740991 - // MinInt53 represents the minimum safe integer in JavaScript (-2^53 + 1) - MinInt53 = -9007199254740991 -) diff --git a/pkg/policy/limits/int.go b/pkg/policy/limits/int.go new file mode 100644 index 0000000..91dd135 --- /dev/null +++ b/pkg/policy/limits/int.go @@ -0,0 +1,49 @@ +package limits + +import ( + "fmt" + + "github.com/ipld/go-ipld-prime" + "github.com/ipld/go-ipld-prime/must" +) + +const ( + // MaxInt53 represents the maximum safe integer in JavaScript (2^53 - 1) + MaxInt53 = 9007199254740991 + // MinInt53 represents the minimum safe integer in JavaScript (-2^53 + 1) + MinInt53 = -9007199254740991 +) + +func ValidateIntegerBoundsIPLD(node ipld.Node) error { + switch node.Kind() { + case ipld.Kind_Int: + val := must.Int(node) + if val > MaxInt53 || val < MinInt53 { + return fmt.Errorf("integer value %d exceeds safe bounds", val) + } + case ipld.Kind_List: + it := node.ListIterator() + for !it.Done() { + _, v, err := it.Next() + if err != nil { + return err + } + if err := ValidateIntegerBoundsIPLD(v); err != nil { + return err + } + } + case ipld.Kind_Map: + it := node.MapIterator() + for !it.Done() { + _, v, err := it.Next() + if err != nil { + return err + } + if err := ValidateIntegerBoundsIPLD(v); err != nil { + return err + } + } + } + + return nil +} diff --git a/pkg/policy/limits/int_test.go b/pkg/policy/limits/int_test.go new file mode 100644 index 0000000..de3f21e --- /dev/null +++ b/pkg/policy/limits/int_test.go @@ -0,0 +1,82 @@ +package limits + +import ( + "testing" + + "github.com/ipld/go-ipld-prime/datamodel" + "github.com/ipld/go-ipld-prime/fluent/qp" + "github.com/ipld/go-ipld-prime/node/basicnode" + "github.com/stretchr/testify/require" +) + +func TestValidateIntegerBoundsIPLD(t *testing.T) { + buildMap := func() datamodel.Node { + nb := basicnode.Prototype.Any.NewBuilder() + qp.Map(1, func(ma datamodel.MapAssembler) { + qp.MapEntry(ma, "foo", qp.Int(MaxInt53+1)) + })(nb) + return nb.Build() + } + + buildList := func() datamodel.Node { + nb := basicnode.Prototype.Any.NewBuilder() + qp.List(1, func(la datamodel.ListAssembler) { + qp.ListEntry(la, qp.Int(MinInt53-1)) + })(nb) + return nb.Build() + } + + tests := []struct { + name string + input datamodel.Node + wantErr bool + }{ + { + name: "valid int", + input: basicnode.NewInt(42), + wantErr: false, + }, + { + name: "max safe int", + input: basicnode.NewInt(MaxInt53), + wantErr: false, + }, + { + name: "min safe int", + input: basicnode.NewInt(MinInt53), + wantErr: false, + }, + { + name: "above MaxInt53", + input: basicnode.NewInt(MaxInt53 + 1), + wantErr: true, + }, + { + name: "below MinInt53", + input: basicnode.NewInt(MinInt53 - 1), + wantErr: true, + }, + { + name: "nested map with invalid int", + input: buildMap(), + wantErr: true, + }, + { + name: "nested list with invalid int", + input: buildList(), + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateIntegerBoundsIPLD(tt.input) + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), "exceeds safe bounds") + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/policy/literal/literal.go b/pkg/policy/literal/literal.go index 618f33d..fcb03cb 100644 --- a/pkg/policy/literal/literal.go +++ b/pkg/policy/literal/literal.go @@ -69,7 +69,11 @@ func Any(v any) (res ipld.Node, err error) { case string: return basicnode.NewString(val), nil case int: - return basicnode.NewInt(int64(val)), nil + i := int64(val) + if i > limits.MaxInt53 || i < limits.MinInt53 { + return nil, fmt.Errorf("integer value %d exceeds safe integer bounds", i) + } + return basicnode.NewInt(i), nil case int8: return basicnode.NewInt(int64(val)), nil case int16: @@ -77,6 +81,9 @@ func Any(v any) (res ipld.Node, err error) { case int32: return basicnode.NewInt(int64(val)), nil case int64: + if val > limits.MaxInt53 || val < limits.MinInt53 { + return nil, fmt.Errorf("integer value %d exceeds safe integer bounds", val) + } return basicnode.NewInt(val), nil case uint: return basicnode.NewInt(int64(val)), nil @@ -87,6 +94,9 @@ func Any(v any) (res ipld.Node, err error) { case uint32: return basicnode.NewInt(int64(val)), nil case uint64: + if val > uint64(limits.MaxInt53) { + return nil, fmt.Errorf("unsigned integer value %d exceeds safe integer bounds", val) + } return basicnode.NewInt(int64(val)), nil case float32: return basicnode.NewFloat(float64(val)), nil diff --git a/pkg/policy/literal/literal_test.go b/pkg/policy/literal/literal_test.go index d2282d0..9f7e1f2 100644 --- a/pkg/policy/literal/literal_test.go +++ b/pkg/policy/literal/literal_test.go @@ -8,6 +8,7 @@ import ( cidlink "github.com/ipld/go-ipld-prime/linking/cid" "github.com/ipld/go-ipld-prime/printer" "github.com/stretchr/testify/require" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) @@ -295,14 +296,11 @@ func TestAnyAssembleIntegerOverflow(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + _, err := Any(tt.input) if tt.shouldErr { - require.Panics(t, func() { - anyAssemble(tt.input) - }) + require.Error(t, err) } else { - require.NotPanics(t, func() { - anyAssemble(tt.input) - }) + require.NoError(t, err) } }) } diff --git a/pkg/policy/match.go b/pkg/policy/match.go index 81a5431..648b877 100644 --- a/pkg/policy/match.go +++ b/pkg/policy/match.go @@ -8,8 +8,6 @@ import ( "github.com/ipld/go-ipld-prime" "github.com/ipld/go-ipld-prime/datamodel" "github.com/ipld/go-ipld-prime/must" - - "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) // Match determines if the IPLD node satisfies the policy. @@ -268,10 +266,6 @@ func isOrdered(expected ipld.Node, actual ipld.Node, satisfies func(order int) b a := must.Int(actual) b := must.Int(expected) - if a > limits.MaxInt53 || a < limits.MinInt53 || b > limits.MaxInt53 || b < limits.MinInt53 { - return false - } - return satisfies(cmp.Compare(a, b)) } diff --git a/pkg/policy/match_test.go b/pkg/policy/match_test.go index 89bf02d..108037a 100644 --- a/pkg/policy/match_test.go +++ b/pkg/policy/match_test.go @@ -11,7 +11,6 @@ import ( "github.com/ipld/go-ipld-prime/node/basicnode" "github.com/stretchr/testify/require" - "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/pkg/policy/literal" ) @@ -902,38 +901,3 @@ func TestPartialMatch(t *testing.T) { }) } } - -func TestIntegerOverflow(t *testing.T) { - tests := []struct { - name string - expected ipld.Node - actual ipld.Node - want bool - }{ - { - name: "valid integers", - expected: literal.Int(42), - actual: literal.Int(43), - want: true, // for gt comparison - }, - { - name: "exceeds MaxInt53", - expected: literal.Int(limits.MaxInt53 + 1), - actual: literal.Int(42), - want: false, - }, - { - name: "below MinInt53", - expected: literal.Int(limits.MinInt53 - 1), - actual: literal.Int(42), - want: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := isOrdered(tt.expected, tt.actual, gt) - require.Equal(t, tt.want, result) - }) - } -} diff --git a/pkg/policy/selector/selector.go b/pkg/policy/selector/selector.go index 249cd44..9c3c2ef 100644 --- a/pkg/policy/selector/selector.go +++ b/pkg/policy/selector/selector.go @@ -10,6 +10,7 @@ import ( "github.com/ipld/go-ipld-prime/datamodel" "github.com/ipld/go-ipld-prime/fluent/qp" "github.com/ipld/go-ipld-prime/node/basicnode" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) // Selector describes a UCAN policy selector, as specified here: @@ -22,6 +23,10 @@ type Selector []segment // - a resolutionerr error if not being able to resolve to a node // - nil and no errors, if the selector couldn't match on an optional segment (with ?). func (s Selector) Select(subject ipld.Node) (ipld.Node, error) { + if err := limits.ValidateIntegerBoundsIPLD(subject); err != nil { + return nil, fmt.Errorf("node contains integer values outside safe bounds: %w", err) + } + return resolve(s, subject, nil) } From 5b816ccc6211d0e04a225c8e2aca005bdfb97128 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 12:06:06 +0100 Subject: [PATCH 08/13] streamline int overflow check for token timestamps --- token/delegation/delegation.go | 32 +++++++++----------------------- token/internal/parse/parse.go | 13 ++++++++++--- token/invocation/invocation.go | 11 +++++++++-- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/token/delegation/delegation.go b/token/delegation/delegation.go index 5548477..110b8e1 100644 --- a/token/delegation/delegation.go +++ b/token/delegation/delegation.go @@ -18,7 +18,6 @@ import ( "github.com/ucan-wg/go-ucan/pkg/command" "github.com/ucan-wg/go-ucan/pkg/meta" "github.com/ucan-wg/go-ucan/pkg/policy" - "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/token/internal/nonce" "github.com/ucan-wg/go-ucan/token/internal/parse" ) @@ -177,18 +176,6 @@ func (t *Token) validate() error { errs = errors.Join(errs, fmt.Errorf("token nonce too small")) } - if t.notBefore != nil { - if err := validateTimestamp(t.notBefore.Unix(), "nbf"); err != nil { - errs = errors.Join(errs, err) - } - } - - if t.expiration != nil { - if err := validateTimestamp(t.expiration.Unix(), "exp"); err != nil { - errs = errors.Join(errs, err) - } - } - return errs } @@ -228,8 +215,15 @@ func tokenFromModel(m tokenPayloadModel) (*Token, error) { tkn.meta = m.Meta - tkn.notBefore = parse.OptionalTimestamp(m.Nbf) - tkn.expiration = parse.OptionalTimestamp(m.Exp) + tkn.notBefore, err = parse.OptionalTimestamp(m.Nbf) + if err != nil { + return nil, fmt.Errorf("parse notBefore: %w", err) + } + + tkn.expiration, err = parse.OptionalTimestamp(m.Exp) + if err != nil { + return nil, fmt.Errorf("parse expiration: %w", err) + } if err := tkn.validate(); err != nil { return nil, err @@ -237,11 +231,3 @@ func tokenFromModel(m tokenPayloadModel) (*Token, error) { return &tkn, nil } - -func validateTimestamp(ts int64, field string) error { - if ts > limits.MaxInt53 || ts < limits.MinInt53 { - return fmt.Errorf("token %s timestamp %d exceeds safe integer bounds", field, ts) - } - - return nil -} diff --git a/token/internal/parse/parse.go b/token/internal/parse/parse.go index 147b308..27af240 100644 --- a/token/internal/parse/parse.go +++ b/token/internal/parse/parse.go @@ -1,9 +1,11 @@ package parse import ( + "fmt" "time" "github.com/ucan-wg/go-ucan/did" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) func OptionalDID(s *string) (did.DID, error) { @@ -13,10 +15,15 @@ func OptionalDID(s *string) (did.DID, error) { return did.Parse(*s) } -func OptionalTimestamp(sec *int64) *time.Time { +func OptionalTimestamp(sec *int64) (*time.Time, error) { if sec == nil { - return nil + return nil, nil } + + if *sec > limits.MaxInt53 || *sec < limits.MinInt53 { + return nil, fmt.Errorf("timestamp value %d exceeds safe integer bounds", *sec) + } + t := time.Unix(*sec, 0) - return &t + return &t, nil } diff --git a/token/invocation/invocation.go b/token/invocation/invocation.go index ee85a94..286c456 100644 --- a/token/invocation/invocation.go +++ b/token/invocation/invocation.go @@ -275,8 +275,15 @@ func tokenFromModel(m tokenPayloadModel) (*Token, error) { tkn.proof = m.Prf tkn.meta = m.Meta - tkn.expiration = parse.OptionalTimestamp(m.Exp) - tkn.invokedAt = parse.OptionalTimestamp(m.Iat) + tkn.expiration, err = parse.OptionalTimestamp(m.Exp) + if err != nil { + return nil, fmt.Errorf("parse expiration: %w", err) + } + + tkn.invokedAt, err = parse.OptionalTimestamp(m.Iat) + if err != nil { + return nil, fmt.Errorf("parse invokedAt: %w", err) + } tkn.cause = m.Cause From 105323b98993ef051a30b481a64fd91a4ddb2cc6 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 12:13:56 +0100 Subject: [PATCH 09/13] moved unit test --- token/delegation/delegation_test.go | 71 ----------------------------- token/internal/parse/parse_test.go | 64 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 71 deletions(-) create mode 100644 token/internal/parse/parse_test.go diff --git a/token/delegation/delegation_test.go b/token/delegation/delegation_test.go index 7450bea..8da08b4 100644 --- a/token/delegation/delegation_test.go +++ b/token/delegation/delegation_test.go @@ -11,7 +11,6 @@ import ( "github.com/ucan-wg/go-ucan/did/didtest" "github.com/ucan-wg/go-ucan/pkg/command" "github.com/ucan-wg/go-ucan/pkg/policy" - "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/token/delegation" ) @@ -208,73 +207,3 @@ func TestEncryptedMeta(t *testing.T) { } }) } - -func TestTokenTimestampBounds(t *testing.T) { - t.Parallel() - - cmd, err := command.Parse("/foo/bar") - require.NoError(t, err) - pol, err := policy.FromDagJson("[]") - require.NoError(t, err) - - tomorrow := time.Now().Add(24 * time.Hour).Unix() - - tests := []struct { - name string - nbf int64 - exp int64 - wantErr bool - }{ - { - name: "valid timestamps", - nbf: tomorrow, - exp: tomorrow + 3600, - wantErr: false, - }, - { - name: "max safe integer", - nbf: tomorrow, - exp: limits.MaxInt53, - wantErr: false, - }, - { - name: "exceeds max safe integer", - nbf: tomorrow, - exp: limits.MaxInt53 + 1, - wantErr: true, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - _, err = delegation.New(didtest.PersonaAlice.DID(), didtest.PersonaBob.DID(), - cmd, pol, - delegation.WithNotBefore(time.Unix(tt.nbf, 0)), - delegation.WithExpiration(time.Unix(tt.exp, 0)), - ) - - if tt.wantErr { - require.Error(t, err) - require.Contains(t, err.Error(), "exceeds safe integer bounds") - } else { - require.NoError(t, err) - } - }) - } - - t.Run("nbf overflow", func(t *testing.T) { - t.Parallel() - - futureExp := time.Now().Add(48 * time.Hour).Unix() - _, err := delegation.New(didtest.PersonaAlice.DID(), didtest.PersonaBob.DID(), - cmd, pol, - delegation.WithNotBefore(time.Unix(limits.MaxInt53+1, 0)), - delegation.WithExpiration(time.Unix(futureExp, 0)), - ) - require.Error(t, err) - require.Contains(t, err.Error(), "exceeds safe integer bounds") - }) -} diff --git a/token/internal/parse/parse_test.go b/token/internal/parse/parse_test.go new file mode 100644 index 0000000..9db6474 --- /dev/null +++ b/token/internal/parse/parse_test.go @@ -0,0 +1,64 @@ +package parse + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" +) + +func TestOptionalTimestamp(t *testing.T) { + tests := []struct { + name string + input *int64 + wantErr bool + }{ + { + name: "nil timestamp", + input: nil, + wantErr: false, + }, + { + name: "valid timestamp", + input: int64Ptr(1625097600), + wantErr: false, + }, + { + name: "max safe integer", + input: int64Ptr(limits.MaxInt53), + wantErr: false, + }, + { + name: "exceeds max safe integer", + input: int64Ptr(limits.MaxInt53 + 1), + wantErr: true, + }, + { + name: "below min safe integer", + input: int64Ptr(limits.MinInt53 - 1), + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := OptionalTimestamp(tt.input) + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), "exceeds safe integer bounds") + require.Nil(t, result) + } else { + require.NoError(t, err) + if tt.input == nil { + require.Nil(t, result) + } else { + require.NotNil(t, result) + } + } + }) + } +} + +func int64Ptr(i int64) *int64 { + return &i +} From 56eab758ed8b2b21dc902babf11c635d01fadd3b Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 12:24:06 +0100 Subject: [PATCH 10/13] move args int validation to their creation --- pkg/args/args.go | 5 +++ pkg/args/args_test.go | 66 +++++++++++++++++++++++++++++++++ pkg/policy/selector/selector.go | 5 --- 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/pkg/args/args.go b/pkg/args/args.go index 3cee3ce..8fdb48c 100644 --- a/pkg/args/args.go +++ b/pkg/args/args.go @@ -16,6 +16,7 @@ import ( "github.com/ipld/go-ipld-prime/node/basicnode" "github.com/ipld/go-ipld-prime/printer" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/pkg/policy/literal" ) @@ -62,6 +63,10 @@ func (a *Args) Add(key string, val any) error { return err } + if err := limits.ValidateIntegerBoundsIPLD(node); err != nil { + return fmt.Errorf("value for key %q: %w", key, err) + } + a.Values[key] = node a.Keys = append(a.Keys, key) diff --git a/pkg/args/args_test.go b/pkg/args/args_test.go index 2a44d0f..938151f 100644 --- a/pkg/args/args_test.go +++ b/pkg/args/args_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ucan-wg/go-ucan/pkg/args" + "github.com/ucan-wg/go-ucan/pkg/policy/limits" "github.com/ucan-wg/go-ucan/pkg/policy/literal" ) @@ -185,6 +186,71 @@ func TestInclude(t *testing.T) { }, maps.Collect(a1.Iter())) } +func TestArgsIntegerBounds(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + key string + val int64 + wantErr string + }{ + { + name: "valid int", + key: "valid", + val: 42, + }, + { + name: "max safe integer", + key: "max", + val: limits.MaxInt53, + }, + { + name: "min safe integer", + key: "min", + val: limits.MinInt53, + }, + { + name: "exceeds max safe integer", + key: "tooBig", + val: limits.MaxInt53 + 1, + wantErr: "exceeds safe integer bounds", + }, + { + name: "below min safe integer", + key: "tooSmall", + val: limits.MinInt53 - 1, + wantErr: "exceeds safe integer bounds", + }, + { + name: "duplicate key", + key: "duplicate", + val: 42, + wantErr: "duplicate key", + }, + } + + a := args.New() + require.NoError(t, a.Add("duplicate", 1)) // tests duplicate key + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := a.Add(tt.key, tt.val) + if tt.wantErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tt.wantErr) + } else { + require.NoError(t, err) + val, err := a.GetNode(tt.key) + require.NoError(t, err) + i, err := val.AsInt() + require.NoError(t, err) + require.Equal(t, tt.val, i) + } + }) + } +} + const ( argsSchema = "type Args { String : Any }" argsName = "Args" diff --git a/pkg/policy/selector/selector.go b/pkg/policy/selector/selector.go index 9c3c2ef..249cd44 100644 --- a/pkg/policy/selector/selector.go +++ b/pkg/policy/selector/selector.go @@ -10,7 +10,6 @@ import ( "github.com/ipld/go-ipld-prime/datamodel" "github.com/ipld/go-ipld-prime/fluent/qp" "github.com/ipld/go-ipld-prime/node/basicnode" - "github.com/ucan-wg/go-ucan/pkg/policy/limits" ) // Selector describes a UCAN policy selector, as specified here: @@ -23,10 +22,6 @@ type Selector []segment // - a resolutionerr error if not being able to resolve to a node // - nil and no errors, if the selector couldn't match on an optional segment (with ?). func (s Selector) Select(subject ipld.Node) (ipld.Node, error) { - if err := limits.ValidateIntegerBoundsIPLD(subject); err != nil { - return nil, fmt.Errorf("node contains integer values outside safe bounds: %w", err) - } - return resolve(s, subject, nil) } From 311b942a6d537c306dfc90881d50ef0d6b1ba8f9 Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 14:22:42 +0100 Subject: [PATCH 11/13] validate invocation token args --- pkg/args/args.go | 11 +++++++++++ token/invocation/invocation.go | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/pkg/args/args.go b/pkg/args/args.go index 8fdb48c..a840c6e 100644 --- a/pkg/args/args.go +++ b/pkg/args/args.go @@ -169,3 +169,14 @@ func (a *Args) Clone() *Args { } return res } + +// Validate checks that all values in the Args are valid according to UCAN specs +func (a *Args) Validate() error { + for key, value := range a.Values { + if err := limits.ValidateIntegerBoundsIPLD(value); err != nil { + return fmt.Errorf("value for key %q: %w", key, err) + } + } + + return nil +} diff --git a/token/invocation/invocation.go b/token/invocation/invocation.go index 286c456..4ab7b8b 100644 --- a/token/invocation/invocation.go +++ b/token/invocation/invocation.go @@ -272,6 +272,10 @@ func tokenFromModel(m tokenPayloadModel) (*Token, error) { tkn.nonce = m.Nonce tkn.arguments = m.Args + if err := tkn.arguments.Validate(); err != nil { + return nil, fmt.Errorf("invalid arguments: %w", err) + } + tkn.proof = m.Prf tkn.meta = m.Meta From da806b1bc5a0276d910376752595151e12fb277f Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 14:32:15 +0100 Subject: [PATCH 12/13] remove TODO comment --- pkg/policy/literal/literal.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/policy/literal/literal.go b/pkg/policy/literal/literal.go index fcb03cb..333ade3 100644 --- a/pkg/policy/literal/literal.go +++ b/pkg/policy/literal/literal.go @@ -60,8 +60,6 @@ func List[T any](l []T) (ipld.Node, error) { // Any creates an IPLD node from any value // If possible, use another dedicated function for your type for performance. func Any(v any) (res ipld.Node, err error) { - // TODO: handle uint overflow below - // some fast path switch val := v.(type) { case bool: From 64d3024dec1693833255182332461394bb973b4a Mon Sep 17 00:00:00 2001 From: Fabio Bozzo Date: Mon, 2 Dec 2024 17:31:19 +0100 Subject: [PATCH 13/13] remove aes comparison and add ciphertext overhead comments --- pkg/meta/internal/crypto/secretbox.go | 3 +- pkg/meta/internal/crypto/secretbox_test.go | 149 --------------------- pkg/meta/meta.go | 1 + token/delegation/options.go | 6 +- 4 files changed, 7 insertions(+), 152 deletions(-) diff --git a/pkg/meta/internal/crypto/secretbox.go b/pkg/meta/internal/crypto/secretbox.go index 70d223c..690be7e 100644 --- a/pkg/meta/internal/crypto/secretbox.go +++ b/pkg/meta/internal/crypto/secretbox.go @@ -25,7 +25,8 @@ func GenerateKey() ([]byte, error) { return key, nil } -// EncryptWithKey encrypts data using secretbox with the provided key +// EncryptWithKey encrypts data using NaCl's secretbox with the provided key. +// 40 bytes of overhead (24-byte nonce + 16-byte MAC) are added to the plaintext size. func EncryptWithKey(data, key []byte) ([]byte, error) { if err := validateKey(key); err != nil { return nil, err diff --git a/pkg/meta/internal/crypto/secretbox_test.go b/pkg/meta/internal/crypto/secretbox_test.go index 09d3831..d87f860 100644 --- a/pkg/meta/internal/crypto/secretbox_test.go +++ b/pkg/meta/internal/crypto/secretbox_test.go @@ -2,12 +2,7 @@ package crypto import ( "bytes" - "crypto/aes" - "crypto/cipher" "crypto/rand" - "errors" - "fmt" - "io" "testing" "github.com/stretchr/testify/require" @@ -147,147 +142,3 @@ func tamperWithBytes(data []byte) []byte { tampered[24] ^= 0x01 // Modify first byte after nonce return tampered } - -func encryptWithAESKey(data, key []byte) ([]byte, error) { - if err := validateKey(key); err != nil { - return nil, err - } - - block, err := aes.NewCipher(key) - if err != nil { - return nil, err - } - - aesGCM, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - - nonce := make([]byte, aesGCM.NonceSize()) - if _, err := io.ReadFull(rand.Reader, nonce); err != nil { - return nil, err - } - - return aesGCM.Seal(nonce, nonce, data, nil), nil -} - -func decryptWithAESKey(data, key []byte) ([]byte, error) { - if err := validateKey(key); err != nil { - return nil, err - } - - block, err := aes.NewCipher(key) - if err != nil { - return nil, err - } - - aesGCM, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - - nonceSize := aesGCM.NonceSize() - if len(data) < nonceSize { - return nil, ErrShortCipherText - } - - nonce, ciphertext := data[:nonceSize], data[nonceSize:] - plaintext, err := aesGCM.Open(nil, nonce, ciphertext, nil) - if err != nil { - return nil, errors.New("decryption failed") - } - - return plaintext, nil -} - -func BenchmarkEncryption(b *testing.B) { - key := make([]byte, keySize) - _, err := rand.Read(key) - require.NoError(b, err) - - sizes := []int{16, 64, 256, 1024, 4096} // Test different payload sizes - for _, size := range sizes { - data := make([]byte, size) - _, err := rand.Read(data) - require.NoError(b, err) - - b.Run(fmt.Sprintf("Secretbox-%dB", size), func(b *testing.B) { - for i := 0; i < b.N; i++ { - encrypted, err := EncryptWithKey(data, key) - require.NoError(b, err) - b.SetBytes(int64(len(encrypted))) - } - }) - - b.Run(fmt.Sprintf("AES-GCM-%dB", size), func(b *testing.B) { - for i := 0; i < b.N; i++ { - encrypted, err := encryptWithAESKey(data, key) - require.NoError(b, err) - b.SetBytes(int64(len(encrypted))) - } - }) - } -} - -func BenchmarkDecryption(b *testing.B) { - key := make([]byte, keySize) - _, err := rand.Read(key) - require.NoError(b, err) - - sizes := []int{16, 64, 256, 1024, 4096} - for _, size := range sizes { - data := make([]byte, size) - _, err := rand.Read(data) - require.NoError(b, err) - - secretboxEncrypted, err := EncryptWithKey(data, key) - require.NoError(b, err) - - aesGCMEncrypted, err := encryptWithAESKey(data, key) - require.NoError(b, err) - - b.Run(fmt.Sprintf("Secretbox-%dB", size), func(b *testing.B) { - for i := 0; i < b.N; i++ { - decrypted, err := DecryptStringWithKey(secretboxEncrypted, key) - require.NoError(b, err) - b.SetBytes(int64(len(decrypted))) - } - }) - - b.Run(fmt.Sprintf("AES-GCM-%dB", size), func(b *testing.B) { - for i := 0; i < b.N; i++ { - decrypted, err := decryptWithAESKey(aesGCMEncrypted, key) - require.NoError(b, err) - b.SetBytes(int64(len(decrypted))) - } - }) - } -} - -// TestCiphertextSizeComparison shows that Secretbox encryption entails -// a slightly larger ciphertext overhead of 40 bytes, compared to AES-GCM, -// whose overhead is just 28 bytes. -func TestCiphertextSizeComparison(t *testing.T) { - key := make([]byte, keySize) - _, err := rand.Read(key) - require.NoError(t, err) - - sizes := []int{0, 16, 64, 256, 1024, 4096} - for _, size := range sizes { - t.Run(fmt.Sprintf("size-%d", size), func(t *testing.T) { - data := make([]byte, size) - _, err := rand.Read(data) - require.NoError(t, err) - - sbCiphertext, err := EncryptWithKey(data, key) - require.NoError(t, err) - - aesCiphertext, err := encryptWithAESKey(data, key) - require.NoError(t, err) - - t.Logf("Input size: %d bytes", size) - t.Logf("Secretbox size: %d bytes (overhead: %d bytes)", len(sbCiphertext), len(sbCiphertext)-size) - t.Logf("AES-GCM size: %d bytes (overhead: %d bytes)", len(aesCiphertext), len(aesCiphertext)-size) - }) - } -} diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 21d56a6..9b0e79f 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -150,6 +150,7 @@ func (m *Meta) Add(key string, val any) error { // AddEncrypted adds a key/value pair in the meta set. // The value is encrypted with the given encryptionKey. // Accepted types for the value are: string, []byte. +// The ciphertext will be 40 bytes larger than the plaintext due to encryption overhead. func (m *Meta) AddEncrypted(key string, val any, encryptionKey []byte) error { var encrypted []byte var err error diff --git a/token/delegation/options.go b/token/delegation/options.go index 4df14e7..3348760 100644 --- a/token/delegation/options.go +++ b/token/delegation/options.go @@ -45,7 +45,8 @@ func WithMeta(key string, val any) Option { } // WithEncryptedMetaString adds a key/value pair in the "meta" field. -// The string value is encrypted with the given aesKey. +// The string value is encrypted with the given key. +// The ciphertext will be 40 bytes larger than the plaintext due to encryption overhead. func WithEncryptedMetaString(key, val string, encryptionKey []byte) Option { return func(t *Token) error { return t.meta.AddEncrypted(key, val, encryptionKey) @@ -53,7 +54,8 @@ func WithEncryptedMetaString(key, val string, encryptionKey []byte) Option { } // WithEncryptedMetaBytes adds a key/value pair in the "meta" field. -// The []byte value is encrypted with the given aesKey. +// The []byte value is encrypted with the given key. +// The ciphertext will be 40 bytes larger than the plaintext due to encryption overhead. func WithEncryptedMetaBytes(key string, val, encryptionKey []byte) Option { return func(t *Token) error { return t.meta.AddEncrypted(key, val, encryptionKey)