add size enforcement and tests (#37)

* add size enforcement and tests

boxer.go

@@ -47,8 +47,6 @@ type boxMaker struct {
 	counterparty *rsa.PublicKey
 }
 
-const maxBoxSize = 4 * 1024 * 1024
-
 func NewBoxer(key *rsa.PrivateKey, counterparty *rsa.PublicKey) boxMaker {
 	return boxMaker{
 		key:          key,
@@ -172,6 +170,10 @@ func (boxer boxMaker) DecodeUnverified(b64 string) (*Box, error) {
 		return nil, fmt.Errorf("decoding base64: %w", err)
 	}
 
+	if len(data) > V0MaxSize {
+		return nil, fmt.Errorf("data too big, is %d, max is %d", len(data), V0MaxSize)
+	}
+
 	return boxer.DecodeRawUnverified(data)
 }
 
@@ -199,14 +201,18 @@ func (boxer boxMaker) DecodePngUnverified(r io.Reader) (*Box, error) {
 		return nil, fmt.Errorf("decoding png: %w", err)
 	}
 
-	if data.Len() > maxBoxSize {
+	if data.Len() > V0MaxSize {
 		return nil, errors.New("looks to be larger than max box size")
 	}
 
 	return boxer.DecodeRawUnverified(data.Bytes())
 }
 
 func (boxer boxMaker) DecodeRaw(data []byte) (*Box, error) {
+	if len(data) > V0MaxSize {
+		return nil, fmt.Errorf("data too big, is %d, max is %d", len(data), V0MaxSize)
+	}
+
 	var outer outerBox
 	if err := msgpack.Unmarshal(data, &outer); err != nil {
 		return nil, fmt.Errorf("unmarshalling outer: %w", err)

cross_language_tests/boxer_cross_test.go

@@ -304,3 +304,89 @@ func TestBoxerRuby(t *testing.T) {
 		})
 	}
 }
+
+func TestBoxerMaxSize(t *testing.T) {
+	t.Parallel()
+
+	//
+	// Setup keys and similar.
+	//
+	aliceKey, err := krypto.RsaRandomKey()
+	require.NoError(t, err)
+	var alicePubPem bytes.Buffer
+	require.NoError(t, krypto.RsaPublicKeyToPem(aliceKey, &alicePubPem))
+
+	bobKey, err := krypto.RsaRandomKey()
+	require.NoError(t, err)
+	var bobPem bytes.Buffer
+	require.NoError(t, krypto.RsaPrivateKeyToPem(bobKey, &bobPem))
+
+	malloryKey, err := krypto.RsaRandomKey()
+	require.NoError(t, err)
+	var malloryPem bytes.Buffer
+	require.NoError(t, krypto.RsaPrivateKeyToPem(malloryKey, &malloryPem))
+
+	aliceBoxer := krypto.NewBoxer(aliceKey, bobKey.Public().(*rsa.PublicKey))
+
+	tooBigBytes := mkrand(t, krypto.V0MaxSize+1)
+	tooBigBytesB64 := base64.StdEncoding.EncodeToString(tooBigBytes)
+
+	t.Run("max size enforced in go", func(t *testing.T) {
+		t.Parallel()
+
+		_, err = aliceBoxer.Decode(tooBigBytesB64)
+		require.ErrorContains(t, err, "data too big")
+
+		_, err = aliceBoxer.DecodeUnverified(tooBigBytesB64)
+		require.ErrorContains(t, err, "data too big")
+	})
+
+	t.Run("max size enforced in ruby", func(t *testing.T) {
+		t.Parallel()
+		dir := t.TempDir()
+
+		pngFile := path.Join(dir, ulid.New()+".png")
+		//#nosec G306 -- Need readable files
+		require.NoError(t, os.WriteFile(pngFile, []byte(tooBigBytesB64), 0644))
+
+		tests := []boxerCrossTestCase{
+			{Key: bobPem.Bytes(), Counterparty: alicePubPem.Bytes(), Ciphertext: tooBigBytesB64, cmd: "decode"},
+			{Key: bobPem.Bytes(), Counterparty: alicePubPem.Bytes(), Ciphertext: tooBigBytesB64, cmd: "decodeunverified"},
+			{Key: bobPem.Bytes(), Ciphertext: tooBigBytesB64, cmd: "decodeunverified"},
+			{Key: bobPem.Bytes(), Counterparty: alicePubPem.Bytes(), PngFile: pngFile, cmd: "decodepng"},
+		}
+
+		for _, tt := range tests {
+			tt := tt
+
+			t.Run("", func(t *testing.T) {
+				t.Parallel()
+
+				if runtime.GOOS == "windows" && tt.cmd == "decodepng" {
+					t.Skip("skip png decode test on windows because ruby library chunky_png is looking for CRLF png signature")
+				}
+
+				testfile := path.Join(dir, ulid.New()+".msgpack")
+				rubyout := path.Join(dir, ulid.New()+"ruby-out")
+
+				//
+				// Setup
+				//
+				b, err := msgpack.Marshal(tt)
+				require.NoError(t, err)
+				//#nosec G306 -- Need readable files
+				require.NoError(t, os.WriteFile(testfile, []byte(base64.StdEncoding.EncodeToString(b)), 0644))
+
+				ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+				defer cancel()
+
+				//#nosec G204 -- No taint on hardcoded input
+				cmd := exec.CommandContext(ctx, "ruby", boxerRB, tt.cmd, testfile, rubyout)
+				out, err := cmd.CombinedOutput()
+
+				require.Error(t, err)
+				require.Contains(t, string(out), "box too large", "actual out: ", string(out))
+			})
+		}
+	})
+}

cross_language_tests/challenge_cross_test.go

@@ -225,6 +225,40 @@ func TestChallenge_GoGenerate_RubyRespond(t *testing.T) {
 	}
 }
 
