Skip to content
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.

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

Feature: add Scorecard to OSS-Fuzz and CIFuzz #1389

Open
azeemshaikh38 opened this issue Dec 13, 2021 · 19 comments
Open

Feature: add Scorecard to OSS-Fuzz and CIFuzz #1389

azeemshaikh38 opened this issue Dec 13, 2021 · 19 comments
Assignees

Comments

@azeemshaikh38
Copy link
Contributor

azeemshaikh38 commented Dec 13, 2021

Improve Scorecard's Fuzzing score by adding it to OSS-Fuzz and enabling CIFuzz GH Action.

@jonathanmetzman fyi.

@naveensrinivasan
Copy link
Member

Which API do you want to fuzz within scorecard?

If it is just to get the checkmark for fuzzing then it doesn't add value.

Probably good for discussion.

@evverx
Copy link
Contributor

evverx commented Dec 14, 2021

FWIW Looking at #1256 (comment), I think it would make sense to cover h2non/filetype at least (assuming scorecard will keep using it for parsing untrusted data).

@evverx
Copy link
Contributor

evverx commented Dec 14, 2021

As far as I know scorecard also parses random bash scripts with https://github.com/mvdan/sh so ideally it should probably be covered as well (though I agree technically it doesn't improve the fuzzing score of scorecard itself)

@naveensrinivasan
Copy link
Member

As far as I know scorecard also parses random bash scripts with https://github.com/mvdan/sh so ideally it should probably be covered as well (though I agree technically it doesn't improve the fuzzing score of scorecard itself)

AFAIK the https://github.com/mvdan/sh is fuzzed probably randomly not using oss-fuzz.

@naveensrinivasan
Copy link
Member

FWIW Looking at #1256 (comment), I think it would make sense to cover h2non/filetype at least (assuming scorecard will keep using it for parsing untrusted data).

I agree,I think it would be better to include the fuzzing that repository instead of Scorecard.

Thanks for bringing it up.

It makes sense.

@evverx
Copy link
Contributor

evverx commented Dec 15, 2021

scorecard has its own file parser located in https://github.com/ossf/scorecard/tree/main/checks/fileparser though (which I think is a good candidate for fuzzing).

AFAIK the https://github.com/mvdan/sh is fuzzed probably randomly not using oss-fuzz

Good to know. Thanks! But given that finding and fixing bugs found by fuzz targets is like playing whack-a-mole if it isn't fuzzed continuous it's probably safe to say it isn't fuzzed.

@naveensrinivasan
Copy link
Member

scorecard has its own file parser located in https://github.com/ossf/scorecard/tree/main/checks/fileparser though (which I think is a good candidate for fuzzing).

AFAIK the https://github.com/mvdan/sh is fuzzed probably randomly not using oss-fuzz

Good to know. Thanks! But given that finding and fixing bugs found by fuzz targets is like playing whack-a-mole if it isn't fuzzed continuous it's probably safe to say it isn't fuzzed.

I agree. It's a valid use case.

@naveensrinivasan
Copy link
Member

I wrote an issue on that repo mvdan/sh#777

@azeemshaikh38
Copy link
Contributor Author

IMO, we should start with a basic framework which allows us to start fuzzing. Over time we can keep identifying and adding more APIs as we see fit. The recent nil-ptr crashes during file parsing are the reason I think this will be useful. Even though the parsing libraries we use might be fuzzed, the crashes were due to our logic.

@naveensrinivasan
Copy link
Member

Looks like the oss-fuzz will support native fuzzing which is a lot easier with the 1.18 release. We should wait until then. google/oss-fuzz#7020

@evverx
Copy link
Contributor

evverx commented Dec 15, 2021

FWIW considering that scorecard isn't on OSS-Fuzz yet and its testing infrastructure doesn't rely on public OSS-Fuzz corpora I think it would be better to integrate it with https://github.com/google/clusterfuzzlite/ in the sense that as long as steps requiring external storage like corpus pruning are behind something like if github.repository == ... it's compatible with forks, which in turn makes it easier for external contributors to test their patches before sending them. Another plus is that CFLite can be kind of pinned and doesn't drag the "Pinned-Dependencies" score down with it. (Though judging by something that looks like release notes it seems it should be possible to pin it eventually as well).

@naveensrinivasan
Copy link
Member

We could look at this when we migrate to go 1.18 especially because it is going to be builtin and is going to be supported by oss-fuzz google/oss-fuzz#7020

@azeemshaikh38
Copy link
Contributor Author

@justaugustus assigning this to you track progress. Part of #1618.

@naveensrinivasan
Copy link
Member

This is creating crashes google/oss-fuzz#7269

@evverx
Copy link
Contributor

evverx commented Feb 13, 2022

FWIW since the fuzz target is public I don't think scorecard bug reports should be private because they can be easily reproduced. For example, I don't have access to bug reports but I can trigger all the crashes like:

