Update OSS-Fuzz integration to use Go native fuzzing #13473

nszetei · 2022-06-14T13:56:53Z

oss-fuzz integration with go native fuzzing.

cc @zmb3 @reedloden

api/types/fuzz_test.go

api/utils/aws/fuzz_test.go

api/types/fuzz_test.go

api/utils/aws/fuzz_test.go

zmb3 · 2022-06-14T14:07:09Z

fuzz/corpora/fuzz_parse_saml_in_response_to/saml_response_signed_assertion

@@ -0,0 +1,41 @@
+<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="_8e8dc5f69a98cc4c1ff3427e5ce34606fd672f91e6" Version="2.0" IssueInstant="2014-07-17T01:01:48Z" Destination="http://sp.example.com/demo1/index.php?acs" InResponseTo="ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685">


Out of curiosity, where did these files come from?

Since I am not sure about the license, I removed them. Thank you for asking, this should be resolved now.

zmb3 · 2022-06-14T14:08:34Z

fuzz/oss-fuzz-build.sh

+  # Fix /root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/[email protected]/fuzz.go:13:21:
+  # not enough arguments in call to Parse
+  rm /root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/ini@*/fuzz.go


What's going on here?

Is this somehow related to this: https://github.com/google/oss-fuzz/blob/master/projects/teleport/build.sh#L19 ?

go.mod we uses github.com/aws/aws-sdk-go-v2/internal/ini with the version 1.3.0.

This specific version uses an obsolete fuzzer https://github.com/aws/aws-sdk-go-v2/blob/v1.3.0/internal/ini/fuzz.go which was later in 1.16.5 removed here. The problem is that fuzzer calls

