Fix off-by-one error in checkInclusion. (#83)

If tree size is X, the highest value we can have for `end` in get-entries is X-1.
letsencrypt · Feb 7, 2019 · 88a177e · 88a177e
1 parent 7aedd1f
commit 88a177e
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 3 deletions.
diff --git a/monitor/inclusion_checker.go b/monitor/inclusion_checker.go
@@ -168,7 +168,7 @@ func (ic *inclusionChecker) checkInclusion() error {
 				"Unable to get entries for consistency check", newTreeSize, current)
 		return nil
 	}
-	newHead, entries, err := ic.getEntries(current, newTreeSize)
+	newHead, entries, err := ic.getEntries(current, newTreeSize-1)
 	if err != nil {
 		inclusionErrors.WithLabelValues(ic.logURI, "getEntries").Inc()
 		return fmt.Errorf("error retrieving entries from %q: %s", ic.logURI, err)

diff --git a/monitor/inclusion_checker_test.go b/monitor/inclusion_checker_test.go
@@ -10,6 +10,7 @@ import (
 	"crypto/x509"
 	"encoding/base64"
 	"errors"
+	"fmt"
 	"log"
 	"os"
 	"testing"
@@ -338,7 +339,9 @@ func TestCheckInclusion(t *testing.T) {
 			stdout: log.New(os.Stdout, "", log.LstdFlags),
 			stderr: log.New(os.Stdout, "", log.LstdFlags),
 		},
-		&InclusionOptions{},
+		&InclusionOptions{
+			FetchBatchSize: 1000,
+		},
 		mc,
 		logKey,
 		mdb)
@@ -391,14 +394,66 @@ func TestCheckInclusion(t *testing.T) {
 	mc.GetSTHFunc = func(context.Context) (*ct.SignedTreeHead, error) {
 		return &ct.SignedTreeHead{TreeSize: 2}, nil
 	}
-	mc.GetEntriesFunc = func(context.Context, int64, int64) ([]ct.LogEntry, error) {
+	mc.GetEntriesFunc = func(_ context.Context, start, end int64) ([]ct.LogEntry, error) {
 		return nil, errors.New("bad")
 	}
 	err = ic.checkInclusion()
 	if err == nil {
 		t.Fatal("Expected checkInclusion to fail when getEntries failed")
 	}
 
+	// Provide a picky GetEntriesFunc that rejects `end` if it's too big. Test for
+	// an off-by-one error we used to have.
+	treeSize := 2
+	mdb.UpdateIndexFunc = func(int64, int64) error {
+		return nil
+	}
+	mc.GetSTHFunc = func(context.Context) (*ct.SignedTreeHead, error) {
+		return &ct.SignedTreeHead{TreeSize: uint64(treeSize)}, nil
+	}
+	mc.GetEntriesFunc = func(_ context.Context, start, end int64) ([]ct.LogEntry, error) {
+		if end >= int64(treeSize) {
+			return nil, fmt.Errorf("end of range is greater than or equal to tree size. Got end=%d", end)
+		}
+		tree := []ct.LogEntry{
+			{
+				Precert: &ct.Precertificate{
+					IssuerKeyHash:  [32]byte{},
+					TBSCertificate: &ctx509.Certificate{Raw: []byte{1, 2, 3}},
+					Submitted:      ct.ASN1Cert{Data: []byte{1, 2, 3}},
+				},
+				Leaf: ct.MerkleTreeLeaf{TimestampedEntry: &ct.TimestampedEntry{
+					EntryType: ct.PrecertLogEntryType,
+					Timestamp: 1234,
+					PrecertEntry: &ct.PreCert{
+						IssuerKeyHash:  [32]byte{},
+						TBSCertificate: []byte{1, 2, 3},
+					},
+				}},
+			},
+			{
+				Precert: &ct.Precertificate{
+					IssuerKeyHash:  [32]byte{},
+					TBSCertificate: &ctx509.Certificate{Raw: []byte{1, 2, 3}},
+					Submitted:      ct.ASN1Cert{Data: []byte{1, 2, 3}},
+				},
+				Leaf: ct.MerkleTreeLeaf{TimestampedEntry: &ct.TimestampedEntry{
+					EntryType: ct.PrecertLogEntryType,
+					Timestamp: 1234,
+					PrecertEntry: &ct.PreCert{
+						IssuerKeyHash:  [32]byte{},
+						TBSCertificate: []byte{1, 2, 3},
+					},
+				}},
+			},
+		}
+		return tree[start : end+1], nil
+	}
+	err = ic.checkInclusion()
+	if err != nil {
+		t.Fatalf("Expected checkInclusion to call GetEntries with end smaller than tree size. Got %s", err)
+	}
+
 	mc.GetEntriesFunc = func(context.Context, int64, int64) ([]ct.LogEntry, error) {
 		return []ct.LogEntry{
 			{