New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A go-fuzz target for Serializer.Encode/Decode, tested with go-fuzz #83750

Merged

k8s-ci-robot merged 1 commit into kubernetes:master from mwwolters:json_fuzz

Oct 19, 2019

Contributor

mwwolters commented Oct 10, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
It fuzzes some serialization code to hopefully identify issues.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot added release-note-none kind/cleanup size/L cncf-cla: yes needs-sig needs-priority needs-ok-to-test labels

Contributor

k8s-ci-robot commented Oct 10, 2019

Hi @mwwolters. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot requested review from deads2k and derekwaynecarr

October 10, 2019 21:50

k8s-ci-robot added sig/api-machinery and removed needs-sig labels

Contributor

fedebongio commented Oct 14, 2019

/assign @caesarxuchao

k8s-ci-robot assigned caesarxuchao

Member

caesarxuchao commented Oct 14, 2019

/ok-to-test

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels

caesarxuchao reviewed

View reviewed changes

Member

caesarxuchao left a comment

Can you link to an issue to give more context on why we need this fuzzer? Thanks.

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json_fuzz.go Outdated Show resolved Hide resolved

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json_fuzz.go Outdated

    
              	"k8s.io/apimachinery/pkg/runtime/schema"

              )

              func FuzzStrictDecode(data []byte) int {

Member

caesarxuchao Oct 14, 2019

Who calls this function? Does the "build gofuzz" tag do some magic?

Member

mikedanese Oct 16, 2019

Hi Chao, the intent is that these will run under oss-fuzz continuously.

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json_fuzz.go Outdated Show resolved Hide resolved

Dor1s commented Oct 16, 2019

@mwwolters Kubernetes is now integrated with OSS-Fuzz, please make sure to add your fuzzer to https://github.com/google/oss-fuzz/blob/master/projects/kubernetes/build.sh after this is merged. Thanks a lot for working on this! //cc @mikedanese

mikedanese reviewed

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json_fuzz.go Outdated Show resolved Hide resolved

Member

mikedanese commented Oct 16, 2019

Sorry I missed this. Can you move this to test/fuzz?

mikedanese reviewed

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json_fuzz.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XXL area/apiserver area/cloudprovider area/conformance area/dependency area/e2e-test-framework area/ipvs and removed size/L labels

mikedanese added priority/important-soon and removed sig/scalability sig/scheduling sig/storage labels

k8s-ci-robot removed the needs-priority label

mikedanese reviewed

View reviewed changes

test/fuzz/json/json.go Outdated

    
              limitations under the License.

              */

              // TODO TODO

Member

mikedanese Oct 17, 2019

TODO TODO

Contributor Author

mwwolters Oct 17, 2019

Whoops.

mikedanese reviewed

View reviewed changes

test/fuzz/json/json.go Outdated Show resolved Hide resolved

mikedanese reviewed

View reviewed changes

test/fuzz/json/json.go Outdated Show resolved Hide resolved

mikedanese reviewed

View reviewed changes

test/fuzz/json/json.go Outdated Show resolved Hide resolved

mikedanese reviewed

View reviewed changes

test/fuzz/json/json.go Outdated

    
              	if err1 != nil {

              		panic("Can't encode decoded data")

              	}

              	if !bytes.Equal(b0.Bytes(), data) {

Member

mikedanese Oct 17, 2019

I wouldn't expect unknown fields to be preserved when using nonstrict.

Contributor Author

mwwolters Oct 17, 2019

Fair point. Removed but added some additional encode steps.

Member

mikedanese commented Oct 17, 2019

CI errors are real:

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/83750/pull-kubernetes-verify/1184881362683826177

fejta-bot commented Oct 17, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

mikedanese removed kind/api-change sig/windows labels

Member

mikedanese commented Oct 18, 2019

/approve
lgtm after the commits are squashed.

Contributor

k8s-ci-robot commented Oct 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, mwwolters

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [mikedanese]

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

k8s-ci-robot added the approved label


          Added go-fuzz target for json serializer Encode/Decode.

78c57cb

mwwolters force-pushed the json_fuzz branch from 6ca894b to 78c57cb Compare

October 18, 2019 16:13

Member

mikedanese commented Oct 18, 2019

/retest
/lgtm

k8s-ci-robot assigned mikedanese

k8s-ci-robot added the lgtm label

Member

mikedanese commented Oct 18, 2019

/retest

mwwolters mentioned this pull request

Added kubernetes json fuzz targets google/oss-fuzz#2967

Merged

Member

mikedanese commented Oct 19, 2019

/retest

k8s-ci-robot merged commit 0a6e5e6 into kubernetes:master

k8s-ci-robot added this to the v1.17 milestone

Contributor

k8s-ci-robot commented Oct 19, 2019

@mwwolters: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-conformance-image-test	d0ccd1ffc0000c53f45774a5f74a2e60dce80a51	link	`/test pull-kubernetes-conformance-image-test`
pull-kubernetes-e2e-gce	`78c57cb`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

cjcullen mentioned this pull request

CVE-2019-11254: kube-apiserver Denial of Service vulnerability from malicious YAML payloads #89535

Closed

pacoxu mentioned this pull request

clean fuzz json dir #105758

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved area/test cncf-cla: yes kind/cleanup lgtm ok-to-test priority/important-soon release-note-none sig/testing size/L