Fix: Disallow timestamp requests where digest length is inconsistent with hash algorithm (#1066)

* Validate hash algorithm availability and digest length in timestamp requests

Signed-off-by: Aaron Lew <[email protected]>

* Add timestamp request verification to JSON request parser

Signed-off-by: Aaron Lew <[email protected]>

* Create VerifyTimestampRequest and verify hash algorithm supported

Signed-off-by: Aaron Lew <[email protected]>

* Fix typo in comment

Signed-off-by: Aaron Lew <[email protected]>

* Fix case of verifyTimestampRequest function name

Signed-off-by: Aaron Lew <[email protected]>

---------

Signed-off-by: Aaron Lew <[email protected]>

Author: aaronlew02

pkg/api/error.go

@@ -28,8 +28,9 @@ import (
 )
 
 const (
-	failedToGenerateTimestampResponse = "Error generating timestamp response"
-	WeakHashAlgorithmTimestampRequest = "Weak hash algorithm in timestamp request"
+	failedToGenerateTimestampResponse        = "Error generating timestamp response"
+	WeakHashAlgorithmTimestampRequest        = "Weak hash algorithm in timestamp request"
+	InconsistentDigestLengthTimestampRequest = "Message digest has incorrect length for specified algorithm"
 )
 
 func errorMsg(message string, code int) *models.Error {

pkg/api/timestamp.go

@@ -99,7 +99,7 @@ func ParseJSONRequest(reqBytes []byte) (*timestamp.Request, string, error) {
 		TSAPolicyOID:  oidInts,
 	}
 
-	return &tsReq, "", nil
+	return verifyTimestampRequest(&tsReq)
 }
 
 func parseDERRequest(reqBytes []byte) (*timestamp.Request, string, error) {
@@ -108,12 +108,7 @@ func parseDERRequest(reqBytes []byte) (*timestamp.Request, string, error) {
 		return nil, failedToGenerateTimestampResponse, err
 	}
 
-	// verify that the request's hash algorithm is supported
-	if err := verification.VerifyRequest(parsed); err != nil {
-		return nil, WeakHashAlgorithmTimestampRequest, err
-	}
-
-	return parsed, "", nil
+	return verifyTimestampRequest(parsed)
 }
 
 func getContentType(r *http.Request) (string, error) {
@@ -188,3 +183,23 @@ func TimestampResponseHandler(params ts.GetTimestampResponseParams) middleware.R
 func GetTimestampCertChainHandler(_ ts.GetTimestampCertChainParams) middleware.Responder {
 	return ts.NewGetTimestampCertChainOK().WithPayload(api.certChainPem)
 }
+
+func verifyTimestampRequest(tsReq *timestamp.Request) (*timestamp.Request, string, error) {
+	if err := verification.VerifyRequest(tsReq); err != nil {
+		// verify that the request's hash algorithm is not weak
+		if errors.Is(err, verification.ErrWeakHashAlg) {
+			return nil, WeakHashAlgorithmTimestampRequest, err
+		}
+		// verify that the request's hash algorithm is supported
+		if errors.Is(err, verification.ErrUnsupportedHashAlg) {
+			return nil, failedToGenerateTimestampResponse, err
+		}
+		// verify that the request's digest length is consistent with the request's hash algorithm
+		if errors.Is(err, verification.ErrInconsistentDigestLength) {
+			return nil, InconsistentDigestLengthTimestampRequest, err
+		}
+		return nil, failedToGenerateTimestampResponse, err
+	}
+
+	return tsReq, "", nil
+}

pkg/verification/verify_request.go

@@ -16,17 +16,32 @@ package verification
 
 import (
 	"crypto"
+	"fmt"
 
 	"github.com/digitorus/timestamp"
 	"github.com/pkg/errors"
 )
 
 var ErrWeakHashAlg = errors.New("weak hash algorithm: must be SHA-256, SHA-384, or SHA-512")
+var ErrUnsupportedHashAlg = errors.New("unsupported hash algorithm")
+var ErrInconsistentDigestLength = errors.New("digest length inconsistent with specified hash algorithm")
 
 func VerifyRequest(ts *timestamp.Request) error {
 	// only SHA-1, SHA-256, SHA-384, and SHA-512 are supported by the underlying library
-	if ts.HashAlgorithm == crypto.SHA1 {
+	switch ts.HashAlgorithm {
+	case crypto.SHA1:
 		return ErrWeakHashAlg
+	case crypto.SHA256, crypto.SHA384, crypto.SHA512:
+	default:
+		return ErrUnsupportedHashAlg
 	}
+
+	expectedDigestLength := ts.HashAlgorithm.Size()
+	actualDigestLength := len(ts.HashedMessage)
+
+	if actualDigestLength != expectedDigestLength {
+		return fmt.Errorf("%w: expected %d bytes, got %d bytes", ErrInconsistentDigestLength, expectedDigestLength, actualDigestLength)
+	}
+
 	return nil
 }

pkg/verification/verify_request_test.go

@@ -16,23 +16,63 @@ package verification
 
 import (
 	"crypto"
+	"errors"
 	"testing"
 
 	"github.com/digitorus/timestamp"
 )
 
 func TestVerifyRequest(t *testing.T) {
-	tsReq := &timestamp.Request{}
-
-	for _, alg := range []crypto.Hash{crypto.SHA256, crypto.SHA384, crypto.SHA512} {
-		tsReq.HashAlgorithm = alg
-		if err := VerifyRequest(tsReq); err != nil {
-			t.Fatalf("unexpected error verifying request, got %v", err)
-		}
+	tests := []struct {
+		name          string
+		tsReq         *timestamp.Request
+		expectedError error
+	}{
+		{
+			name:          "Valid SHA256",
+			tsReq:         &timestamp.Request{HashAlgorithm: crypto.SHA256, HashedMessage: make([]byte, crypto.SHA256.Size())},
+			expectedError: nil,
+		},
+		{
+			name:          "Valid SHA384",
+			tsReq:         &timestamp.Request{HashAlgorithm: crypto.SHA384, HashedMessage: make([]byte, crypto.SHA384.Size())},
+			expectedError: nil,
+		},
+		{
+			name:          "Valid SHA512",
+			tsReq:         &timestamp.Request{HashAlgorithm: crypto.SHA512, HashedMessage: make([]byte, crypto.SHA512.Size())},
+			expectedError: nil,
+		},
+		{
+			name:          "Weak Hash SHA1",
+			tsReq:         &timestamp.Request{HashAlgorithm: crypto.SHA1, HashedMessage: make([]byte, crypto.SHA1.Size())},
+			expectedError: ErrWeakHashAlg,
+		},
+		{
+			name:          "Unsupported Hash Algorithm",
+			tsReq:         &timestamp.Request{HashAlgorithm: crypto.SHA224, HashedMessage: make([]byte, crypto.SHA224.Size())},
+			expectedError: ErrUnsupportedHashAlg,
+		},
+		{
+			name:          "Inconsistent Digest Length",
+			tsReq:         &timestamp.Request{HashAlgorithm: crypto.SHA256, HashedMessage: make([]byte, 31)}, // SHA256 size is 32
+			expectedError: ErrInconsistentDigestLength,
+		},
 	}
 
-	tsReq.HashAlgorithm = crypto.SHA1
-	if err := VerifyRequest(tsReq); err != ErrWeakHashAlg {
-		t.Fatalf("expected error with weak hash algorithm, got %v", err)
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := VerifyRequest(tc.tsReq)
+			if tc.expectedError != nil {
+				if err == nil {
+					t.Fatalf("expected error %v, got nil", tc.expectedError)
+				}
+				if !errors.Is(err, tc.expectedError) {
+					t.Fatalf("expected error to be or wrap %v, but got %v (error message: %q)", tc.expectedError, err, err.Error())
+				}
+			} else if err != nil {
+				t.Fatalf("expected no error, but got %v", err)
+			}
+		})
 	}
 }