Bug 1924532: Update gogo/protobuf to v1.3.2 #161

EmilienM · 2021-02-03T16:26:54Z

gogo/protobuf@v1.3.2 fixes https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3121

Ref: https://issues.redhat.com/browse/OCPBUGSM-24279

gogo/protobuf@v1.3.2 fixes https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3121 Ref: https://issues.redhat.com/browse/OCPBUGSM-24279 Signed-off-by: Emilien Macchi <emilien@redhat.com>

openshift-ci-robot · 2021-02-03T16:27:57Z

@EmilienM: This pull request references Bugzilla bug 1924532, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1924532: Update gogo/protobuf to v1.3.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

iamemilio · 2021-02-03T16:58:57Z

/lgtm

iamemilio · 2021-02-03T16:59:11Z

/approve
/hold until passing

openshift-ci-robot · 2021-02-03T16:59:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamemilio

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [iamemilio]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mdbooth · 2021-02-04T10:45:53Z

/hold

This is hopefully just precautionary as I'm honestly not sure what I'm looking at here. However, I have 2 concerns about this patch:

The revendor of gogo/protobuf doesn't seem to contain the CVE fix: gogo/protobuf@b03c65e
I'm not sure about the various dependent mod updates

Presumably (2) is just transitive updates from gogo/protobuf, but I'd like to convince myself of that. The first one seems problematic, though.

mdbooth · 2021-02-04T10:48:28Z

vendor/github.com/gogo/protobuf/proto/text_parser.go

 			return "", "", fmt.Errorf(`\%c%s is not a valid Unicode code point`, r, ss)
 		}
-		return string(i), s, nil
+		return string(rune(i)), s, nil


This doesn't seem to be the actual CVE fix: gogo/protobuf@b03c65e

Going to try to work out what's going on here, as the explicit request was to bump to v1.3.2, which you've done.

go mod vendor performs some treeshaking on a subpackage-basis.

The fix you linked refers to the file plugin/unmarshal/unmarshal.go which is not present in the vendor/github.com/gogo/protobuf folder. This means that the fix applies to some code that is not used in CAPO

Bumping the dependency prevents any further code change to introduce the vulnerable version of the affected plugin/unmarshal subpackage; however it can be deferred to 4.8 at this point.

What seems to be going on here is that the vendor copy doesn't contain the plugin/unmarshal/unmarshal.go at all, so doesn't contain the CVE fix. This discussion seems relevant: golang/go#26366 (comment) . If I'm reading that correctly, the go mod vendor is only pulling in files from the target which are actually used to build. If that's the case then presumably:

This patch is correctly updating the module version to 1.3.2

It turns out it's not critical as we're not touching the CVE code anyway

mdbooth · 2021-02-04T11:56:38Z

This patch correctly bumps gogo/protobuf to v1.3.2.

HOWEVER, because the CVE is in a code generator, this isn't how this problem needs to be addressed. We need to address the problem in every package which we vendor which generates code with protobuf. As per my original supposition that seems to be almost everything. Grepping for instances of generated vulnerable code reveals 51 instances:

