CIFuzz and GitHub code scanning alerts

[jonathanmetzman]

Recently I've added support for Github code scanning alerts in CIFuzz/ClusterFuzzLite. Would any CIFuzz/CFL users like to volunteer to be the first to use this? All you would need to do is make a small change to your workflow files, in return, bugs found by CIFuzz/ClusterFuzzLite will be reported using a better UI.

Thanks!

CC @alex @evverx @rouault

[alex]

Sure -- though I'm not sure we've had a finding so far (yay memory safe languages).

https://github.com/alex/rust-asn1/tree/main/.github/workflows is where the workflows are.

[evverx]

@jonathanmetzman GitHub recently introduced a code scanning feature with weird side effects: https://github.com/redhat-plumbers-in-action/differential-shellcheck/issues/215 so I don't think I can add it to CFLite workflows that aren't triggered on push events (because they are run against several stable branches) but the other CIFuzz/CFLite workflows should probably be fine. I wonder if there is a test repo somewhere where it should be possible to take a look at what those new CIFuzz/CFLite alerts look like?

[evverx]

the other CIFuzz/CFLite workflows should probably be fine

On second thought I think depending on how exactly CIFuzz generates SARIF it can be affected by https://github.blog/changelog/2023-03-17-code-scanning-shows-more-accurate-and-relevant-alerts-on-pull-requests/ in the sense that the following "code scanning" feature:

the tool reports only alerts inside the lines of code that the pull request has changed

can make some alerts effectively invisible. Then again I'm not sure how SARIF is generated by CIFuzz.

[evverx]

@jamacku I'm not sure if you are interested in this or not but since you already dug into that GH feature I think you're in a better position to decide whether it should be fine to turn CIFuzz alerts on in the systemd repository. As far as I can remember systemd merged PRs where CIFuzz reported issues in the past. Those alerts should make CIFuzz findings much more noticeable hopefully.

[jamacku]

@evverx I think we can try it and see the results for enabling code scanning for oss-fuzz. It could be helpful and more visible for systemd maintainers.

NOTE: I saw that GitHub started comparing base scans (made on push to branch) and scans on Pull Requests using partial fingerprints. And tries to report only new findings. So some reports could be lost (not reported on PR). I have experienced this issue with ShellCheck, where many defects could be accumulated on the small number of code lines. But I think this isn't the case for oss-fuzz.

[evverx]

ShellCheck, where many defects could be accumulated on the small number of code lines. But I think this isn't the case for oss-fuzz

@jamacku agreed. My concern was that unlike, say, ShellCheck CIFuzz can't always pinpoint code responsible for newly introduced bugs because it has only ASan/UBSan/MSan backtraces it should somehow match with PR diffs to make them visible. If those backtraces happen to point to some seemingly unrelated code my understanding is that GitHub hides alerts like that and they pop up later once PRs are merged.

[jamacku]

@evverx Yes, as far as I know, defects that don't have code line numbers directly associated with them won't be shown on PRs (as code annotations). They will still be visible for systemd org/repo members in the Security section but only after PR is merged or if you manually filter by pr number.

As a safety check, you can also set the status check of GA running CIFuzz to fail for better visibility (we do it in differential-shellcheck). But the best would be to test behavior (on fork if possible), GitHub doesn't document these things, so you unfortunately never know what you get :smile: .

[evverx]

They will still be visible for systemd org/repo members in the Security section but only after PR is merged or if you manually filter by pr number

The problem is that external contributors should see that too to be able to fix bugs in their own PRs without having to wait for systemd maintainers. All in all, that security tab is security theater mostly. I mean, it's possible to just fork a repo and run all those actions to get the same alerts there. But I digress :-)

you can also set the status check of GA running CIFuzz to fail for better visibility

Makes sense.

Anyway, since I haven't seen SARIF yet I'm mostly speculating here. Let's wait for @jonathanmetzman to weigh in.

[jonathanmetzman]

can make some alerts effectively invisible. Then again I'm not sure how SARIF is generated by CIFuzz.

Unfortunately I think this can make some of our alerts invisible. This feature really only makes sense for static analysis not dynamic analysis.

I guess turning on alerts won't make issues any less visible than the are currently (currently people need to check the results of CI when it is run on a commit).

[evverx]

