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

Update CodeQL Workflow for Code Security Analysis #1346

Closed
wants to merge 2 commits into from

Conversation

b4yuan
Copy link
Contributor

@b4yuan b4yuan commented Feb 6, 2024

We are researchers at Purdue University in the USA. We are studying the potential benefits and costs of using CodeQL on open-source repositories of embedded software.

We wrote up a report of our findings so far. The TL;DR is “CodeQL outperforms the other freely-available static analysis tools, with fairly low false positive rates and lots of real defects”. You can read about the report here: https://arxiv.org/abs/2310.00205

This PR builds on the previous workflow functionality by allowing for rule filtration, uploading CodeQL results to Code scanning, and uploading CodeQL results as an artifact.

False positives: We find that around 20% of errors are false positives, but that these FPs are polarized and only a few rules contribute to most FPs. We find that the top rules contributing to FPs are: cpp/uninitialized-local, cpp/missing-check-scanf, cpp/suspicious-pointer-scaling, cpp/unbounded-write, cpp/constant-comparison, and cpp/inconsistent-null-check. Adding a filter to filter out certain rules that contribute to a high FP rate can be done simply in the workflow file.

Uploading to Code scanning: This workflow uploads the results to Code scanning under the Security tab on GitHub:
image
From here, vulnerabilities can be addressed simply by clicking the bug or dismissed for various reasons:
image

Uploading as an artifact: This workflow also uploads the results as an artifact with a retention period of 5 days.

Include rule filtering to filter out rules with high false positive rates, uploading CodeQL results to 'Code scanning' under the Security tab on Github, uploading CodeQL results as an artifact

Signed-off-by: Brian <89487381+b4yuan@users.noreply.github.com>
Signed-off-by: Brian <89487381+b4yuan@users.noreply.github.com>
@ejoerns
Copy link
Member

ejoerns commented Feb 28, 2024

@b4yuan Thank you for your contribution.

This PR builds on the previous workflow functionality by allowing for rule filtration, uploading CodeQL results to Code scanning, and uploading CodeQL results as an artifact.

The results are already available in GitHub code scanning with the current workflow.
Since this is also our primary interface to check this, I don't see any advantage of uploading them.

False positives: We find that around 20% of errors are false positives, but that these FPs are polarized and only a few rules contribute to most FPs. We find that the top rules contributing to FPs are: cpp/uninitialized-local, cpp/missing-check-scanf, cpp/suspicious-pointer-scaling, cpp/unbounded-write, cpp/constant-comparison, and cpp/inconsistent-null-check. Adding a filter to filter out certain rules that contribute to a high FP rate can be done simply in the workflow file.

As you may have noticed, we have some false positives, too. But also several reasonable remarks.

We would however like to choose a different approach to address these:

  • Many of (our) false positives are caused by erroneously inspecting meson-generated files (see 6850 for example). Here the pattern for the inspected source code files may need to be adapted.

  • Other frequent warnings are "poorly documented large function" or "Commented-out code" which are probably valid aspects we should fix or dismiss individually.

  • The remaining number of false positives is on a manageable scale so that we can dismiss them individually.

I don't see any advantage in just deactivating checks that don't cause actual problems by now. If it turns out in the future that a specific check is prone to trigger false positives quite often in this project, we can still think about filtering it out.

@ejoerns ejoerns closed this Feb 28, 2024
ejoerns added a commit to ejoerns/rauc that referenced this pull request Feb 28, 2024
There is a severe number of false-positive in code scanning caused by
inspecting meson-internal test files like
'build/meson-private/tmpzb46osmq/testfile.c'.

As a workaround, use the 'filter-sarif' action to filter out these
results before uploading the SARIF (Static Analysis Results Interchange
Format).

This PR was inspired by rauc#1346 and the
example from https://github.com/advanced-security/filter-sarif.

Signed-off-by: Enrico Joerns <ejo@pengutronix.de>
ejoerns added a commit to ejoerns/rauc that referenced this pull request Feb 29, 2024
There is a severe number of false-positive in code scanning caused by
inspecting meson-internal test files like
'build/meson-private/tmpzb46osmq/testfile.c'.

As a workaround, use the 'filter-sarif' action to filter out these
results before uploading the SARIF (Static Analysis Results Interchange
Format).

This PR was inspired by rauc#1346 and the
example from https://github.com/advanced-security/filter-sarif.

Signed-off-by: Enrico Joerns <ejo@pengutronix.de>
@ejoerns
Copy link
Member

ejoerns commented Feb 29, 2024

Inspired by your PR, I created #1357 to fix the primary class of false positives we currently see in this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants