✨ New Probe: Memory safety #4499

balteravishay · 2025-01-22T19:06:52Z

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.

PR title follows the guidelines defined in our pull request documentation

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.)

Added independent probe that checks for ecosystem specific non-memory safety practices in the codebase and flags them.

codecov · 2025-01-22T20:19:01Z

Codecov Report

Attention: Patch coverage is 83.72093% with 21 lines in your changes missing coverage. Please review.

Project coverage is 68.34%. Comparing base (353ed60) to head (537aa61).
Report is 119 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4499      +/-   ##
==========================================
+ Coverage   66.80%   68.34%   +1.54%     
==========================================
  Files         230      248      +18     
  Lines       16602    18763    +2161     
==========================================
+ Hits        11091    12824    +1733     
- Misses       4808     5089     +281     
- Partials      703      850     +147

balteravishay · 2025-01-24T08:15:24Z

here are some repos to test the code:

go run main.go --repo github.com/microsoft/midi --probes memorysafe --format probe (.net unsafe code)
go run main.go --repo github.com/microsoft/winget-cli --probes memorysafe --format probe (.net safe code)
go run main.go --repo github.com/pkujhd/goloader --probes memorysafe --format probe (go unsafe code)
go run main.go --repo github.com/ossf/scorecard --probes memorysafe --format probe (go safe code)

spencerschrock · 2025-01-30T16:30:01Z

Hoping to have some time to look at this tomorrow. Quick question about organization:

Added independent probe that checks for ecosystem specific non-memory safety practices

Probes are usually one specific behavior. In this case, the probe is concerned with looking for unsafe usage across ecosystems. I'm not sure if your intention was to use this probe for the go race detector follow up too, but generally different behavior tests get broken up into different probes.

spencerschrock

didn't quite go through the tests yet, since the semantics of the probe will probably change through this discussion.

probes/memorysafe/def.yml

probes/memorysafe/impl.go

spencerschrock · 2025-01-31T22:41:34Z

probes/memorysafe/impl.go

+				"Golang code uses the unsafe package", &finding.Location{
+					Path: path,
+				}, finding.OutcomeFalse)


we have much more info than path available if we use :
https://pkg.go.dev/go/ast#ImportSpec.Pos and https://pkg.go.dev/go/ast#ImportSpec.End

Something like:

position := fset.Position(i.Pos())

and then we have access to line info too and can set finding. LineStart. https://pkg.go.dev/go/token#Position

added linestart. do you think any other information should be added?

probes/memorysafe/impl.go

balteravishay · 2025-02-05T19:51:14Z

Hoping to have some time to look at this tomorrow. Quick question about organization:

Added independent probe that checks for ecosystem specific non-memory safety practices

Probes are usually one specific behavior. In this case, the probe is concerned with looking for unsafe usage across ecosystems. I'm not sure if your intention was to use this probe for the go race detector follow up too, but generally different behavior tests get broken up into different probes.

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.
The concern with breaking that up for different probes is that user will have to run a bunch of different probes, per ecosystem, per guideline, just to get the overall view.
hope this makes sense.

spencerschrock · 2025-02-06T15:41:47Z

The concern with breaking that up for different probes is that user will have to run a bunch of different probes, per ecosystem, per guideline, just to get the overall view.

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.

balteravishay · 2025-02-06T18:19:14Z

The concern with breaking that up for different probes is that user will have to run a bunch of different probes, per ecosystem, per guideline, just to get the overall view.

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>

* 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>

balteravishay · 2025-02-17T21:17:44Z

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)?

No, for example the fuzzed probe is the same regardless of ecosystem. For both C# and Go, the behavior being tested is the same: is the project enabling/using some feature to step around the normally memory safe (type) features.

@spencerschrock, this is now fixed to only test unsafe code blocks.

spencerschrock · 2025-02-19T15:43:52Z

@spencerschrock, this is now fixed to only test unsafe code blocks.

Thanks will take a look today or tomorrow.

spencerschrock

a few minor things to fix

spencerschrock · 2025-02-20T17:38:43Z

probes/unsafeblock/def.yml

