Fix security errors in lint steps #2061

tstirrat15 · 2024-09-09T16:48:10Z

Description

A new go version was published that fixes a security vulnerability. This takes that patch.

Changes

Bump go version in root and magefile

Testing

Review. See that things pass.

ecordell

there's another go.mod in e2e as well

tstirrat15

See additional comments

tstirrat15 · 2024-09-11T21:42:22Z

internal/datastore/common/gc_test.go

@@ -68,7 +68,7 @@ func (gc *fakeGC) DeleteBeforeTx(_ context.Context, rev datastore.Revision) (Del

 	revInt := rev.(revisions.TransactionIDRevision).TransactionID()

-	return gc.deleter.DeleteBeforeTx(int64(revInt))


This seemed like a type mismatch, so I propagated it through. I'd welcome a check of that assumption.

Yeah, seems so

tstirrat15 · 2024-09-11T21:45:44Z

internal/datastore/common/changes.go

+	// We checked for underflow above, so the current byte size
+	// should fit in a uint64
+	currentByteSize, _ := safecast.ToUint64(ch.currentByteSize)
+
+	if currentByteSize > ch.maxByteSize {


As I was going through these, I tended to go one of four ways:

Something about the context makes it obvious that the cast is safe, so I used safecast to mark it as such by ignoring the error

There's potential for a miscast and it seems like the situation would represent a bug, so I return a MustBugF

There's potential for a miscast and it seems like it could realistically happen, so I return a normal error

It was possible to redeclare the code in such a way that largely avoided casting issues

I think we should return the error as a MustBugf even if we know it can be safe; it is "extra" code but it means if anything changes we're guaranteed to catch the error vs having a silent bug appear

tstirrat15 · 2024-09-11T21:48:32Z

internal/datastore/crdb/pool/balancer.go

+	for seed == 0 {
+		// Sum64 returns a uint64, and safecast will return 0 if it's not castable,
+		// which will happen about half the time (?). We just keep running it until
+		// we get a seed that fits in the box.
+		// Subtracting math.MaxInt64 should mean that we retain the entire range of
+		// possible values.
+		seed, _ = safecast.ToInt64(new(maphash.Hash).Sum64() - math.MaxInt64)
+	}


This felt a little wonky but seemed to capture the desired effect.

I think we should just make the seed a uint64 then? @ecordell?

NewSource() and Seed() both take an int64, which is what this value is used for.

interpreting the uint64 bits as int64 bits doesn't really matter for this application. the requirements for this use of rand are super minimal, we really just want "don't do the same exact thing on every replica 100% of the time".

can we just divide it by 2 and then cast? or is there a way to mark this specific "unsafe" cast as "I don't care about wraparound at all"?

Yeah, we can // ignore gosec for this particular case, or we can use the approach I came up with. I'm fine with either.

tstirrat15 · 2024-09-11T21:50:32Z

internal/datastore/postgres/common/pgx.go

+		maxConns, err := safecast.ToInt32(*opts.MaxOpenConns)
+		if err != nil {
+			return err
+		}
+		pgxConfig.MaxConns = maxConns


This would require a user to put a very specific value in, but we might as well catch it.

tstirrat15 · 2024-09-11T21:51:46Z

internal/datastore/postgres/revisions.go

-	xminInt := int64(decoded.Xmin)
+	xminInt, err := safecast.ToInt64(decoded.Xmin)
+	if err != nil {
+		return datastore.NoRevision, spiceerrors.MustBugf("Could not cast xmin to int64")


This seemed like it would be a bug since we were the ones who stored the revision in the first place

tstirrat15 · 2024-09-11T22:05:22Z

internal/dispatch/graph/lookupresources2_test.go

+						uintPageSize, err := safecast.ToUint32(pageSize)
+						require.NoError(err)


I made a bunch of these changes to tests. They're overkill, especially since the values are statically defined in most cases, so I can walk these back out if desired.

tstirrat15 · 2024-09-11T22:06:32Z

pkg/cache/cache_otter.go

+		// We make an assumption that if the cast fails, it's because the value
+		// was too big, so we set to maxint in that case.
+		uintCost = math.MaxUint32
+	}


Please check this

Log it as well, but this seems fine

tstirrat15 · 2024-09-11T22:07:03Z

pkg/cmd/datastore/datastore.go

+	maxRetries, err := safecast.ToUint8(opts.MaxRetries)
+	if err != nil {
+		return nil, errors.New("max-retries could not be cast to uint8")
+	}


Another branch that we're unlikely to hit, but it could result in very weird behavior if we do.

tstirrat15 · 2024-09-11T22:14:37Z

pkg/development/assertions.go

@@ -42,14 +43,18 @@ func runAssertions(devContext *DevContext, assertions []blocks.Assertion, expect
 	for _, assertion := range assertions {
 		tpl := tuple.MustFromRelationship[*v1t.ObjectReference, *v1t.SubjectReference, *v1t.ContextualizedCaveat](assertion.Relationship)

+		// NOTE: zeroes are fine here to mean "unknown"


If safecast can't successfully cast, it returns zero. For pointing at line numbers, I think this should be fine (especially since you'd have to do something weird to end up out of bounds here).

We should either log the error or have it panic

tstirrat15 · 2024-09-11T22:15:27Z

pkg/genutil/ensure.go

@@ -13,16 +17,18 @@ func MustEnsureUInt32(value int) uint32 {

 // EnsureUInt32 ensures that the specified value can be represented as a uint32.
 func EnsureUInt32(value int) (uint32, error) {


These functions are basically doing safecast things anyway, so I rewrote them in terms of safecast to silence the errors. We can upstream it later.

josephschorr · 2024-09-14T01:19:45Z

internal/datastore/common/changes.go

-	if ch.currentByteSize > int64(ch.maxByteSize) {
+	// We checked for underflow above, so the current byte size
+	// should fit in a uint64
+	currentByteSize, _ := safecast.ToUint64(ch.currentByteSize)


return the error even if we think it cannot be hit; its safer than ignoring it

josephschorr · 2024-09-14T01:20:22Z

internal/datastore/common/changes.go

+	// We checked for underflow above, so the current byte size
+	// should fit in a uint64
+	currentByteSize, _ := safecast.ToUint64(ch.currentByteSize)
+
+	if currentByteSize > ch.maxByteSize {


I think we should return the error as a MustBugf even if we know it can be safe; it is "extra" code but it means if anything changes we're guaranteed to catch the error vs having a silent bug appear

josephschorr · 2024-09-14T01:20:36Z

internal/datastore/common/gc_test.go

@@ -68,7 +68,7 @@ func (gc *fakeGC) DeleteBeforeTx(_ context.Context, rev datastore.Revision) (Del

 	revInt := rev.(revisions.TransactionIDRevision).TransactionID()

-	return gc.deleter.DeleteBeforeTx(int64(revInt))


Yeah, seems so

josephschorr · 2024-09-14T01:21:40Z

internal/datastore/crdb/pool/balancer.go

+	for seed == 0 {
+		// Sum64 returns a uint64, and safecast will return 0 if it's not castable,
+		// which will happen about half the time (?). We just keep running it until
+		// we get a seed that fits in the box.
+		// Subtracting math.MaxInt64 should mean that we retain the entire range of
+		// possible values.
+		seed, _ = safecast.ToInt64(new(maphash.Hash).Sum64() - math.MaxInt64)
+	}


I think we should just make the seed a uint64 then? @ecordell?

josephschorr · 2024-09-14T01:22:24Z

internal/datastore/crdb/pool/pool.go

@@ -141,12 +142,16 @@ func (p *RetryPool) ID() string {

 // MaxConns returns the MaxConns configured on the underlying pool
 func (p *RetryPool) MaxConns() uint32 {
-	return uint32(p.pool.Config().MaxConns)
+	// This should be non-negative
+	maxConns, _ := safecast.ToUint32(p.pool.Config().MaxConns)


If we have enough of these examples, we should add our own MustToUint32 method and just have it panic if it fails

josephschorr · 2024-09-14T01:22:39Z

internal/datastore/crdb/readwrite.go

-	if delLimit > 0 && uint64(modified.RowsAffected()) == delLimit {
+	rowsAffected, err := safecast.ToUint64(modified.RowsAffected())
+	if err != nil {
+		return false, spiceerrors.MustBugf("could not cast RowsAffected to uint64")


always wrap the error when using MustBugf

In fact, we should probably add a custom linter to ensure this at some point. Might be worth filing an issue

josephschorr · 2024-09-14T01:24:30Z

pkg/cache/cache_otter.go

+		// We make an assumption that if the cast fails, it's because the value
+		// was too big, so we set to maxint in that case.
+		uintCost = math.MaxUint32
+	}


Log it as well, but this seems fine

josephschorr · 2024-09-14T01:24:55Z

pkg/development/assertions.go

@@ -42,14 +43,18 @@ func runAssertions(devContext *DevContext, assertions []blocks.Assertion, expect
 	for _, assertion := range assertions {
 		tpl := tuple.MustFromRelationship[*v1t.ObjectReference, *v1t.SubjectReference, *v1t.ContextualizedCaveat](assertion.Relationship)

+		// NOTE: zeroes are fine here to mean "unknown"


We should either log the error or have it panic

josephschorr

LGTM

tstirrat15 added 2 commits September 9, 2024 10:46

Bump go version to take go patch

99ae458

Remove erroneously-committed file

0c6bad2

tstirrat15 requested a review from a team September 9, 2024 16:48

github-actions bot added the area/dependencies Affects dependencies label Sep 9, 2024

ecordell previously approved these changes Sep 9, 2024

View reviewed changes

ecordell reviewed Sep 9, 2024

View reviewed changes

Add e2e mod file as well

6b53f31

tstirrat15 dismissed ecordell’s stale review via 6b53f31 September 9, 2024 16:49

tstirrat15 added 2 commits September 9, 2024 11:34

Bump version for analyzers

9d6db3b

Bump go version in container

8da4081

github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Sep 10, 2024

tstirrat15 added 2 commits September 10, 2024 10:39

Bump in dockerfile.release as well

efd1869

Fix missing annotation

74043c7

github-actions bot added the area/api v1 Affects the v1 API label Sep 10, 2024

Fix lint errors

9bf5f3d

github-actions bot added the area/datastore Affects the storage system label Sep 10, 2024

Fix unsafe conversion errors

5731e26

github-actions bot added area/CLI Affects the command line area/dispatch Affects dispatching of requests labels Sep 11, 2024

tstirrat15 added 2 commits September 11, 2024 16:01

Remove lossy cast

31d4af3

Simplify casts in development package

c8150ef

tstirrat15 commented Sep 11, 2024

View reviewed changes

tstirrat15 added 3 commits September 11, 2024 17:03

Fix limit max

54cba16

Attempt at test fix

b1fc26d

Lots of lint fixes

9b9b291

tstirrat15 requested a review from vroldanbet as a code owner September 12, 2024 18:40

github-actions bot added the area/schema Affects the Schema Language label Sep 12, 2024

tstirrat15 added 3 commits September 12, 2024 12:45

Run formatting

62afc3c

Fix a typo that led to nil dereference

25422a8

Get types lining up

5fd005a

tstirrat15 added 6 commits September 12, 2024 15:36

Add safecast to e2e

7fbcc18

Fix releaser issues

d8e30c6

Try using goamd64 flag to point at correct file

015bd0e

Reinstate erroneously-removed bits

0ac9768

Pin goreleaser version for now

8ad8c4e

Make files pass goreleaser-pro check

a358e68

josephschorr reviewed Sep 14, 2024

View reviewed changes

tstirrat15 added 3 commits September 16, 2024 10:15

Log errors when encountered

4b1ab86

Wrap errors where applicable

c781a10

Fix log.Err usage

d4d3071

josephschorr approved these changes Sep 16, 2024

View reviewed changes

tstirrat15 added this pull request to the merge queue Sep 16, 2024

tstirrat15 merged commit d9162eb into main Sep 16, 2024
22 checks passed

tstirrat15 deleted the fix-security-errors branch September 16, 2024 17:03

github-actions bot locked and limited conversation to collaborators Sep 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix security errors in lint steps #2061

Fix security errors in lint steps #2061

tstirrat15 commented Sep 9, 2024

ecordell left a comment

tstirrat15 left a comment

tstirrat15 Sep 11, 2024

josephschorr Sep 14, 2024

tstirrat15 Sep 11, 2024

josephschorr Sep 14, 2024

tstirrat15 Sep 11, 2024

josephschorr Sep 14, 2024

ecordell Sep 16, 2024 •

edited

Loading

tstirrat15 Sep 16, 2024

tstirrat15 Sep 11, 2024

tstirrat15 Sep 11, 2024

tstirrat15 Sep 11, 2024

tstirrat15 Sep 11, 2024

josephschorr Sep 14, 2024

tstirrat15 Sep 11, 2024

tstirrat15 Sep 11, 2024

josephschorr Sep 14, 2024

tstirrat15 Sep 11, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr Sep 14, 2024

josephschorr left a comment

		@@ -68,7 +68,7 @@ func (gc *fakeGC) DeleteBeforeTx(_ context.Context, rev datastore.Revision) (Del

		revInt := rev.(revisions.TransactionIDRevision).TransactionID()

		return gc.deleter.DeleteBeforeTx(int64(revInt))

		uintPageSize, err := safecast.ToUint32(pageSize)
		require.NoError(err)

		@@ -13,16 +17,18 @@ func MustEnsureUInt32(value int) uint32 {

		// EnsureUInt32 ensures that the specified value can be represented as a uint32.
		func EnsureUInt32(value int) (uint32, error) {

Fix security errors in lint steps #2061

Fix security errors in lint steps #2061

Conversation

tstirrat15 commented Sep 9, 2024

Description

Changes

Testing

ecordell left a comment

Choose a reason for hiding this comment

tstirrat15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr left a comment

Choose a reason for hiding this comment

ecordell Sep 16, 2024 •

edited

Loading