Repository: google/exposure-notifications-server
Pull request #865 (2020-08-12 23:00:24)

Fix revision token validation
Previously when validating a revision token, we would accept a full payload of TEK+metadata, and then immediately perform a database lookup on the TEK, ignoring any metadata. As a result, users could make requests with arbitrarily changed metadata, and the server would never throw a validation error. The server would not actually _accept_ those changed metadata values, it would just happily ignore them and use the persisted values in the database. This commit adds validation to ensure the incoming TEK metadata matches the database TEK metadata matches the revision token metadata. It also improves the logging around these cases.

In a similar vein, the only way the above is possible is due to a bug in how we construct revision token. This commit also inverts the logic to always include ALL previous revision tokens (and their metadata) FIRST in the revision token, then include any newly uploaded TEKs. This ensures that a partial upload of new keys would still return a revision token that could be used to revise any of the previous 14 days of keys, not just the uploaded ones.

Fixes GH-864

**Release Note**

```release-note
Fix revision token validation and ensure revision tokens are valid for the previous 14 days of keys, not just the keys uploaded in the revision request.
```

/assign @mikehelmick 

google-oss-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/865#" title="Author self-approved">sethvargo</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/google/exposure-notifications-server/blob/main/OWNERS)~~ [sethvargo]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
google-oss-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/865#pullrequestreview-466337146" title="Approved">mikehelmick</a>*, *<a href="https://github.com/google/exposure-notifications-server/pull/865#" title="Author self-approved">sethvargo</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/google/exposure-notifications-server/blob/main/OWNERS)~~ [mikehelmick,sethvargo]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/internal/publish/database/exposure.go b/internal/publish/database/exposure.go
index 1737b3d..a34e8da 100644
--- a/internal/publish/database/exposure.go
+++ b/internal/publish/database/exposure.go
@@ -44,12 +44,19 @@ var (
 	// ErrExistingKeyNotInToken is returned when attempting to present an exposure that already exists, but
 	// isn't in the provided revision token.
 	ErrExistingKeyNotInToken = errors.New("sent existing exposure key that is not in revision token")
+
 	// ErrNoRevisionToken is returned when presenting exposures that already exists, but no revision
 	// token was presented.
 	ErrNoRevisionToken = errors.New("sent existing exposures but no revision token present")
+
 	// ErrRevisionTokenMetadataMismatch is returned when a revision token has the correct TEK in it,
 	// but the new request is attempting to change the metadata of the key (intervalNumber/Count)
-	ErrRevisionTokenMetadataMismatch = errors.New("changing exposure key metadata is not allowed ")
+	ErrRevisionTokenMetadataMismatch = errors.New("changing exposure key metadata is not allowed")
+
+	// ErrIncomingMetadataMismatch is returned when incoming data has a known TEK
+	// in it, but the new request is attempting to change the metadata of the key
+	// (intervalNumber/Count).
+	ErrIncomingMetadataMismatch = errors.New("incoming exposure key metadata does not match expected values")
 )
 
 type PublishDB struct {
@@ -365,53 +372,86 @@ func (db *PublishDB) InsertAndReviseExposures(ctx context.Context, incoming []*m
 	logger := logging.FromContext(ctx)
 	updated := 0
 	err := db.db.InTx(ctx, pgx.ReadCommitted, func(tx pgx.Tx) error {
-		// Read any existing TEKs FOR UPDATE in this transaction.
+		// Build the base64-encoded list of keys - this is needed so we can lookup
+		// the keys in the database. Also build a lookup map by key for validation
+		// later.
 		b64keys := make([]string, len(incoming))
-		for i, e := range incoming {
-			b64keys[i] = e.ExposureKeyBase64()
+		incomingMap := make(map[string]*model.Exposure, len(incoming))
+		for i, v := range incoming {
+			b64keys[i] = v.ExposureKeyBase64()
+			incomingMap[v.ExposureKeyBase64()] = v
 		}
+
+		// Lookup the keys in the database and build a lookup map for validation
+		// later.
 		existing, err := db.ReadExposures(ctx, tx, b64keys)
 		if err != nil {
 			return fmt.Errorf("unable to check for existing records")
 		}
+		existingMap := make(map[string]*model.Exposure, len(existing))
+		for _, v := range existing {
+			existingMap[v.ExposureKeyBase64()] = v
+		}
 
-		// See if the revision token is relevant. We only need to check it if keys are being revised.
+		// See if the revision token is relevant. We only need to check it if keys
+		// are being revised.
 		if len(existing) > 0 {
-			// Turn the token data into a map for easy comparison.
-			allowedRevisions := make(map[string]*pb.RevisableKey)
-			if token != nil {
-				for _, rk := range token.RevisableKeys {
-					allowedRevisions[base64.StdEncoding.EncodeToString(rk.TemporaryExposureKey)] = rk
-				}
-			}
-			if len(allowedRevisions) == 0 {
-				logger.Errorf("attempt to revise keys, but no revision token presented")
+			// Check if a revision token is required.
+			if token == nil {
+				logger.Errorw("attempted to revise keys, but revision token is missing")
 				if tokenRequired {
 					return ErrNoRevisionToken
 				}
 			}
-			// Check that any existing exposures are present in the token. It doesn't mater if they
-			// would be materially changed or not (the revise keys steps below)
-			for k, v := range existing {
-				if rk, ok := allowedRevisions[k]; !ok {
-					// user sent in an existing key they they do not have the token for.
-					logger.Errorf("attempt to revise key not in revision token")
-					if tokenRequired {
-						return ErrExistingKeyNotInToken
+
+			// Build a map of allowed revisions for validation and comparison.
+			allowedRevisions := make(map[string]*pb.RevisableKey, len(token.RevisableKeys))
+			for _, v := range token.RevisableKeys {
+				b := base64.StdEncoding.EncodeToString(v.TemporaryExposureKey)
+				allowedRevisions[b] = v
+			}
+
+			// Check that any existing exposures are present in the token.
+			for k, ex := range existing {
+				// Check the incoming values first.
+				if in, ok := incomingMap[k]; ok {
+					if ex.IntervalNumber != in.IntervalNumber || ex.IntervalCount != in.IntervalCount {
+						logger.Errorw("incoming metadata mismatch",
+							"existing_count", ex.IntervalCount,
+							"existing_number", ex.IntervalNumber,
+							"incoming_count", in.IntervalCount,
+							"incoming_number", in.IntervalNumber)
+						if tokenRequired {
+							return ErrIncomingMetadataMismatch
+						}
 					}
-				} else {
-					// Validate that the key parameters haven't changed.
-					if v.IntervalNumber != rk.IntervalNumber || v.IntervalCount != rk.IntervalCount {
-						logger.Errorf("revision token metadata mismatch")
+				}
+
+				// Now check against allowed revisions.
+				if rk, ok := allowedRevisions[k]; ok {
+					if ex.IntervalNumber != rk.IntervalNumber || ex.IntervalCount != rk.IntervalCount {
+						logger.Errorw("token metadata mismatch",
+							"existing_count", ex.IntervalCount,
+							"existing_number", ex.IntervalNumber,
+							"incoming_count", rk.IntervalCount,
+							"incoming_number", rk.IntervalNumber)
 						if tokenRequired {
 							return ErrRevisionTokenMetadataMismatch
 						}
 					}
+				} else {
+					// User sent in an existing key they they do not have the token for.
+					logger.Errorw("cannot revise key: not in revision token")
+					if tokenRequired {
+						return ErrExistingKeyNotInToken
+					}
 				}
 			}
-			// Revision token is valid for this request, not required, or bypassed.
 		}
 
+		// If we got this far, the revision token is valid for this request, not
+		// required, or bypassed.
+
 		// Run through the merge logic.
 		exposures, err := model.ReviseKeys(ctx, existing, incoming)
 		if err != nil {
diff --git a/internal/revision/revision.go b/internal/revision/revision.go
index f7fcb44..d60df8c 100644
--- a/internal/revision/revision.go
+++ b/internal/revision/revision.go
@@ -186,24 +186,27 @@ func buildTokenBufer(previous *pb.RevisionTokenData, eKeys []*model.Exposure) *p
 		RevisableKeys: make([]*pb.RevisableKey, 0, len(eKeys)),
 	}
 	got := make(map[string]struct{})
-	for _, k := range eKeys {
-		got[k.ExposureKeyBase64()] = struct{}{}
-		pbKey := pb.RevisableKey{
-			TemporaryExposureKey: make([]byte, len(k.ExposureKey)),
-			IntervalNumber:       k.IntervalNumber,
-			IntervalCount:        k.IntervalCount,
-		}
-		copy(pbKey.TemporaryExposureKey, k.ExposureKey)
-		tokenData.RevisableKeys = append(tokenData.RevisableKeys, &pbKey)
-	}
-	// Add in previous keys that weren't also in the new exposures
+
+	// Add in previous keys from the revision token. This needs to come first so
+	// the revision token is valid for all keys, not just the ones uploaded now.
 	if previous != nil {
 		for _, rk := range previous.RevisableKeys {
-			if _, ok := got[base64.StdEncoding.EncodeToString(rk.TemporaryExposureKey)]; !ok {
-				tokenData.RevisableKeys = append(tokenData.RevisableKeys, rk)
-			}
+			got[base64.StdEncoding.EncodeToString(rk.TemporaryExposureKey)] = struct{}{}
+			tokenData.RevisableKeys = append(tokenData.RevisableKeys, rk)
 		}
 	}
+
+	// Now add new keys and their metadata, iff they aren't already in the list.
+	for _, k := range eKeys {
+		if _, ok := got[k.ExposureKeyBase64()]; !ok {
+			tokenData.RevisableKeys = append(tokenData.RevisableKeys, &pb.RevisableKey{
+				TemporaryExposureKey: append([]byte{}, k.ExposureKey...), // deep copy
+				IntervalNumber:       k.IntervalNumber,
+				IntervalCount:        k.IntervalCount,
+			})
+		}
+	}
+
 	return &tokenData
 }
 


Test patch:
diff --git a/internal/publish/database/exposure_test.go b/internal/publish/database/exposure_test.go
index d8d018e..b100be2 100644
--- a/internal/publish/database/exposure_test.go
+++ b/internal/publish/database/exposure_test.go
@@ -16,6 +16,7 @@ package database
 
 import (
 	"context"
+	"crypto/rand"
 	"errors"
 	"fmt"
 	"sort"
@@ -244,6 +245,20 @@ func listExposures(ctx context.Context, db *PublishDB, c IterateExposuresCriteri
 	return exps, nil
 }
 
+func testRandomTEK(tb testing.TB) []byte {
+	tb.Helper()
+
+	b := make([]byte, 16)
+	n, err := rand.Read(b)
+	if err != nil {
+		tb.Fatal(err)
+	}
+	if n < 16 {
+		tb.Fatalf("expected %v to be %v", n, 16)
+	}
+	return b
+}
+
 func TestReviseExposures(t *testing.T) {
 	t.Parallel()
 
@@ -254,8 +269,7 @@ func TestReviseExposures(t *testing.T) {
 	createdAt := time.Now().UTC().Add(-2 * time.Hour).Truncate(time.Hour)
 	revisedAt := time.Now().UTC().Truncate(time.Hour)
 
-	existingTEK := []byte{1, 1, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4}
-
+	existingTEK := testRandomTEK(t)
 	existingExp := &model.Exposure{
 		ExposureKey:      existingTEK,
 		TransmissionRisk: verifyapi.TransmissionRiskClinical,
@@ -274,6 +288,88 @@ func TestReviseExposures(t *testing.T) {
 		t.Fatalf("wrong number of changed exposures, want: 1, got: %v", n)
 	}
 
+	// Attempt to revise without a revision token - this should fail.
+	{
+		revisedExp := &model.Exposure{}
+		*revisedExp = *existingExp
+		revisedExp.TransmissionRisk = verifyapi.TransmissionRiskConfirmedStandard
+		revisedExp.CreatedAt = revisedAt
+		revisedExp.ReportType = verifyapi.ReportTypeConfirmed
+
+		if _, err := pubDB.InsertAndReviseExposures(ctx, []*model.Exposure{revisedExp}, nil, true); !errors.Is(err, ErrNoRevisionToken) {
+			t.Fatalf("expected %#v to be %#v", err, ErrNoRevisionToken)
+		}
+	}
+
+	// Attempt to revise with a revision token that doesn't include the TEK - this
+	// should fail.
+	{
+		revisedExp := &model.Exposure{}
+		*revisedExp = *existingExp
+		revisedExp.TransmissionRisk = verifyapi.TransmissionRiskConfirmedStandard
+		revisedExp.CreatedAt = revisedAt
+		revisedExp.ReportType = verifyapi.ReportTypeConfirmed
+
+		token := &pb.RevisionTokenData{
+			RevisableKeys: []*pb.RevisableKey{
+				{
+					TemporaryExposureKey: testRandomTEK(t), // TEK mismatch
+					IntervalNumber:       100,
+					IntervalCount:        144,
+				},
+			},
+		}
+
+		if _, err := pubDB.InsertAndReviseExposures(ctx, []*model.Exposure{revisedExp}, token, true); !errors.Is(err, ErrExistingKeyNotInToken) {
+			t.Fatalf("expected %#v to be %#v", err, ErrExistingKeyNotInToken)
+		}
+	}
+
+	// Attempt to revise with a revision token that has the wrong metadata - this
+	// should fail.
+	{
+		revisedExp := &model.Exposure{}
+		*revisedExp = *existingExp
+		revisedExp.TransmissionRisk = verifyapi.TransmissionRiskConfirmedStandard
+		revisedExp.CreatedAt = revisedAt
+		revisedExp.ReportType = verifyapi.ReportTypeConfirmed
+
+		token := &pb.RevisionTokenData{
+			RevisableKeys: []*pb.RevisableKey{
+				{
+					TemporaryExposureKey: existingTEK,
+					IntervalNumber:       101, // Changed, should cause failure
+					IntervalCount:        144,
+				},
+			},
+		}
+
+		if _, err := pubDB.InsertAndReviseExposures(ctx, []*model.Exposure{revisedExp}, token, true); !errors.Is(err, ErrRevisionTokenMetadataMismatch) {
+			t.Fatalf("expected %#v to be %#v", err, ErrRevisionTokenMetadataMismatch)
+		}
+	}
+
+	// Attempt to modify metadata on incoming keys - this should fail.
+	{
+		revisedExp := &model.Exposure{}
+		*revisedExp = *existingExp
+		revisedExp.IntervalCount = 124 // Changed, should make request invalid
+
+		token := &pb.RevisionTokenData{
+			RevisableKeys: []*pb.RevisableKey{
+				{
+					TemporaryExposureKey: existingTEK,
+					IntervalNumber:       101, // Changed, should cause failure
+					IntervalCount:        144,
+				},
+			},
+		}
+
+		if _, err := pubDB.InsertAndReviseExposures(ctx, []*model.Exposure{revisedExp}, token, true); !errors.Is(err, ErrIncomingMetadataMismatch) {
+			t.Fatalf("expected %#v to be %#v", err, ErrIncomingMetadataMismatch)
+		}
+	}
+
 	// Modify the existing on in place.
 	revisedExp := &model.Exposure{
 		ExposureKey:      existingTEK,
@@ -309,38 +405,6 @@ func TestReviseExposures(t *testing.T) {
 		t.Fatalf("wrong error, want: '%v' got: %v", ErrNoRevisionToken, err)
 	}
 
-	// Build a wrong revision token
-	{
-		var badToken pb.RevisionTokenData
-		badToken.RevisableKeys = append(badToken.RevisableKeys, &pb.RevisableKey{
-			TemporaryExposureKey: []byte{0, 0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3},
-			IntervalNumber:       100,
-			IntervalCount:        144,
-		})
-
-		if _, err := pubDB.InsertAndReviseExposures(ctx, revisions, &badToken, true); err == nil {
-			t.Fatalf("expected error revising with bad token")
-		} else if !errors.Is(err, ErrExistingKeyNotInToken) {
-			t.Fatalf("wrong error, want: '%v' got: %v", ErrExistingKeyNotInToken, err)
-		}
-	}
-
-	// Revision token with bad metadata
-	{
-		var badMetadata pb.RevisionTokenData
-		badMetadata.RevisableKeys = append(badMetadata.RevisableKeys, &pb.RevisableKey{
-			TemporaryExposureKey: existingTEK,
-			IntervalNumber:       101,
-			IntervalCount:        144,
-		})
-
-		if _, err := pubDB.InsertAndReviseExposures(ctx, revisions, &badMetadata, true); err == nil {
-			t.Fatalf("expected error revising with bad token")
-		} else if !errors.Is(err, ErrRevisionTokenMetadataMismatch) {
-			t.Fatalf("wrong error, want: '%v' got: %v", ErrRevisionTokenMetadataMismatch, err)
-		}
-	}
-
 	// Revision token that allows revision of the revised key.
 	var token pb.RevisionTokenData
 	token.RevisableKeys = append(token.RevisableKeys, &pb.RevisableKey{
diff --git a/internal/revision/revision_test.go b/internal/revision/revision_test.go
index 91817eb..3f2f238 100644
--- a/internal/revision/revision_test.go
+++ b/internal/revision/revision_test.go
@@ -93,15 +93,43 @@ func TestBuildTokenBuffer(t *testing.T) {
 						IntervalNumber:       200144,
 						IntervalCount:        144,
 					},
+					{
+						TemporaryExposureKey: []byte{8, 9, 10, 11},
+						IntervalNumber:       200144,
+						IntervalCount:        82,
+					},
 					{
 						TemporaryExposureKey: []byte{1, 2, 3, 4},
 						IntervalNumber:       200000,
 						IntervalCount:        144,
 					},
+				},
+			},
+		},
+		{
+			name: "merge_duplicates_prefers_previous",
+			publish: []*model.Exposure{
+				{
+					ExposureKey:    []byte{4, 3, 2, 1},
+					IntervalNumber: 200134,
+					IntervalCount:  122,
+				},
+			},
+			previous: &pb.RevisionTokenData{
+				RevisableKeys: []*pb.RevisableKey{
 					{
-						TemporaryExposureKey: []byte{8, 9, 10, 11},
+						TemporaryExposureKey: []byte{4, 3, 2, 1},
 						IntervalNumber:       200144,
-						IntervalCount:        82,
+						IntervalCount:        144,
+					},
+				},
+			},
+			want: &pb.RevisionTokenData{
+				RevisableKeys: []*pb.RevisableKey{
+					{
+						TemporaryExposureKey: []byte{4, 3, 2, 1},
+						IntervalNumber:       200144,
+						IntervalCount:        144,
 					},
 				},
 			},