add size enforcement and tests

.golangci.yml

@@ -1,6 +1,4 @@
 run:
-  skip-dirs:
-    - test-cmds
   timeout: 5m
 
 linters:
@@ -36,4 +34,4 @@ issues:
     # False positive: https://github.com/kunwardeep/paralleltest/issues/8.
     - linters:
       - paralleltest
-      text: "does not use range value in test Run"
+      text: "does not use range value in test Run"

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/aes_cross_test.go

@@ -40,7 +40,6 @@ func TestAesRuby(t *testing.T) {
 		{AuthData: mkrand(t, 32), Plaintext: mkrand(t, 1024)},
 	}
 
-	//#nosec G306 -- Need readable files
 	for _, tt := range tests {
 		tt := tt
 		t.Run("", func(t *testing.T) {
@@ -60,6 +59,8 @@ func TestAesRuby(t *testing.T) {
 
 				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))
 			})
 

cross_language_tests/boxer_cross_test.go

@@ -70,7 +70,6 @@ func TestBoxerRuby(t *testing.T) {
 	}
 
 	// Ruby Decrypt Tests
-	//#nosec G306 -- Need readable files
 	for _, message := range testMessages {
 		message := message
 
@@ -93,6 +92,7 @@ func TestBoxerRuby(t *testing.T) {
 
 				b, err := msgpack.Marshal(rubyCommand)
 				require.NoError(t, err)
+				//#nosec G306 -- Need readable files
 				require.NoError(t, os.WriteFile(rubyInFile, []byte(base64.StdEncoding.EncodeToString(b)), 0644))
 
 				ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
@@ -160,6 +160,7 @@ func TestBoxerRuby(t *testing.T) {
 			var png bytes.Buffer
 			pngFile := path.Join(dir, ulid.New()+".png")
 			require.NoError(t, aliceBoxer.EncodePng(responseTo, message, &png))
+			//#nosec G306 -- Need readable files
 			require.NoError(t, os.WriteFile(pngFile, png.Bytes(), 0644))
 
 			tests := []boxerCrossTestCase{
@@ -200,6 +201,7 @@ func TestBoxerRuby(t *testing.T) {
 					//
 					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)
@@ -248,6 +250,7 @@ func TestBoxerRuby(t *testing.T) {
 
 				b, err := msgpack.Marshal(rubyCommand)
 				require.NoError(t, err)
+				//#nosec G306 -- Need readable files
 				require.NoError(t, os.WriteFile(rubyInFile, []byte(base64.StdEncoding.EncodeToString(b)), 0644))
 
 				ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
@@ -301,3 +304,95 @@ 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()
+
+		responseTo := ulid.New()
+		ciphertext, err := aliceBoxer.Encode(responseTo, tooBigBytes)
+		require.NoError(t, err)
+
+		var png bytes.Buffer
+		pngFile := path.Join(dir, ulid.New()+".png")
+		require.NoError(t, krypto.ToPngNoMaxSize(&png, tooBigBytes))
+		//#nosec G306 -- Need readable files
+		require.NoError(t, os.WriteFile(pngFile, png.Bytes(), 0644))
+
+		tests := []boxerCrossTestCase{
+			{Key: bobPem.Bytes(), Counterparty: alicePubPem.Bytes(), Ciphertext: ciphertext, cmd: "decode"},
+			{Key: bobPem.Bytes(), Counterparty: alicePubPem.Bytes(), Ciphertext: ciphertext, cmd: "decodeunverified"},
+			{Key: bobPem.Bytes(), Ciphertext: ciphertext, 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,7 +225,40 @@ func TestChallenge_GoGenerate_RubyRespond(t *testing.T) {
 	}
 }
 
-// #nosec G306 -- Need readable files
+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 {
@@ -236,7 +269,9 @@ func rubyChallengeExec(rubyCmd, dir string, inputData rubyChallengeCmd) ([]byte,
 
 	inFilePath := filepath.Join(dir, "in")
 
-	if err := os.WriteFile(inFilePath, testCaseBytesBase64, 0644); err != nil {
+	//#nosec G306 -- Need readable files
+	err = os.WriteFile(inFilePath, testCaseBytesBase64, 0644)
+	if err != nil {
 		return nil, err
 	}
 

cross_language_tests/rsa_cross_test.go

@@ -37,7 +37,6 @@ func TestRsaRuby(t *testing.T) {
 		{Plaintext: mkrand(t, 128)},
 	}
 
-	//#nosec G306 -- Need readable files
 	for _, tt := range tests {
 		tt := tt
 		t.Run("", func(t *testing.T) {
@@ -72,6 +71,7 @@ func TestRsaRuby(t *testing.T) {
 
 				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))
 
 				cmd := exec.CommandContext(ctx, "ruby", rsaRB, "decrypt", testfile, path.Join(dir, "ruby-decrypt"))
@@ -98,6 +98,8 @@ func TestRsaRuby(t *testing.T) {
 
 				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))
 
 				cmd := exec.CommandContext(ctx, "ruby", rsaRB, "encrypt", testfile, path.Join(dir, "ruby-encrypt"))
@@ -130,6 +132,8 @@ func TestRsaRuby(t *testing.T) {
 
 				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))
 
 				cmd := exec.CommandContext(ctx, "ruby", rsaRB, "verify", testfile, path.Join(dir, "ruby-verify"))
@@ -157,6 +161,8 @@ func TestRsaRuby(t *testing.T) {
 
 				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))
 
 				cmd := exec.CommandContext(ctx, "ruby", rsaRB, "sign", testfile, path.Join(dir, "ruby-signed"))

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)