Increase checks for invalid HashingCosts

Signed-off-by: Joe Richey <joerichey@google.com>
[ebiggers: moved the new checks from PassphraseHash to CheckValidity]
Signed-off-by: Eric Biggers <ebiggers@google.com>

2 files changed, 76 insertions, 13 deletions

diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go
index 10b3d17..f98c643 100644
--- a/crypto/crypto_test.go
+++ b/crypto/crypto_test.go
@@ -76,6 +76,12 @@ var hashTestCases = []hashTestCase{
 		costs:   &metadata.HashingCosts{Time: 1, Memory: 1 << 10, Parallelism: 1},
 		hexHash: "a66f5398e33761bf161fdf1273e99b148f07d88d12d85b7673fddd723f95ec34",
 	},
+	// Make sure we maintain our backwards compatible behavior, where
+	// Parallelism is truncated to 8-bits unless TruncationFixed is true.
+	{
+		costs:   &metadata.HashingCosts{Time: 1, Memory: 1 << 10, Parallelism: 257},
+		hexHash: "a66f5398e33761bf161fdf1273e99b148f07d88d12d85b7673fddd723f95ec34",
+	},
 	{
 		costs:   &metadata.HashingCosts{Time: 10, Memory: 1 << 10, Parallelism: 1},
 		hexHash: "5fa2cb89db1f7413ba1776258b7c8ee8c377d122078d28fe1fd645c353787f50",
@@ -88,6 +94,15 @@ var hashTestCases = []hashTestCase{
 		costs:   &metadata.HashingCosts{Time: 1, Memory: 1 << 10, Parallelism: 10},
 		hexHash: "b7c3d7a0be222680b5ea3af3fb1a0b7b02b92cbd7007821dc8b84800c86c7783",
 	},
+	{
+		costs:   &metadata.HashingCosts{Time: 1, Memory: 1 << 11, Parallelism: 255},
+		hexHash: "d51af3775bbdd0cba31d96fd6d921d9de27f521ceffe667618cd7624f6643071",
+	},
+	// Adding TruncationFixed shouldn't matter if Parallelism < 256.
+	{
+		costs:   &metadata.HashingCosts{Time: 1, Memory: 1 << 11, Parallelism: 255, TruncationFixed: true},
+		hexHash: "d51af3775bbdd0cba31d96fd6d921d9de27f521ceffe667618cd7624f6643071",
+	},
 }
 
 // Checks that len(array) == expected
@@ -493,16 +508,21 @@ func TestComputeKeyDescriptorBadVersion(t *testing.T) {
 
 // Run our test cases for passphrase hashing
 func TestPassphraseHashing(t *testing.T) {
+	pk, err := fakePassphraseKey()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer pk.Wipe()
+
 	for i, testCase := range hashTestCases {
-		pk, err := fakePassphraseKey()
-		if err != nil {
-			t.Fatal(err)
+		if err := testCase.costs.CheckValidity(); err != nil {
+			t.Errorf("Hash test %d: for costs=%+v hashing failed: %v", i, testCase.costs, err)
+			continue
 		}
-		defer pk.Wipe()
-
 		hash, err := PassphraseHash(pk, fakeSalt, testCase.costs)
 		if err != nil {
-			t.Fatal(err)
+			t.Errorf("Hash test %d: for costs=%+v hashing failed: %v", i, testCase.costs, err)
+			continue
 		}
 		defer hash.Wipe()
 
@@ -514,6 +534,29 @@ func TestPassphraseHashing(t *testing.T) {
 	}
 }
 
+var badCosts = []*metadata.HashingCosts{
+	// Bad Time costs
+	{Time: 0, Memory: 1 << 11, Parallelism: 1},
+	{Time: 1 << 33, Memory: 1 << 11, Parallelism: 1},
+	// Bad Memory costs
+	{Time: 1, Memory: 5, Parallelism: 1},
+	{Time: 1, Memory: 1 << 33, Parallelism: 1},
+	// Bad Parallelism costs
+	{Time: 1, Memory: 1 << 11, Parallelism: 0, TruncationFixed: false},
+	{Time: 1, Memory: 1 << 11, Parallelism: 0, TruncationFixed: true},
+	{Time: 1, Memory: 1 << 11, Parallelism: 256, TruncationFixed: false},
+	{Time: 1, Memory: 1 << 11, Parallelism: 256, TruncationFixed: true},
+	{Time: 1, Memory: 1 << 11, Parallelism: 257, TruncationFixed: true},
+}
+
+func TestBadParameters(t *testing.T) {
+	for i, costs := range badCosts {
+		if costs.CheckValidity() == nil {
+			t.Errorf("Hash test %d: expected error for costs=%+v", i, costs)
+		}
+	}
+}
+
 func BenchmarkWrap(b *testing.B) {
 	for n := 0; n < b.N; n++ {
 		Wrap(fakeWrappingKey, fakeValidPolicyKey)
diff --git a/metadata/checks.go b/metadata/checks.go
index bddc8a7..d7dea41 100644
--- a/metadata/checks.go
+++ b/metadata/checks.go
@@ -20,6 +20,9 @@
 package metadata
 
 import (
+	"log"
+	"math"
+
 	"github.com/pkg/errors"
 	"google.golang.org/protobuf/proto"
 
@@ -57,20 +60,37 @@ func (s SourceType) CheckValidity() error {
 	return nil
 }
 
+// MaxParallelism is the maximum allowed value for HashingCosts.Parallelism.
+const MaxParallelism = math.MaxUint8
+
 // CheckValidity ensures the hash costs will be accepted by Argon2.
 func (h *HashingCosts) CheckValidity() error {
 	if h == nil {
 		return errNotInitialized
 	}
-	if h.Time <= 0 {
-		return errors.Errorf("time=%d is not positive", h.Time)
+
+	minP := int64(1)
+	p := uint8(h.Parallelism)
+	if h.Parallelism < minP || h.Parallelism > MaxParallelism {
+		if h.TruncationFixed || p == 0 {
+			return errors.Errorf("parallelism cost %d is not in range [%d, %d]",
+				h.Parallelism, minP, MaxParallelism)
+		}
+		// Previously we unconditionally casted costs.Parallelism to a uint8,
+		// so we replicate this behavior for backwards compatibility.
+		log.Printf("WARNING: Truncating parallelism cost of %d to %d", h.Parallelism, p)
 	}
-	if h.Parallelism <= 0 {
-		return errors.Errorf("parallelism=%d is not positive", h.Parallelism)
+
+	minT := int64(1)
+	maxT := int64(math.MaxUint32)
+	if h.Time < minT || h.Time > maxT {
+		return errors.Errorf("time cost %d is not in range [%d, %d]", h.Time, minT, maxT)
 	}
-	minMemory := 8 * h.Parallelism
-	if h.Memory < minMemory {
-		return errors.Errorf("memory=%d is less than minimum (%d)", h.Memory, minMemory)
+
+	minM := 8 * int64(p)
+	maxM := int64(math.MaxUint32)
+	if h.Memory < minM || h.Memory > maxM {
+		return errors.Errorf("memory cost %d KiB is not in range [%d, %d]", h.Memory, minM, maxM)
 	}
 	return nil
 }