Got it. One last thing as far as I can remember there are MSan false positives in the systemd repository (because systemd doesn't build its dependencies with MSan) and if they aren't included in SARIF files it should probably be fine to turn this on then.

[jonathanmetzman]

Got it. One last thing as far as I can remember there are MSan false positives in the systemd repository (because systemd doesn't build its dependencies with MSan) and if they aren't included in SARIF files it should probably be fine to turn this on then.

Reporting things as sarif will probably be gated on an input variable. So we can block MSAN false positives by making the config a little more complex.

[evverx]

I think ideally systemd should start building its dependencies with MSan :-) but I put that PR on hold.

So we can block MSAN false positives by making the config a little more complex

I wonder if it would be possible to just skip known issues without editing configs? CIFuzz itself doesn't report those false positives. The fuzz targets fail and the backtraces are there but since they are reproducible with the latest builds CIFuzz just ignores them. It could similarly ignore them when SARIF is generated I think.

[jonathanmetzman]

Ah so then these issues won't be reported.

On Thu, Apr 27, 2023 at 3:11 PM Evgeny Vereshchagin < @.***> wrote:

I think ideally systemd should start building its dependencies with MSan :-) but I put that PR on hold.

So we can block MSAN false positives by making the config a little more complex

I wonder if it would be possible to just skip known issues without editing configs? CIFuzz itself doesn't report those false positives. The fuzz targets fails and the backtraces are there but since they are reproducible with the latest builds CIFuzz just ignores them. It could similarly ignore them when SARIF is generated I think.

— Reply to this email directly, view it on GitHub https://github.com/google/oss-fuzz/issues/10090#issuecomment-1526209629, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPG6LSSESXDTPSJHMUAAKDXDLAG3ANCNFSM6AAAAAAW6WRE3Y . You are receiving this because you were mentioned.Message ID: @.***>

[evverx]

@jonathanmetzman on a somewhat related note that "OSSF scorecard" thing is going to complain about "security-events: write" and inject its promotional links once it's added. I wonder if there is a way to somehow avoid it? That project doesn't seem to be interested in fixing its stuff but maybe OSS-Fuzz could somehow affect it.

[jonathanmetzman]

on a somewhat related note that "OSSF scorecard" thing is going to complain about "security-events: write" and inject its promotional links once it's added

Because we are writing sarif files now?

I wonder if there is a way to somehow avoid it?

I'm hoping this isn't the process for every code scanning tool: https://github.com/ossf/scorecard/issues/2840 but maybe I need to do this?

[jonathanmetzman]

I'll document what to do to turn on the code scanning by tomorrow. Thanks a lot for your help!

[evverx]

Because we are writing sarif files now?

Yeah. "security-events" should be set to "write" to be able to send SARIF files but that scorecard thing thinks that because of that supply chain is under attack and emits "high severity" alerts: https://github.com/systemd/systemd/pull/26928#issuecomment-1488300416 :-)

[evverx]

I'm hoping this isn't the process for every code scanning tool: https://github.com/ossf/scorecard/issues/2840 but maybe I need to do this?

I don't know. I got some sort of cease and desist there when I asked them not to show promotional links leading to semi-automated PRs fixing imaginary "security" issues.

[jonathanmetzman]

@evverx sarif is documented now? Could you help me out and try this on systemd? https://google.github.io/oss-fuzz/getting-started/continuous-integration/ I can send a PR if you'd like.

[jamacku]

if: failure() && steps.build.outcome == 'success'

I think it should be if: always() && .... Otherwise, GitHub won't mark resolved issues as fixed.

[jonathanmetzman]

Ah, let me try changing this. Thank you!

[evverx]

I can send a PR if you'd like

@jonathanmetzman that would be great. Thanks! Could you cc @jamacku there? @jamacku is an expert on GH Actions in the systemd repository.

sarif is documented now

I think security-events: write should be documented too. Without that the action can't upload SARIF in the systemd repository (where all the actions are read-only by default).

I think it should be if: always() && ...

@jamacku It's a good point. That if: failure() statement came from https://github.com/google/oss-fuzz/commit/8ba4f3a3755f8a7a5f8071b174e7189fc26fa4dd and it doesn't seem to be applicable to the part sending SARIF. I'm not sure what should be used there instead.

[jamacku]

@jamacku It's a good point. That if: failure() statement came from 8ba4f3a and it doesn't seem to be applicable to the part sending SARIF. I'm not sure what should be used there instead.

I think it's ok to upload artifacts only when CIFuzz fails, but SARIF should be uploaded always (if: always() && steps.build.outcome == 'success').

[evverx]

I'm hoping this isn't the process for every code scanning tool: https://github.com/ossf/scorecard/issues/2840 but maybe I need to do this?

My understanding is that @joycebrum could help with this. @joycebrum I wonder if apart from CIFuzz it would be possible to address https://github.com/systemd/systemd/pull/26928#issuecomment-1488300416? The "differential-shellcheck" action is a code scanning action that is maintained by @jamacku. It's legit. If it helps its scorecard score is 8.1 at the moment to judge from https://github.com/redhat-plumbers-in-action/differential-shellcheck/.

[jonathanmetzman]

This is done.

[evverx]

I still think it would be great if CIFuzz could attach backtraces. Its alerts are concise and to the point but they make CIFuzz look kind of grumpy :-)