+outcome:
+  - For supported ecosystem, the probe returns OutcomeFalse per unsafe block.
+  - If the project has no unsafe blocks, the probe returns OutcomeTrue.
+remediation:
+  onOutcome: False


this seems inverted with what I would expect

unsafeblock would return true if there are unsafe blocks.

spencerschrock · 2025-02-20T17:46:04Z

probes/unsafeblock/impl.go

+	prominentLangs, err := getLanguageChecks(raw)
+	if err != nil {
+		return nil, Probe, err
+	}
+	findings := []finding.Finding{}
+	for _, lang := range prominentLangs {


nit: prominentLangs -> langs

"prominent" is something the fuzzing check to imply a language has a higher than average amount of lines of code. here you're just concerned with languages in the repo.

alternatively, do we even need to use the languages? checkDotnetAllowUnsafeBlocks and checkGoUnsafePackage are limited to .go and .csproj files. Although I assume there's a speed benefit as written here for projects that aren't written in either Go or C#.

spencerschrock · 2025-02-20T17:48:46Z

probes/unsafeblock/impl.go

+		found, err := finding.NewWith(fs, Probe,
+			"All supported ecosystems do not declare or use unsafe code blocks", nil, finding.OutcomeTrue)


as mentioned elsewhere, the outcome is flipped from what I would expect.

spencerschrock · 2025-02-20T18:07:31Z

probes/unsafeblock/impl.go

+	if len(langs) == 0 {
+		return []languageMemoryCheckConfig{}, nil
+	}


nit: this seems redundant as not entering the loop would produce the same result.

spencerschrock · 2025-02-20T18:15:41Z

probes/unsafeblock/impl.go

+			found, err := finding.NewWith(fs, Probe,
+				"Golang code uses the unsafe package", &finding.Location{
+					Path: path, LineStart: &lineStart,
+				}, finding.OutcomeFalse)


flagging outcome flip

spencerschrock · 2025-02-20T18:22:59Z

probes/unsafeblock/impl_test.go

+				return os.Open(file)
+			}).AnyTimes()
+			raw.RepoClient = mockRepoClient
+			raw.Dlogger = checker.NewLogger()


normally in tests we use the TestDetailLogger

scorecard/utests/utlib.go

Lines 38 to 41 in 3b42b6e

// TestDetailLogger implements `checker.DetailLogger`.

type TestDetailLogger struct {

messages []checker.CheckDetail

}

spencerschrock · 2025-02-20T18:23:35Z

probes/unsafeblock/impl_test.go

+		return nil, fmt.Errorf("error")
+	}).AnyTimes()
+	raw.RepoClient = mockRepoClient
+	raw.Dlogger = checker.NewLogger()


same comment about logger

spencerschrock · 2025-02-20T18:24:24Z

probes/unsafeblock/impl_test.go

+				return nil, fmt.Errorf("error")
+			}).AnyTimes()
+			raw.RepoClient = mockRepoClient
+			raw.Dlogger = checker.NewLogger()


balteravishay temporarily deployed to gitlab January 22, 2025 19:07 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test January 22, 2025 19:07 — with GitHub Actions Inactive

balteravishay force-pushed the memory-safety-probe branch from a159254 to 56f3ef7 Compare January 22, 2025 19:08

balteravishay temporarily deployed to gitlab January 22, 2025 20:10 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test January 22, 2025 20:10 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab January 22, 2025 22:04 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test January 22, 2025 22:05 — with GitHub Actions Inactive

balteravishay marked this pull request as ready for review January 22, 2025 22:13

balteravishay requested a review from a team as a code owner January 22, 2025 22:13

balteravishay requested review from justaugustus and raghavkaul and removed request for a team January 22, 2025 22:13

balteravishay temporarily deployed to gitlab January 23, 2025 20:48 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test January 23, 2025 20:48 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab January 23, 2025 21:03 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test January 23, 2025 21:03 — with GitHub Actions Inactive

balteravishay mentioned this pull request Jan 23, 2025

Adding memory safety related checks #3736

Open

