-
Notifications
You must be signed in to change notification settings - Fork 520
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
✨ New Probe: Memory safety #4499
Conversation
a159254
to
56f3ef7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4499 +/- ##
==========================================
+ Coverage 66.80% 68.36% +1.56%
==========================================
Files 230 248 +18
Lines 16602 18762 +2160
==========================================
+ Hits 11091 12827 +1736
- Misses 4808 5084 +276
- Partials 703 851 +148 |
here are some repos to test the code: go run main.go --repo github.com/microsoft/midi --probes memorysafe --format probe (.net unsafe code) |
Hoping to have some time to look at this tomorrow. Quick question about organization:
Probes are usually one specific behavior. In this case, the probe is concerned with looking for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't quite go through the tests yet, since the semantics of the probe will probably change through this discussion.
the intention of this probe is to save the end user the need to know which best practices they are expected to have in the repo, and to give them a kind of a one stop shop for finding out if there's anything they're missing. Our goal was to try and implement the different practices that are suggested in the memory safety guides for memory-safe-by-default and non-memory-safe-by-default guides into this single probe. |
On the other hand, our goal for probes is that consumers are concerned with specific behavior and by having them do one thing it's easier to build policy. |
ok, so just for clarity, for this PR do you want to separate also to two different probes (one for c# and one for go)? |
Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
* feat: create binary without OS and arch in name Previously binaries were created with their architecture and OS included as part of their binary name. Removing the `binary: scorecard-<linux|darwin|windows>-{{ .Arch }}` line allows us to collapse all of the different build configs into a single universal build that caters for [linux,darwin,windos]*[arm64,amd64] `- -buildmode=exe` was not needed on the windows builds and was also removed. closes ossf#4517 Signed-off-by: Tim Sparg <6872586+timothysparg@users.noreply.github.com> * fix: resolve name_template deprecation As per https://goreleaser.com/deprecations#snapshotname_template `snapshot.name_template` has been replaced with `snapshot.version_template` Signed-off-by: Tim Sparg <6872586+timothysparg@users.noreply.github.com> --------- Signed-off-by: Tim Sparg <6872586+timothysparg@users.noreply.github.com> Signed-off-by: balteravishay <avishay.balter@gmail.com>
* add git handler for GitHub repositories This is primarily aimed at helping in cases where a repository's .gitattributes file causes files to not be analyzed. Signed-off-by: Spencer Schrock <sschrock@google.com> * use variadic options to configure GitHub repoclient This will let us use the new entrypoint in a backwards compatible way, similar to the scorecard.Run change made in the v5 release. Signed-off-by: Spencer Schrock <sschrock@google.com> * add flag to enable github git mode Signed-off-by: Spencer Schrock <sschrock@google.com> * rename flag to be forge agnostic export-ignore is not a github specific feature, and other forges, like gitlab, suffer from the same bug. Signed-off-by: Spencer Schrock <sschrock@google.com> * move git file handler to internal package This will allow sharing with GitLab in a followup PR Signed-off-by: Spencer Schrock <sschrock@google.com> * add a test Signed-off-by: Spencer Schrock <sschrock@google.com> * use new toplevel gitmode argument also moves a func around for smaller PR diff. Signed-off-by: Spencer Schrock <sschrock@google.com> * add path traversal test Signed-off-by: Spencer Schrock <sschrock@google.com> * change flag to file-mode Signed-off-by: Spencer Schrock <sschrock@google.com> * fix repo typo in options test the value isn't used to connect to anything though. Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com> Signed-off-by: balteravishay <avishay.balter@gmail.com>
go-git is not thread-safe, so the old method would lead to race conditions when multiple checks called ListFiles at the same time. This was reproducible when built with `-race`, and now no longer occurs because it's done inside a sync.Once. Signed-off-by: Spencer Schrock <sschrock@google.com> Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
…ols (ossf#4479) Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
Signed-off-by: balteravishay <avishay.balter@gmail.com>
…ols (ossf#4526) Signed-off-by: balteravishay <avishay.balter@gmail.com>
…sf#4529) Signed-off-by: balteravishay <avishay.balter@gmail.com>
ca8d999
to
d537a44
Compare
@spencerschrock fixed all comments here. |
Previously, this relied on the use of a detail logger, but we're not guaranteed to have one when running probes on their own. Instead, probes usually convey parse errors as OutcomeError. This change also required updating the test runner, as OnMatchingFileContentDo makes use of ListFiles under the hood. So simply returning a list of files was causing issues where csproj files were being parsed as Go files and vice versa. Signed-off-by: Spencer Schrock <sschrock@google.com>
@balteravishay had to tweak this to fix an issue similar to the one in #4531 . Parse errors now return a finding.OutcomeError, similar to Pinned-Dependencies |
Thank you very much @spencerschrock. Patch looks good to me. |
What kind of change does this PR introduce?
This PR provides the foundation for addressing issue #3736 by adding a new probe that checks if the code uses non memory safe practices for the repository languages.
The goal is to automate the detection of as many of the practices that the memory safety SIG provides under the Best Practices - Memory-Safe By Default Languages and the Best Practices - Non Memory-Safe By Default Languages guides.
What is the current behavior?
Today scorecard does not detect memory safe practices in it's core features or in any of the probes.
What is the new behavior (if this is a feature change)?**
Probe detects the following:
for golang it detects if the code imports the unsafe package and points to the locations where it is used.
for c# it detects if the projects allow for unsafe blocks which is a requirement for any project that would use any form of .Net unsafe code, pointer types, and function pointers
Tests for the changes have been added (for bug fixes/features)
Which issue(s) this PR fixes
This code change addresses issue #3736 but does not close it (to be discussed)
Special notes for your reviewer
This code change and the implementation of it were discussed in scorecard community calls with @spencerschrock
Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note
(In particular, describe what changes users might need to make in their
application as a result of this pull request.)