$ ENABLE_SARIF= ./scorecard --local ../systemd/ --format sarif --policy test/crash-381edcd3ad75663c10f0d7fc3ea8daf7709267c7
panic: internal error: attempted to parse unknown event (please report): none [recovered]
	panic: internal error: attempted to parse unknown event (please report): none

goroutine 1 [running]:
gopkg.in/yaml%2ev3.handleErr(0xc00025f5a8)
	gopkg.in/[email protected]/yaml.go:294 +0x6d
panic({0xcc8240, 0xc000044cb0})
	runtime/panic.go:1038 +0x215
gopkg.in/yaml%2ev3.(*parser).parse(0xc000059000)
	gopkg.in/[email protected]/decode.go:163 +0x194
gopkg.in/yaml%2ev3.(*parser).parseChild(...)
	gopkg.in/[email protected]/decode.go:194
gopkg.in/yaml%2ev3.(*parser).document(0xc000059000)
	gopkg.in/[email protected]/decode.go:203 +0x7d
gopkg.in/yaml%2ev3.(*parser).parse(0xc000059000)
	gopkg.in/[email protected]/decode.go:156 +0xab
gopkg.in/yaml%2ev3.unmarshal({0xc00041e400, 0x8, 0x200}, {0xc97fc0, 0xc000044ca0}, 0x14)
	gopkg.in/[email protected]/yaml.go:161 +0x306
gopkg.in/yaml%2ev3.Unmarshal(...)
	gopkg.in/[email protected]/yaml.go:89
github.com/ossf/scorecard/v4/policy.ParseFromYAML({0xc00041e400, 0x8, 0x200})
	github.com/ossf/scorecard/v4/policy/policy.go:73 +0x128
github.com/ossf/scorecard/v4/cmd.readPolicy()
	github.com/ossf/scorecard/v4/cmd/root.go:283 +0xa5
github.com/ossf/scorecard/v4/cmd.scorecardCmd(0x1675cc0, {0xe795af, 0x6, 0x6})
	github.com/ossf/scorecard/v4/cmd/root.go:124 +0x150
github.com/spf13/cobra.(*Command).execute(0x1675cc0, {0xc0000b4010, 0x6, 0x6})
	github.com/spf13/[email protected]/command.go:860 +0x5f8
github.com/spf13/cobra.(*Command).ExecuteC(0x1675cc0)
	github.com/spf13/[email protected]/command.go:974 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/[email protected]/command.go:902
github.com/ossf/scorecard/v4/cmd.Execute()
	github.com/ossf/scorecard/v4/cmd/root.go:104 +0x25
main.main()
	github.com/ossf/scorecard/v4/main.go:21 +0x17

@naveensrinivasan looking at https://github.com/google/oss-fuzz/pull/7269/files it seems a seed corpus wasn't added there. I think it would probably make sense to address that in #1631. Seed corpora are briefly mentioned at https://google.github.io/oss-fuzz/advanced-topics/ideal-integration/#seed-corpus

@naveensrinivasan
Copy link
Member

naveensrinivasan commented Feb 14, 2022

There are multiple crashes, I have gotten a few emails about this from oss-fuzz.

There need to be multiple things that have to be done before Scorecard is integrated into oss-fuzz

  • Have a separate module within Scorecard to have a dependency with go-fuzz and go-tip/go 1.18 beta 2 for building and testing fuzz
  • Choose the target func to test
  • Integrated into GitHub CI for ensuring the code compiles
  • Document of how to run/test Fuzz.
  • Build the initial corpus, test the target and fix crashes in Scorecard
  • Integrate with CIFuzz
  • Submit a patch to oss-fuzz after these above steps with corpus.

The reason behind the above steps was based on me integrating sigstore into oss-fuzz https://github.com/sigstore/sigstore/tree/main/test/fuzz

I think this is the order of things that have been done to avoid getting multiple crashes.

This is some amount of work. We have to decide to either fix this or another option is to remove google/oss-fuzz#7269 this till we get to do it from our end. The default disclosure policy is 90 days for oss-fuzz.

Unless we prioritize to work on this.

@justaugustus @azeemshaikh38 @laurentsimon @inferno-chromium Thoughts?

@evverx
Copy link
Contributor

evverx commented Feb 14, 2022

The default disclosure policy is 90 days for oss-fuzz

FWIW I don't think that's what really happens in practice. Once fuzz targets are public it's safe to assume that bugs they can discover are (kind of) publicly known (because they are actually run outside of OSS-Fuzz).

I completely agree with @naveensrinivasan on how scorecard should be integrated.

@naveensrinivasan
Copy link
Member

The default disclosure policy is 90 days for oss-fuzz

FWIW I don't think that's what really happens in practice. Once fuzz targets are public it's safe to assume that bugs they can discover are (kind of) publicly known (because they are actually run outside of OSS-Fuzz).

I completely agree with @naveensrinivasan on how scorecard should be integrated.

👍

Copy link

github-actions bot commented Nov 3, 2023

This issue is stale because it has been open for 60 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

4 participants