diff options
| -rw-r--r-- | .github/workflows/test.yml | 112 | ||||
| -rw-r--r-- | disco/disco_fuzz_test.go | 95 | ||||
| -rw-r--r-- | disco/disco_fuzzer.go | 17 | ||||
| -rw-r--r-- | net/stun/stun_fuzz_test.go | 14 | ||||
| -rw-r--r-- | net/stun/stun_fuzzer.go | 12 |
5 files changed, 157 insertions, 93 deletions
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 60d2066de..e8f663745 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -2,20 +2,6 @@ # both PRs and merged commits, and for the latter reports failures to slack. name: CI -env: - # Our fuzz job, powered by OSS-Fuzz, fails periodically because we upgrade to - # new Go versions very eagerly. OSS-Fuzz is a little more conservative, and - # ends up being unable to compile our code. - # - # When this happens, we want to disable the fuzz target until OSS-Fuzz catches - # up. However, we also don't want to forget to turn it back on when OSS-Fuzz - # can once again build our code. - # - # This variable toggles the fuzz job between two modes: - # - false: we expect fuzzing to be happy, and should report failure if it's not. - # - true: we expect fuzzing is broken, and should report failure if it start working. - TS_FUZZ_CURRENTLY_BROKEN: false - on: push: branches: @@ -296,63 +282,61 @@ jobs: fuzz: - # This target periodically breaks (see TS_FUZZ_CURRENTLY_BROKEN at the top - # of the file), so it's more complex than usual: the 'build fuzzers' step - # might fail, and depending on the value of 'TS_FUZZ_CURRENTLY_BROKEN', that - # might or might not be fine. The steps after the build figure out whether - # the success/failure is expected, and appropriately pass/fail the job - # overall accordingly. - # - # Practically, this means that all steps after 'build fuzzers' must have an - # explicit 'if' condition, because the default condition for steps is - # 'success()', meaning "only run this if no previous steps failed". - if: github.event_name == 'pull_request' runs-on: ubuntu-22.04 + strategy: + matrix: + include: + - pkg: ./disco + test: FuzzDisco + - pkg: ./net/dns/resolver + test: FuzzClampEDNSSize + - pkg: ./net/stun + test: FuzzStun + - pkg: ./util/deephash + test: FuzzTime + - pkg: ./util/deephash + test: FuzzAddr + - pkg: ./util/hashx + test: Fuzz + steps: - - name: build fuzzers - id: build - uses: google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@master - # continue-on-error makes steps.build.conclusion be 'success' even if - # steps.build.outcome is 'failure'. This means this step does not - # contribute to the job's overall pass/fail evaluation. - continue-on-error: true + - name: checkout + uses: actions/checkout@v3 + - name: Restore Build Cache + uses: actions/cache@v3 with: - oss-fuzz-project-name: 'tailscale' - dry-run: false - language: go - - name: report unexpectedly broken fuzz build - if: steps.build.outcome == 'failure' && env.TS_FUZZ_CURRENTLY_BROKEN != 'true' - run: | - echo "fuzzer build failed, see above for why" - echo "if the failure is due to OSS-Fuzz not being on the latest Go yet," - echo "set TS_FUZZ_CURRENTLY_BROKEN=true in .github/workflows/test.yml" - echo "to temporarily disable fuzzing until OSS-Fuzz works again." - exit 1 - - name: report unexpectedly working fuzz build - if: steps.build.outcome == 'success' && env.TS_FUZZ_CURRENTLY_BROKEN == 'true' - run: | - echo "fuzzer build succeeded, but we expect it to be broken" - echo "please set TS_FUZZ_CURRENTLY_BROKEN=false in .github/workflows/test.yml" - echo "to reenable fuzz testing" - exit 1 - - name: run fuzzers - id: run - # Run the fuzzers whenever they're able to build, even if we're going to - # report a failure because TS_FUZZ_CURRENTLY_BROKEN is set to the wrong - # value. - if: steps.build.outcome == 'success' - uses: google/oss-fuzz/infra/cifuzz/actions/run_fuzzers@master + # Note: unlike the other setups, this is only grabbing the mod download + # cache, rather than the whole mod directory, as the download cache + # contains zips that can be unpacked in parallel faster than they can be + # fetched and extracted by tar + path: | + ~/.cache/go-build + ~/go/pkg/mod/cache + ~\AppData\Local\go-build + # The -2- here should be incremented when the scheme of data to be + # cached changes (e.g. path above changes). + key: ${{ github.job }}-${{ runner.os }}-go-2-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ github.job }}-${{ runner.os }}-go-2- + - name: Restore fuzz corpus cache + uses: actions/cache@v3 with: - oss-fuzz-project-name: 'tailscale' - fuzz-seconds: 300 - dry-run: false - language: go + path: | + ~/.cache/go-build/fuzz + # Uses an incrementing key to constantly collide and upload, but this + # cache action is kept separate from any build cache action. + key: fuzz-${{ matrix.pkg }}-${{ matrix.test }}-${{ github.run_id }} + restore-keys: | + fuzz-${{ matrix.pkg }}-${{ matrix.test }}- + - name: fuzz ${{matrix.pkg}} ${{matrix.test}} + id: run + run: ./tool/go test -fuzz=${{ matrix.test }} -fuzztime=60s ${{ matrix.pkg }} - name: upload crash uses: actions/upload-artifact@v3 - if: steps.run.outcome != 'success' && steps.build.outcome == 'success' + if: steps.run.outcome != 'success' with: - name: artifacts - path: ./out/artifacts + name: testdata + path: ./**/testdata depaware: runs-on: ubuntu-22.04 diff --git a/disco/disco_fuzz_test.go b/disco/disco_fuzz_test.go new file mode 100644 index 000000000..105897fa3 --- /dev/null +++ b/disco/disco_fuzz_test.go @@ -0,0 +1,95 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package disco + +import ( + "bytes" + "reflect" + "testing" + + "golang.org/x/exp/slices" +) + +func FuzzDisco(f *testing.F) { + f.Fuzz(func(t *testing.T, data1 []byte) { + if data1 == nil { + return + } + data2 := make([]byte, 0, len(data1)) + + m1, e1 := Parse(data1) + if m1 == nil || reflect.ValueOf(m1).IsNil() { + if e1 == nil { + t.Fatal("nil message and nil error!") + } + t.Logf("message result is actually nil, can't be serialized again") + return + } + + data2 = m1.AppendMarshal(data2) + m2, e2 := Parse(data2) + if m2 == nil || reflect.ValueOf(m2).IsNil() { + if e2 == nil { + t.Fatal("nil message and nil error!") + } + t.Errorf("second message result is actually nil!") + } + + t.Logf("m1: %#v", m1) + t.Logf("m2: %#v", m1) + t.Logf("data1:\n%x", data1) + t.Logf("data2:\n%x", data2) + + if e1 != nil && e2 != nil { + if e1.Error() != e2.Error() { + t.Errorf("error mismatch: %v != %v", e1, e2) + } + return + } + + // Explicitly ignore the case where the fuzzer made a different version + // byte, it's not interesting. + data1[1] = v0 + // The protocol doesn't have a length at this layer, and so it will + // ignore meaningless trailing data such as a key that is more than 0 + // bytes, but less than keylen bytes. + if len(data2) < len(data1) { + data1 = data1[:len(data2)] + } + + if !bytes.Equal(data1, data2) { + t.Errorf("data mismatch:\n%x\n%x", data1, data2) + } + + switch t1 := m1.(type) { + case *Ping: + t2, ok := m2.(*Ping) + if !ok { + t.Errorf("m1 and m2 are not the same type") + } + if *t1 != *t2 { + t.Errorf("m1 and m2 are not the same") + } + case *Pong: + t2, ok := m2.(*Pong) + if !ok { + t.Errorf("m1 and m2 are not the same type") + } + if *t1 != *t2 { + t.Errorf("m1 and m2 are not the same") + } + case *CallMeMaybe: + t2, ok := m2.(*CallMeMaybe) + if !ok { + t.Errorf("m1 and m2 are not the same type") + } + + if !slices.Equal(t1.MyNumber, t2.MyNumber) { + t.Errorf("m1 and m2 are not the same") + } + default: + t.Fatalf("unknown message type %T", m1) + } + }) +} diff --git a/disco/disco_fuzzer.go b/disco/disco_fuzzer.go deleted file mode 100644 index b9ffabfb0..000000000 --- a/disco/disco_fuzzer.go +++ /dev/null @@ -1,17 +0,0 @@ -// Copyright (c) Tailscale Inc & AUTHORS -// SPDX-License-Identifier: BSD-3-Clause -//go:build gofuzz - -package disco - -func Fuzz(data []byte) int { - m, _ := Parse(data) - - newBytes := m.AppendMarshal(data) - parsedMarshall, _ := Parse(newBytes) - - if m != parsedMarshall { - panic("Parsing error") - } - return 1 -} diff --git a/net/stun/stun_fuzz_test.go b/net/stun/stun_fuzz_test.go new file mode 100644 index 000000000..1daf03521 --- /dev/null +++ b/net/stun/stun_fuzz_test.go @@ -0,0 +1,14 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package stun + +import "testing" + +func FuzzStun(f *testing.F) { + f.Fuzz(func(t *testing.T, data []byte) { + _, _, _ = ParseResponse(data) + + _, _ = ParseBindingRequest(data) + }) +} diff --git a/net/stun/stun_fuzzer.go b/net/stun/stun_fuzzer.go deleted file mode 100644 index 6f0c9e3b0..000000000 --- a/net/stun/stun_fuzzer.go +++ /dev/null @@ -1,12 +0,0 @@ -// Copyright (c) Tailscale Inc & AUTHORS -// SPDX-License-Identifier: BSD-3-Clause -//go:build gofuzz - -package stun - -func FuzzStunParser(data []byte) int { - _, _, _ = ParseResponse(data) - - _, _ = ParseBindingRequest(data) - return 1 -} |