+func TestChallenge_MaxSize(t *testing.T) {
+	t.Parallel()
+
+	tooBigBytes := mkrand(t, krypto.V0MaxSize+1)
+
+	t.Run("max size enforced in go", func(t *testing.T) {
+		t.Parallel()
+		_, err := challenge.UnmarshalChallenge(tooBigBytes)
+		require.ErrorContains(t, err, "exceeds max size", "should get an error due to size")
+	})
+
+	t.Run("max size enforced in ruby", func(t *testing.T) {
+		t.Parallel()
+
+		dir := t.TempDir()
+		rubyPrivateSigningKey := ecdsaKey(t)
+
+		rubyChallengeCmdData := rubyChallengeCmd{
+			RubyPrivateSigningKey: privateEcKeyToPem(t, rubyPrivateSigningKey),
+		}
+
+		out, err := rubyChallengeExec("generate", dir, rubyChallengeCmdData)
+		require.NoError(t, err, string(out))
+
+		rubyChallengeCmdData = rubyChallengeCmd{
+			ResponsePack: tooBigBytes,
+		}
+
+		out, err = rubyChallengeExec("open_response_png", dir, rubyChallengeCmdData)
+		require.Error(t, err, string(out))
+		require.Contains(t, string(out), "response too large", "should get an error due to size")
+	})
+}
+
 func rubyChallengeExec(rubyCmd, dir string, inputData rubyChallengeCmd) ([]byte, error) {
 	testCaseBytes, err := msgpack.Marshal(inputData)
 	if err != nil {

lib/krypto/boxer.rb

@@ -82,6 +82,11 @@ def decode_unverified(data)
     end
 
     def decode(data, verify: true, raw: false, png: false)
+      # Limit size to prevent garbage from filling memory
+      if data.size > MAX_CHALLENGE_SIZE
+        raise "box too large"
+      end
+
       data = unpng(data) if png
       data = Base64.strict_decode64(data) unless raw || png
       outer = Outer.new(MessagePack.unpack(data).slice(*OUTER_FIELDS.map(&:to_s)))

lib/krypto/challenge.rb

@@ -5,6 +5,9 @@
 require "openssl"
 
 module Krypto
+  # Limit size to prevent garbage from filling memory
+  MAX_CHALLENGE_SIZE = 4 * 1024 * 1024
+
   class Challenge
     def self.generate(signing_key, challenge_id, challenge_data, request_data, timestamp: Time.now)
       private_encryption_key = RbNaCl::PrivateKey.generate
@@ -29,6 +32,11 @@ def self.generate(signing_key, challenge_id, challenge_data, request_data, times
     end
 
     def self.unmarshal(data, png: false, base64: true)
+      # Limit size to prevent garbage from filling memory
+      if data.size > MAX_CHALLENGE_SIZE
+        raise "challenge too large"
+      end
+
       data = ::Krypto::Png.decode_blob(data) if png
       data = Base64.strict_decode64(data) if base64
       OuterChallenge.new(MessagePack.unpack(data).slice(*OUTER_CHALLENGE_FIELDS.map(&:to_s)))

lib/krypto/challenge_response.rb

@@ -7,6 +7,10 @@
 module Krypto
   class ChallengeResponse
     def self.unmarshal(data, png: false, base64: true)
+      if data.size > MAX_CHALLENGE_SIZE
+        raise "response too large"
+      end
+
       data = ::Krypto::Png.decode_blob(data) if png
       data = Base64.strict_decode64(data) if base64
 

pkg/challenge/challenge.go

@@ -144,6 +144,10 @@ func (o *OuterChallenge) RespondPng(signer crypto.Signer, signer2 crypto.Signer,
 }
 
 func UnmarshalChallenge(outerChallengeBytes []byte) (*OuterChallenge, error) {
+	if len(outerChallengeBytes) > krypto.V0MaxSize {
+		return nil, fmt.Errorf("challenge exceeds max size: %d, max is %d", len(outerChallengeBytes), krypto.V0MaxSize)
+	}
+
 	var outerChallenge OuterChallenge
 	if err := msgpack.Unmarshal(outerChallengeBytes, &outerChallenge); err != nil {
 		return nil, err

pkg/challenge/response.go

@@ -84,6 +84,10 @@ type InnerResponse struct {
 }
 
 func UnmarshalResponse(outerResponseBytes []byte) (*OuterResponse, error) {
+	if len(outerResponseBytes) > krypto.V0MaxSize {
+		return nil, fmt.Errorf("response to large: is %d, max is %d", len(outerResponseBytes), krypto.V0MaxSize)
+	}
+
 	var outerResponse OuterResponse
 	if err := msgpack.Unmarshal(outerResponseBytes, &outerResponse); err != nil {
 		return nil, err

pkg/secureenclave/secureenclave.go

@@ -133,6 +133,9 @@ func findKey(publicKeySha1 []byte) (*ecdsa.PublicKey, error) {
 func rawToEcdsa(raw []byte) *ecdsa.PublicKey {
 	ecKey := new(ecdsa.PublicKey)
 	ecKey.Curve = elliptic.P256()
+	// lint here suggestest using ecdh package, but we are using ecdsa key through out the code
+	// have found a straight forward to go from ecdh.P256().NewPublicKey(raw) -> ecdsa.PublicKey
+	//nolint:staticcheck
 	ecKey.X, ecKey.Y = elliptic.Unmarshal(ecKey.Curve, raw)
 	return ecKey
 }
@@ -142,6 +145,9 @@ func publicKeyLookUpHash(key *ecdsa.PublicKey) ([]byte, error) {
 		return nil, errors.New("public key has nil XY coordinates")
 	}
 
+	// lint here suggestest using ecdh package, but we are using ecdsa key through out the code
+	// have found a straight forward to go from ecdh.P256().NewPublicKey(raw) -> ecdsa.PublicKey
+	//nolint:staticcheck
 	keyBytes := elliptic.Marshal(elliptic.P256(), key.X, key.Y)
 	hash := sha1.New()
 	hash.Write(keyBytes)

png.go

@@ -16,14 +16,14 @@ const (
 	pixelsInHeader      = 2
 	alphaValue          = 0xFF
 
-	v0MaxSize = 1 << 24
+	// Limit size to prevent garbage from filling memory
+	V0MaxSize = 4 * 1024 * 1024
 )
 
 func ToPng(w io.Writer, data []byte) error {
 	dataSize := len(data)
-
-	if dataSize > v0MaxSize {
-		return fmt.Errorf("data too big: %d is bigger than %d", dataSize, v0MaxSize)
+	if dataSize > V0MaxSize {
+		return fmt.Errorf("data too big: %d is bigger than %d", dataSize, V0MaxSize)
 	}
 
 	pixelCount := divCeil(len(data), usableBytesPerPixel)