$ git grep -l "if skippy < 0 {"
vendor/k8s.io/api/admission/v1/generated.pb.go
vendor/k8s.io/api/admission/v1beta1/generated.pb.go
vendor/k8s.io/api/admissionregistration/v1/generated.pb.go
vendor/k8s.io/api/admissionregistration/v1beta1/generated.pb.go
vendor/k8s.io/api/apps/v1/generated.pb.go
vendor/k8s.io/api/apps/v1beta1/generated.pb.go
vendor/k8s.io/api/apps/v1beta2/generated.pb.go
vendor/k8s.io/api/authentication/v1/generated.pb.go
vendor/k8s.io/api/authentication/v1beta1/generated.pb.go
vendor/k8s.io/api/authorization/v1/generated.pb.go
vendor/k8s.io/api/authorization/v1beta1/generated.pb.go
vendor/k8s.io/api/autoscaling/v1/generated.pb.go
vendor/k8s.io/api/autoscaling/v2beta1/generated.pb.go
vendor/k8s.io/api/autoscaling/v2beta2/generated.pb.go
vendor/k8s.io/api/batch/v1/generated.pb.go
vendor/k8s.io/api/batch/v1beta1/generated.pb.go
vendor/k8s.io/api/batch/v2alpha1/generated.pb.go
vendor/k8s.io/api/certificates/v1/generated.pb.go
vendor/k8s.io/api/certificates/v1beta1/generated.pb.go
vendor/k8s.io/api/coordination/v1/generated.pb.go
vendor/k8s.io/api/coordination/v1beta1/generated.pb.go
vendor/k8s.io/api/core/v1/generated.pb.go
vendor/k8s.io/api/discovery/v1alpha1/generated.pb.go
vendor/k8s.io/api/discovery/v1beta1/generated.pb.go
vendor/k8s.io/api/events/v1/generated.pb.go
vendor/k8s.io/api/events/v1beta1/generated.pb.go
vendor/k8s.io/api/extensions/v1beta1/generated.pb.go
vendor/k8s.io/api/flowcontrol/v1alpha1/generated.pb.go
vendor/k8s.io/api/imagepolicy/v1alpha1/generated.pb.go
vendor/k8s.io/api/networking/v1/generated.pb.go
vendor/k8s.io/api/networking/v1beta1/generated.pb.go
vendor/k8s.io/api/node/v1alpha1/generated.pb.go
vendor/k8s.io/api/node/v1beta1/generated.pb.go
vendor/k8s.io/api/policy/v1beta1/generated.pb.go
vendor/k8s.io/api/rbac/v1/generated.pb.go
vendor/k8s.io/api/rbac/v1alpha1/generated.pb.go
vendor/k8s.io/api/rbac/v1beta1/generated.pb.go
vendor/k8s.io/api/scheduling/v1/generated.pb.go
vendor/k8s.io/api/scheduling/v1alpha1/generated.pb.go
vendor/k8s.io/api/scheduling/v1beta1/generated.pb.go
vendor/k8s.io/api/settings/v1alpha1/generated.pb.go
vendor/k8s.io/api/storage/v1/generated.pb.go
vendor/k8s.io/api/storage/v1alpha1/generated.pb.go
vendor/k8s.io/api/storage/v1beta1/generated.pb.go
vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/generated.pb.go
vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.pb.go
vendor/k8s.io/apimachinery/pkg/api/resource/quantity_proto.go
vendor/k8s.io/apimachinery/pkg/apis/meta/v1/generated.pb.go
vendor/k8s.io/apimachinery/pkg/apis/meta/v1beta1/generated.pb.go
vendor/k8s.io/apimachinery/pkg/runtime/generated.pb.go
vendor/k8s.io/apimachinery/pkg/util/intstr/generated.pb.go

mandre

This needs a backport of kubernetes/kubernetes#98477 to release-1.20 and bumping the affected k8s modules.

openshift-bot · 2021-05-16T13:28:44Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-ci · 2021-05-16T13:28:51Z

@EmilienM: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-05-28T02:18:09Z

@EmilienM: This pull request references Bugzilla bug 1924532. The bug has been updated to no longer refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1924532: Update gogo/protobuf to v1.3.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This moves the OS_CLOUD data to a ConfigMap and then references that in the environment of the controller, instead of injecting OS_CLOUD directly into the environment variables. This fixes openshift#161.

Update gogo/protobuf to v1.3.2

af3cda4

gogo/protobuf@v1.3.2 fixes https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3121 Ref: https://issues.redhat.com/browse/OCPBUGSM-24279 Signed-off-by: Emilien Macchi <emilien@redhat.com>

openshift-ci-robot requested review from mandre and mdbooth February 3, 2021 16:27

EmilienM changed the title ~~Update gogo/protobuf to v1.3.2~~ Bug 1924532 - Update gogo/protobuf to v1.3.2 Feb 3, 2021

EmilienM changed the title ~~Bug 1924532 - Update gogo/protobuf to v1.3.2~~ Bug 1924532: Update gogo/protobuf to v1.3.2 Feb 3, 2021

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Feb 3, 2021

openshift-ci-robot assigned iamemilio Feb 3, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2021

mdbooth suggested changes Feb 4, 2021

View reviewed changes

mandre reviewed Feb 15, 2021

View reviewed changes

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 16, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2021

EmilienM closed this May 28, 2021

Bug 1924532: Update gogo/protobuf to v1.3.2 #161

Bug 1924532: Update gogo/protobuf to v1.3.2 #161

Uh oh!

Conversation

EmilienM commented Feb 3, 2021

Uh oh!

openshift-ci-robot commented Feb 3, 2021

Uh oh!

iamemilio commented Feb 3, 2021

Uh oh!

iamemilio commented Feb 3, 2021

Uh oh!

openshift-ci-robot commented Feb 3, 2021

Uh oh!

mdbooth commented Feb 4, 2021

Uh oh!

mdbooth Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

mdbooth Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Feb 4, 2021

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented May 16, 2021

Uh oh!

openshift-ci bot commented May 16, 2021

Uh oh!

openshift-ci bot commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants