From f2173f1770176616997486d824a26ab926048b86 Mon Sep 17 00:00:00 2001 From: Doria Keung Date: Tue, 24 Sep 2024 16:36:57 -0400 Subject: [PATCH] Add exemptions and/or checks for G115. --- .golangci.yml | 55 +++++++++++++++++++ private/buf/bufcurl/reflection_resolver.go | 10 ++++ private/bufpkg/bufimage/bufimage.go | 6 ++ private/bufpkg/bufimage/build_image.go | 6 ++ .../option_extension_descriptor_test.go | 10 ++++ .../pkg/normalpath/normalpath_unix_test.go | 4 +- 6 files changed, 89 insertions(+), 2 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 221bb82007..3222e2676e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -287,3 +287,58 @@ issues: # trip this off. path: private/pkg/oauth2/device.go text: "G101:" + # G115 checks for integer overflow from integer conversions. There are known false + # positives from the check (https://github.com/securego/gosec/issues/1212) that are + # actively being worked on. Each exemption below is a false positive or for a safe operation, + # such as parsing indices from descriptors and/or images. + - linters: + - gosec + # Loop index conversion to uint64. + path: private/buf/bufgen/features.go + text: "G115:" + - linters: + - gosec + # Converting result from utf8.RuneCountInString to uint64. + path: private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/field.go + text: "G115:" + - linters: + - gosec + # PluginReference revision is validated with a bounds check at construction time. + path: private/bufpkg/bufremoteplugin/bufremoteplugin.go + text: "G115:" + - linters: + - gosec + # A bounds check has been added for int32 -> uint32 conversion this is being flagged + # as a false positive. + path: private/buf/bufcurl/reflection_resolver.go + text: "G115:" + - linters: + - gosec + # bufprotosource converts indices to int32 to form the source path. Since it is parsing + # from the fileDescriptor set, the operation should be safe. + path: private/bufpkg/bufprotosource/paths.go + text: "G115:" + - linters: + - gosec + # bufimageutil is handling images and converting loop indices to int32. Since it is + # parsing from an Image, the operation should be safe. + path: private/bufpkg/bufimage/bufimageutil/bufimageutil.go + text: "G115:" + - linters: + - gosec + # Bounds checks have been added with assertion statements to ensure safe int -> int32 + # conversions, this is a false positive. + path: private/bufpkg/bufprotosource/option_extension_descriptor_test.go + text: "G115:" + - linters: + - gosec + # This converts results from strconv.ParseInt with the bit size set to 32 to int32, + # so it should be a safe conversion, this is a false positive. + path: private/buf/bufprotopluginexec/version.go + text: "G115:" + - linters: + - gosec + # This checks the cel constraints from an Image, and converts loop indices to int32 + # to set the source path for the location, this operation should be safe. + path: private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/cel.go + text: "G115:" diff --git a/private/buf/bufcurl/reflection_resolver.go b/private/buf/bufcurl/reflection_resolver.go index 7436e6c5cd..6729c53fb4 100644 --- a/private/buf/bufcurl/reflection_resolver.go +++ b/private/buf/bufcurl/reflection_resolver.go @@ -164,6 +164,11 @@ func (r *reflectionResolver) ListServices() ([]protoreflect.FullName, error) { } switch response := resp.MessageResponse.(type) { case *reflectionv1.ServerReflectionResponse_ErrorResponse: + // This should never happen, however we do a bounds check to ensure we are doing a safe + // conversion from int32 (ErrorResponse.ErrorCode) to uint32 (connect.Code). + if response.ErrorResponse.ErrorCode < 0 { + return nil, fmt.Errorf("server replied with unsupported error code: %v", response.ErrorResponse.ErrorCode) + } return nil, connect.NewWireError(connect.Code(response.ErrorResponse.ErrorCode), errors.New(response.ErrorResponse.ErrorMessage)) case *reflectionv1.ServerReflectionResponse_ListServicesResponse: serviceNames := make([]protoreflect.FullName, len(response.ListServicesResponse.Service)) @@ -338,6 +343,11 @@ func (r *reflectionResolver) fileByNameLocked(name string) ([]*descriptorpb.File func descriptorsInResponse(resp *reflectionv1.ServerReflectionResponse) ([]*descriptorpb.FileDescriptorProto, error) { switch response := resp.MessageResponse.(type) { case *reflectionv1.ServerReflectionResponse_ErrorResponse: + // This should never happen, however we do a bounds check to ensure we are doing a safe + // conversion from int32 (ErrorResponse.ErrorCode) to uint32 (connect.Code). + if response.ErrorResponse.ErrorCode < 0 { + return nil, fmt.Errorf("server replied with unsupported error code: %v", response.ErrorResponse.ErrorCode) + } return nil, connect.NewWireError(connect.Code(response.ErrorResponse.ErrorCode), errors.New(response.ErrorResponse.ErrorMessage)) case *reflectionv1.ServerReflectionResponse_FileDescriptorResponse: files := make([]*descriptorpb.FileDescriptorProto, len(response.FileDescriptorResponse.FileDescriptorProto)) diff --git a/private/bufpkg/bufimage/bufimage.go b/private/bufpkg/bufimage/bufimage.go index dc55ca53f4..6e00f22603 100644 --- a/private/bufpkg/bufimage/bufimage.go +++ b/private/bufpkg/bufimage/bufimage.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "io/fs" + "math" "slices" "sort" "strings" @@ -703,6 +704,11 @@ func reparseImageProto(protoImage *imagev1.Image, resolver protoencoding.Resolve } } if !isPublic { + // This should never happen, however we do a bounds check to ensure that we are + // doing a safe conversion for the index. + if i > math.MaxInt32 || i < math.MinInt32 { + return fmt.Errorf("unused dependency index out-of-bounds for int32 conversion: %v", i) + } bufExt.UnusedDependency = append(bufExt.UnusedDependency, int32(i)) } } diff --git a/private/bufpkg/bufimage/build_image.go b/private/bufpkg/bufimage/build_image.go index de32fac3b7..2b64ef0f51 100644 --- a/private/bufpkg/bufimage/build_image.go +++ b/private/bufpkg/bufimage/build_image.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "math" "strings" "github.com/bufbuild/buf/private/bufpkg/bufanalysis" @@ -306,6 +307,11 @@ func getImageFilesRec( dependency := fileDescriptor.Imports().Get(i).FileDescriptor if unusedDependencyFilenames != nil { if _, ok := unusedDependencyFilenames[dependency.Path()]; ok { + // This should never happen, however we do a bounds check to ensure that we are + // doing a safe conversion for the index. + if i > math.MaxInt32 || i < math.MinInt32 { + return nil, fmt.Errorf("unused dependency index out-of-bounds for int32 conversion: %v", i) + } unusedDependencyIndexes = append( unusedDependencyIndexes, int32(i), diff --git a/private/bufpkg/bufprotosource/option_extension_descriptor_test.go b/private/bufpkg/bufprotosource/option_extension_descriptor_test.go index 9501b54f4f..b83ecc52a1 100644 --- a/private/bufpkg/bufprotosource/option_extension_descriptor_test.go +++ b/private/bufpkg/bufprotosource/option_extension_descriptor_test.go @@ -15,6 +15,7 @@ package bufprotosource import ( + "math" "testing" "github.com/stretchr/testify/assert" @@ -121,10 +122,19 @@ func TestOptionExtensionLocation(t *testing.T) { func checkLocation(t *testing.T, loc Location, sourceCodeInfoLoc *descriptorpb.SourceCodeInfo_Location) { t.Helper() assert.Equal(t, sourceCodeInfoLoc.GetLeadingComments(), loc.LeadingComments()) + // Bounds assertions for int -> int32 conversion + assert.Less(t, loc.StartLine(), math.MaxInt32) + assert.Greater(t, loc.StartLine(), math.MinInt32) + assert.Less(t, loc.StartColumn(), math.MaxInt32) + assert.Greater(t, loc.StartLine(), math.MinInt32) span := []int32{int32(loc.StartLine() - 1), int32(loc.StartColumn() - 1)} if loc.EndLine() != loc.StartLine() { + assert.Less(t, loc.EndLine(), math.MaxInt32) + assert.Greater(t, loc.EndLine(), math.MinInt32) span = append(span, int32(loc.EndLine()-1)) } + assert.Less(t, loc.EndColumn(), math.MaxInt32) + assert.Greater(t, loc.EndColumn(), math.MinInt32) span = append(span, int32(loc.EndColumn()-1)) assert.Equal(t, sourceCodeInfoLoc.Span, span) } diff --git a/private/pkg/normalpath/normalpath_unix_test.go b/private/pkg/normalpath/normalpath_unix_test.go index c3576c6e78..d6084bdd9b 100644 --- a/private/pkg/normalpath/normalpath_unix_test.go +++ b/private/pkg/normalpath/normalpath_unix_test.go @@ -230,8 +230,8 @@ func TestStripComponents(t *testing.T) { testStripComponents(t, 5, "", false, "foo/bar/baz/bat") } -func testStripComponents(t *testing.T, count int, expected string, expectedOK bool, path string) { - actual, ok := StripComponents(path, uint32(count)) +func testStripComponents(t *testing.T, count uint32, expected string, expectedOK bool, path string) { + actual, ok := StripComponents(path, count) assert.Equal(t, expectedOK, ok) assert.Equal(t, expected, actual) }