balteravishay temporarily deployed to gitlab January 24, 2025 08:17 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test January 24, 2025 08:17 — with GitHub Actions Inactive

spencerschrock reviewed Jan 31, 2025

View reviewed changes

balteravishay temporarily deployed to gitlab February 5, 2025 19:53 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test February 5, 2025 19:53 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab February 5, 2025 21:20 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test February 5, 2025 21:20 — with GitHub Actions Inactive

balteravishay and others added 14 commits February 17, 2025 21:15

fix list

68cb573

Signed-off-by: balteravishay <avishay.balter@gmail.com>

fix search in map

b40e7b1

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump the github-actions group with 2 updates (ossf#4503)

7506bf2

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump actions/setup-go from 5.2.0 to 5.3.0 (ossf#4504)

2be94e7

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump github.com/golang/glog from 1.2.2 to 1.2.4 (ossf#4507)

0d5e023

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump github/codeql-action in the github-actions group (ossf#4509)

4e272cd

Signed-off-by: balteravishay <avishay.balter@gmail.com>

pr comments

5db617d

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump the github-actions group with 4 updates (ossf#4513)

dbf9080

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump the golang group across 8 directories with 1 update (ossf#4512)

fcd6b0d

Signed-off-by: balteravishay <avishay.balter@gmail.com>

🌱 Bump the gomod group across 2 directories with 8 updates (ossf#4515)

b8110a2

Signed-off-by: balteravishay <avishay.balter@gmail.com>

change to be specific for unsafe blocks

208b23c

Signed-off-by: balteravishay <avishay.balter@gmail.com>

balteravishay force-pushed the memory-safety-probe branch from 0ed8dcc to 208b23c Compare February 17, 2025 21:15

balteravishay temporarily deployed to gitlab February 17, 2025 21:15 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test February 17, 2025 21:16 — with GitHub Actions Inactive

Merge branch 'main' into memory-safety-probe

0ff1ab4

balteravishay temporarily deployed to gitlab February 17, 2025 21:16 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test February 17, 2025 21:16 — with GitHub Actions Inactive

Merge branch 'main' into memory-safety-probe

dfb9b7e

balteravishay temporarily deployed to gitlab February 18, 2025 14:57 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test February 18, 2025 14:57 — with GitHub Actions Inactive

Merge branch 'main' into memory-safety-probe

537aa61

balteravishay temporarily deployed to gitlab February 18, 2025 17:29 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test February 18, 2025 17:29 — with GitHub Actions Inactive

balteravishay requested a review from spencerschrock February 19, 2025 12:14

spencerschrock approved these changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ New Probe: Memory safety #4499

✨ New Probe: Memory safety #4499

balteravishay commented Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

balteravishay commented Jan 24, 2025

spencerschrock commented Jan 30, 2025

spencerschrock left a comment

spencerschrock Jan 31, 2025

balteravishay Feb 5, 2025

balteravishay commented Feb 5, 2025

spencerschrock commented Feb 6, 2025

balteravishay commented Feb 6, 2025

balteravishay commented Feb 17, 2025

spencerschrock commented Feb 19, 2025

spencerschrock left a comment

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

spencerschrock Feb 20, 2025

		found, err := finding.NewWith(fs, Probe,
		"All supported ecosystems do not declare or use unsafe code blocks", nil, finding.OutcomeTrue)

	// TestDetailLogger implements `checker.DetailLogger`.
	type TestDetailLogger struct {
	messages []checker.CheckDetail
	}

✨ New Probe: Memory safety #4499

Are you sure you want to change the base?

✨ New Probe: Memory safety #4499

Conversation

balteravishay commented Jan 22, 2025

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior (if this is a feature change)?**

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

balteravishay commented Jan 24, 2025

spencerschrock commented Jan 30, 2025

spencerschrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balteravishay commented Feb 5, 2025

spencerschrock commented Feb 6, 2025

balteravishay commented Feb 6, 2025

balteravishay commented Feb 17, 2025

spencerschrock commented Feb 19, 2025

spencerschrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2025 •

edited

Loading