if _, err := Parse(b); err != nil {

but the signature in 1.3.0 is already different:

func Parse(f io.Reader, path string) (Sections, error) {

This does not trigger an error during normal builds, but I did some debugging and during the compilation of the target when using go-118-fuzz-build (dependency for oss-fuzz), it links against all files, including the useless fuzz.go:

$ strace -s 4096 -f -e execve go-118-fuzz-build -o $fuzzer.a -func $function github.com/gravitational/teleport/lib/services 2>&1 [ .. SNIP .. ] [pid 6349] execve("/root/.go/pkg/tool/linux_amd64/compile", ["/root/.go/pkg/tool/linux_amd64/compile", "-o", "/tmp/go-build1849907098/b451/_pkg_.a", "-trimpath", "/root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/[email protected]=>github.com/aws/aws-sdk-go-v2/internal/[email protected];/tmp/go-build1849907098/b451=>", "-p", "github.com/aws/aws-sdk-go-v2/internal/ini", "-lang=go1.15", "-complete", "-installsuffix", "shared", "-buildid", "_Y4HsbPwFpnPCDjtZue_/_Y4HsbPwFpnPCDjtZue_", "-goversion", "go1.18", "-shared", "-d=libfuzzer", "-nolocalimports", "-importcfg", "/tmp/go-build1849907098/b451/importcfg", "-pack", [ .. SNIP ..] "/root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/[email protected]/errors.go", "/root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/[email protected]/expression.go", "/root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/[email protected]/fuzz.go", "/root/go/pkg/mod/github.com/aws/aws-sdk-go-v2/internal/[email protected]/go_module_metadata.go", [ .. SNIP ..]

I know this is far from ideal, but if we keep this dependency version, I am not sure what else to do here than remove the file, since the other solutions involve modifying the already established compilation process.

zmb3 · 2022-06-14T14:09:00Z

fuzz/oss-fuzz-build.sh

+# compile_native_go_fuzzer $TELEPORT_PREFIX/lib/srv/db/mongodb/protocol \
+#   FuzzMongoRead fuzz_mongo_read


Why is this one commented out?

The FuzzMongoRead is already implemented here https://github.com/gravitational/teleport/blob/master/lib/srv/db/mongodb/protocol/fuzz_test.go, but it is currently not usable since there is an overflow bug that has to be fixed. I'll document this in the deliverable.

For the second function, pkcs7.Parse from mozilla panicked during fuzzing tests. Is it better to remove them from the build script, or leave it commented out here?

Did the issue with pkcs7.Parse get filed upstream?

@reedloden Yes, I saw this issue from February mozilla-services/pkcs7#67 and that's why I haven't filled a new one.

About the FuzzMongoRead, I described the issue to @jakule today and he mentioned that he would take a look.

From a quick look, mozilla-services/pkcs7#68 might fix it, though haven't looked that closely.

zmb3 · 2022-06-14T14:09:58Z

fuzz/oss-fuzz-build.sh

@@ -0,0 +1,144 @@
+#!/bin/bash -eu


Can we add a comment at the beginning here, or better yet a README in the fuzz directory that explains what these files are and how to use them?

README sounds better. However, can we wait until the deliverable report is ready? I suppose there will be more tests and adjustments after the deployment and I wanted to avoid documenting something in the semi-final form.

zmb3 · 2022-06-14T14:10:41Z

fuzz/oss-fuzz-build.sh

+  go get github.com/AdamKorcz/go-118-fuzz-build/utils
+  go get k8s.io/client-go/tools/[email protected]
+  go get github.com/ThalesIgnite/[email protected]


Could you explain what's going on here? I'm a little hesitant about pinning to specific versions of things here - how will we know when to update these? Why are they necessary?

The first package is required by go oss-fuzz. I am not happy to include the specific versions too, k8s.io/client-go is needed for several fuzzers and even when for the go test -fuzz the dependencies are resolved automatically, oss-fuzz (that is based on the libfuzzer engine) reports these errors during my test:

replacing *testing.F /root/go/pkg/mod/github.com/!thales!ignite/[email protected]/aead.go:29:2: missing go.sum entry for module providing package github.com/miekg/pkcs11 (imported by github.com/ThalesIgnite/crypto11); to add: go get github.com/ThalesIgnite/[email protected]

I was not able to find a better solution than including this minimal set of packages with specific versions.

Since #13473 (comment) is related, I'll answer here, I noticed the similar workaround here https://github.com/google/oss-fuzz/blob/master/projects/teleport/build.sh with the fuzz.go removal.

I can try to debug it again tomorrow to understand why this is happening and come up with a better solution. Also, if you have any suggestions on why this occurs, let me please know.

Update, to reproduce this, I cloned the teleport and did go get github.com/AdamKorcz/go-118-fuzz-build/utils (needed by oss-fuzz), then also the make fails.

go get upgrades some dependencies, including these below:

- github.com/imdario/mergo v0.3.5 // indirect + github.com/imdario/mergo v0.3.12 // indirect - github.com/miekg/pkcs11 v1.0.3-0.20190429190417-a667d056470f // indirect + github.com/miekg/pkcs11 v1.0.3 // indirect

I did not find a solution to avoid the upgrade, but what I can do is to update all dependencies at once, e.g., this works:

go get github.com/AdamKorcz/go-118-fuzz-build/utils go get -u all || true go mod tidy go get github.com/AdamKorcz/go-118-fuzz-build/utils

If you want to ask me about the true statement, it's because I prefer to run the shell script with the parameter to stop when error occurs and go get -u all at the end yields these, which is indeed evaluated by bash as error:

go: all: module github.com/Azure/azure-sdk-for-go/sdk/azcore@upgrade found (v1.1.0), but does not contain package github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pipeline go: all: module github.com/chai2010/gettext-go@upgrade found (v1.0.2), but does not contain package github.com/chai2010/gettext-go/gettext go: all: module github.com/chai2010/gettext-go@upgrade found (v1.0.2), but does not contain package github.com/chai2010/gettext-go/gettext/mo go: all: module github.com/chai2010/gettext-go@upgrade found (v1.0.2), but does not contain package github.com/chai2010/gettext-go/gettext/plural go: all: module github.com/chai2010/gettext-go@upgrade found (v1.0.2), but does not contain package github.com/chai2010/gettext-go/gettext/po go: all: module k8s.io/client-go@upgrade found (v0.24.1), but does not contain package k8s.io/client-go/pkg/apis/clientauthentication/v1alpha1 go: all: module sigs.k8s.io/kustomize/api@upgrade found (v0.11.5), but does not contain package sigs.k8s.io/kustomize/api/filters/prefixsuffix

Could this be improved, or can we leave it this way atm?

We can leave it for now, but it's a little concerning that the fuzzers will be running with different versions of the dependencies than what is actually used in the product and defined in go.mod.

lib/auth/fuzz_test.go

Co-authored-by: Zac Bergquist <[email protected]>

api/types/fuzz_test.go

jakule · 2022-06-14T17:24:34Z

@nszetei

Why do we need the fuzz/oss-fuzz-build.sh ? Is this is somehow related to how oss-fuzz works? With Go 1.18 technically you just need to run go test -fuzz to run fuzz tests.
Do we have any plans to run the "problematic" input as a part of our unit test suite? I know that after you add input with f.Add() then go test run fuzz tests as a normal unit test.
Why we never set any corpus with f.Add()? Providing some initial values should increase the effectiveness of fuzzing.
I don't see any other validation rather than require.NotPanics(). I understand that in some cases it's hard to write a different assertion, but I don't think this is the rule. For example for serialize/deserialize type function we should be able to check if a input is the same after serialization/deserialization if no error was returned in the middle.

nszetei · 2022-06-15T08:49:01Z

Hi @jakule, thanks for your valuable input. I added some references since Go Native with oss-fuzz is relatively new, with some stuff documented only in GitHub issues or the source code.

Why do we need the fuzz/oss-fuzz-build.sh ? Is this is somehow related to how oss-fuzz works? With Go 1.18 technically you just need to run go test -fuzz to run fuzz tests.

Yes, exactly for this reason. It's the standard way how to do it and it is described in more detail here, although it does not reveal why.

I did some digging and oss-fuzz docker go image internally uses go114-fuzz-build to leverage the native libfuzzer instrumentation (required to use oss-fuzz). You can't have it with go test -fuzz, although there is a proposal for implementing it for golang.

Do we have any plans to run the "problematic" input as a part of our unit test suite? I know that after you add input with f.Add() then go test run fuzz tests as a normal unit test.

That's a great idea. After we are done with the integration, we will provide all necessary documentation and mention this possibility as a recommendation when running go test -fuzz without oss-fuzz.

Why we never set any corpus with f.Add()? Providing some initial values should increase the effectiveness of fuzzing.

Because f.Add() is not supported by oss-fuzz and we are primarily focusing on this integration. This is even worse, because oss-fuzz uses a different corpus than the one used when you invoke go test -fuzz. In case you have only one file, for oss-fuzz it's enough to pack the corpus files into one zip file and name it the same way as the fuzzer. So what I did is that I stored the testcases inside the fuzz/corpora directory. Then the build script just processes these files and creates the archives.

We can change the solution in the future after they start supporting the same formats, just currently, the options are very limited. I was playing with corpus2ossfuzz which supports only bytes (extending to other datatypes is trivial), but since the 255 characters limitation, I would rather stick with the solution I proposed (pure files + zipping them).

I don't see any other validation rather than require.NotPanics(). I understand that in some cases it's hard to write a different assertion, but I don't think this is the rule. For example for serialize/deserialize type function we should be able to check if a input is the same after serialization/deserialization if no error was returned in the middle.

Can you please point me to some functions or tests suitable for implementing this? Something that does not require running the teleport or keeping the state, and it's not part of the golang or managed by google, since these are already fuzzed enough. Thank you.

zmb3 · 2022-06-17T14:19:16Z

/gcbrun

zmb3 · 2022-06-17T15:02:15Z

/gcbrun

zmb3 · 2022-06-17T15:48:00Z

/gcbrun

zmb3 · 2022-06-17T16:21:32Z

/gcbrun

zmb3 · 2022-06-17T17:04:20Z

/gcbrun

zmb3 · 2022-06-18T15:06:12Z

I'm going to abandon this one and open my own PR with these changes. GCB gets weird sometimes with PRs from forks, and I'm tired of trying to make it work.

zmb3 · 2022-06-18T15:33:25Z

/gcbrun

zmb3 · 2022-06-18T18:10:23Z

/gcbrun

zmb3 · 2022-06-18T21:04:27Z

/gcbrun

zmb3 · 2022-06-18T21:28:43Z

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉 finally.

rosstimothy · 2022-06-22T20:57:11Z

I'm thinking this should have also included a version bump in go.mod

teleport/go.mod

Line 3 in 0ab9716

go 1.17

Technically you still only need go 1.17 to build, however you can't run the tests now without getting errors because testing.F wasn't introduced until go 1.18

$ make test-go 
lib/auth/fuzz_test.go:27:35: undefined: testing.F
lib/auth/fuzz_test.go:38:31: undefined: testing.F

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod. Also, run `go mod tidy`.

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

* [v10] Bump go.mod to use Go 1.18 We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod. * go mod tidy api/go.mod * forgot my /e/

oss fuzz integration

600b085

github-actions bot added database-access Database access related issues and PRs kubernetes labels Jun 14, 2022

github-actions bot requested review from r0mant and zmb3 June 14, 2022 13:57

zmb3 reviewed Jun 14, 2022

View reviewed changes

Update api/types/fuzz_test.go

ee600c5

Co-authored-by: Zac Bergquist <[email protected]>

jakule reviewed Jun 14, 2022

View reviewed changes

api/types/fuzz_test.go Show resolved Hide resolved

reedloden added the security Security Issues label Jun 14, 2022

nszetei and others added 5 commits June 15, 2022 14:51

fixes

26d2886

do not fail if the file is missing

ef0eb97

missing go-118-fuzz-build fix

1ff4867

missing go-118-fuzz-build fix (2)

394ef20

Merge branch 'master' into oss-fuzz-integration-doyensec

750b3af

zmb3 approved these changes Jun 17, 2022

View reviewed changes

jimbishopp approved these changes Jun 17, 2022

View reviewed changes

zmb3 enabled auto-merge (squash) June 17, 2022 15:02

zmb3 added 2 commits June 17, 2022 09:08

Merge branch 'master' into oss-fuzz-integration-doyensec

9c786e7

Merge branch 'master' into oss-fuzz-integration-doyensec

e8eaa8c

reedloden changed the title ~~oss fuzz integration~~ Update OSS-Fuzz integration to use Go native fuzzing Jun 17, 2022

Merge branch 'master' into oss-fuzz-integration-doyensec

8bcbf7f

Merge branch 'master' into oss-fuzz-integration-doyensec

bee4391

Merge branch 'master' into oss-fuzz-integration-doyensec

0bf1806

come on GCB 3

bef79bd

Merge branch 'master' into oss-fuzz-integration-doyensec

1330100

come on GCB 4

a2fe1d9

zmb3 merged commit 0ab9716 into gravitational:master Jun 18, 2022

This was referenced Jun 22, 2022

Update to Go 1.18 #12578

Merged

Bump go.mod to use Go 1.18 #13771

Merged

reedloden added a commit that referenced this pull request Jun 24, 2022

Bump go.mod to use Go 1.18

1770160

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod. Also, run `go mod tidy`.

reedloden added a commit that referenced this pull request Jun 29, 2022

Bump go.mod to use Go 1.18

0dd5a99

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod. Also, run `go mod tidy`.

reedloden added a commit that referenced this pull request Jun 29, 2022

Bump go.mod to use Go 1.18 (#13771)

83eb9b2

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

reedloden added a commit that referenced this pull request Jun 29, 2022

[v10] Bump go.mod to use Go 1.18

6160fa2

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

reedloden mentioned this pull request Jun 29, 2022

[v10] Bump go.mod to use Go 1.18 #13958

Closed

reedloden added a commit that referenced this pull request Jul 1, 2022

[v10] Bump go.mod to use Go 1.18

063c49a

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

lxea pushed a commit that referenced this pull request Sep 2, 2022

[v10] Bump go.mod to use Go 1.18

c9ff926

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

lxea pushed a commit that referenced this pull request Sep 5, 2022

[v10] Bump go.mod to use Go 1.18

eb9c39d

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

lxea pushed a commit that referenced this pull request Sep 8, 2022

[v10] Bump go.mod to use Go 1.18

32c6b7f

We're now using Go 1.18 features (native fuzzing in #13473), which means we need to accurately state that we require Go 1.18 in our go.mod.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update OSS-Fuzz integration to use Go native fuzzing #13473

Update OSS-Fuzz integration to use Go native fuzzing #13473

nszetei commented Jun 14, 2022

zmb3 Jun 14, 2022

nszetei Jun 15, 2022

zmb3 Jun 14, 2022

jakule Jun 14, 2022

nszetei Jun 15, 2022

zmb3 Jun 14, 2022

nszetei Jun 14, 2022

reedloden Jun 17, 2022

nszetei Jun 17, 2022

reedloden Jun 17, 2022

zmb3 Jun 14, 2022

nszetei Jun 15, 2022

zmb3 Jun 14, 2022

nszetei Jun 14, 2022 •

edited

Loading

nszetei Jun 15, 2022 •

edited

Loading

zmb3 Jun 17, 2022

jakule commented Jun 14, 2022

nszetei commented Jun 15, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

rosstimothy commented Jun 22, 2022 •

edited

Loading

		@@ -0,0 +1,41 @@
		<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="_8e8dc5f69a98cc4c1ff3427e5ce34606fd672f91e6" Version="2.0" IssueInstant="2014-07-17T01:01:48Z" Destination="http://sp.example.com/demo1/index.php?acs" InResponseTo="ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685">

		# compile_native_go_fuzzer $TELEPORT_PREFIX/lib/srv/db/mongodb/protocol \
		# FuzzMongoRead fuzz_mongo_read

Update OSS-Fuzz integration to use Go native fuzzing #13473

Update OSS-Fuzz integration to use Go native fuzzing #13473

Conversation

nszetei commented Jun 14, 2022

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

nszetei Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

nszetei Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakule commented Jun 14, 2022

nszetei commented Jun 15, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 17, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

zmb3 commented Jun 18, 2022

rosstimothy commented Jun 22, 2022 • edited Loading

nszetei Jun 14, 2022 •

edited

Loading

nszetei Jun 15, 2022 •

edited

Loading

rosstimothy commented Jun 22, 2022 •

edited

Loading