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

Signed-off-by: balteravishay <[email protected]>

The intention was always to sort, but the wrong sort function was used. This led to an unsorted list, which was dependent on map iteration order, leading to flaky unit tests. This was reproducible with go test when passing a large --count option. Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: balteravishay <[email protected]>

Signed-off-by: balteravishay <[email protected]>

Since upgrading to v1.9.1 we've had performance issues in the weekly analysis, which may be related to one of the Maven features here. Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: balteravishay <[email protected]>

…sf#4467) Signed-off-by: balteravishay <[email protected]>

Signed-off-by: balteravishay <[email protected]>

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

Codecov Report

Attention: Patch coverage is 79.28571% with 29 lines in your changes missing coverage. Please review.

Project coverage is 68.58%. Comparing base (353ed60) to head (7e27479).
Report is 104 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4499      +/-   ##
==========================================
+ Coverage   66.80%   68.58%   +1.77%     
==========================================
  Files         230      247      +17     
  Lines       16602    18583    +1981     
==========================================
+ Hits        11091    12745    +1654     
- Misses       4808     5003     +195     
- Partials      703      835     +132

Signed-off-by: balteravishay <[email protected]>

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)

Signed-off-by: balteravishay <[email protected]>

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.

spencerschrock · 2025-01-31T19:32:58Z

probes/memorysafe/def.yml

+id: memorysafe
+lifecycle: experimental
+short: Flags non memory safe practices in this project.
+motivation: >
+  Memory safety in software should be considered a continuum, rather than being binary.
+  This probe does not consider a specific ecosystem more or less memory safe than another, but rather tries to surface non memory safe code or practices in the project, in the context of the ecosystems it is using.
+implementation: >
+  The probe is ecosystem-specific and tries to flag non memory safe code blocks in the project by looking at the code and practices used in the project.
+  It may look for specific memory safety practices, such as the use tools or non memory-safe patterns and code.
+  The probe supports multiple checks for each ecosystem, and the outcome is based on the results of these checks.


this would need to be altered to focus on just unsafe detection if the probe does just one thing. Though unsafe is too generic.

So maybe something like unsafeblock? Open to suggestions/brainstorming.

spencerschrock · 2025-01-31T19:35:53Z

probes/memorysafe/def.yml

+  clients:
+    - github


It's just based on file content? so it could support all of them?

- github - gitlab - localdir

spencerschrock · 2025-01-31T19:42:43Z

probes/memorysafe/impl.go

+			if langFunc == nil {
+				raw.Dlogger.Warn(&checker.LogMessage{
+					Text: fmt.Sprintf("no function pointer found for language %s", lang.Desc),
+				})
+			}


what's the goal for this? it would still panic below because we call langFunc(raw) below.

We could avoid the panic by adding a continue, but this is based on languageMemorySafeSpecs which should be constant. So any nil pointer is a programming mistake, and a panic could be appropriate to keep.

My vote is to remove this block, since the warning wouldn't be visible to anyone due to the panic. Thoughts?

spencerschrock · 2025-01-31T19:43:53Z

probes/memorysafe/impl.go

+func getRepositoryLanguageChecks(raw *checker.CheckRequest) []languageMemoryCheckConfig {
+	langs, err := raw.RepoClient.ListProgrammingLanguages()
+	if err != nil {
+		raw.Dlogger.Warn(&checker.LogMessage{
+			Text: fmt.Sprintf("RepoClient retured error for ListProgrammingLanguages: %v", err),
+		})


instead of handling the error and continuing, let's just return it. We're not going to be able to do much without the repo's languages.

func getLanguageChecks(raw *checker.CheckRequest) ([]languageMemoryCheckConfig, error) {

spencerschrock · 2025-01-31T22:23:40Z

probes/memorysafe/impl.go

+		dl, ok := args[1].(checker.DetailLogger)
+		if !ok {
+			// panic if it is not correct type
+			panic(fmt.Sprintf("expected type checker.DetailLogger, got %v", reflect.TypeOf(args[1])))


we should move the type checking out of the err != nil block to make the panics when passing wrong types more obvious.

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

spencerschrock · 2025-01-31T22:47:43Z

probes/memorysafe/impl.go

+	if len(findings) == 0 {
+		found, err := finding.NewWith(fs, Probe,
+			"Golang code does not use the unsafe package", nil, finding.OutcomeTrue)


we could also move this (and the c# version) to one combined top level check in Run before the return,depending on the semantics we want the probe to have.

spencerschrock · 2025-01-31T22:48:05Z

probes/memorysafe/impl.go

+		dl, ok := args[1].(checker.DetailLogger)
+		if !ok {
+			// panic if it is not correct type
+			panic(fmt.Sprintf("expected type checker.DetailLogger, got %v", reflect.TypeOf(args[1])))


same comment about moving type check

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 and others added 11 commits January 22, 2025 19:08

draft code

62a5754

Signed-off-by: balteravishay <[email protected]>

🌱 Bump actions/setup-go from 5.1.0 to 5.2.0 (ossf#4445)

4c08d96

Signed-off-by: balteravishay <[email protected]>

🌱 Bump golang.org/x/crypto from 0.29.0 to 0.31.0 (ossf#4460)

ebff76f

Signed-off-by: balteravishay <[email protected]>

Downgrade osv-scanner to v1.9.0 (ossf#4461)

af1f8a0

Since upgrading to v1.9.1 we've had performance issues in the weekly analysis, which may be related to one of the Maven features here. Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: balteravishay <[email protected]>

🌱 Bump the github-actions group across 1 directory with 3 updates (os…

d6cf0ab

…sf#4467) Signed-off-by: balteravishay <[email protected]>

🌱 Bump the gomod group across 2 directories with 4 updates (ossf#4469)

7e1edaf

Signed-off-by: balteravishay <[email protected]>

update definition.yaml

56ba2e1

Signed-off-by: balteravishay <[email protected]>

remove prominent languages code

f4eb0fd

Signed-off-by: balteravishay <[email protected]>

tests

83d1fd2

Signed-off-by: balteravishay <[email protected]>

more implementation details

56f3ef7

Signed-off-by: balteravishay <[email protected]>

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

Merge branch 'main' into memory-safety-probe

fac9482

Signed-off-by: balteravishay <[email protected]>

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

more coverage

a047af7

Signed-off-by: balteravishay <[email protected]>

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

more tests

7765eae

Signed-off-by: balteravishay <[email protected]>

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

fix list

0c47404

Signed-off-by: balteravishay <[email protected]>

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

fix search in map

7e27479

Signed-off-by: balteravishay <[email protected]>

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

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

spencerschrock Jan 31, 2025

spencerschrock Jan 31, 2025

spencerschrock Jan 31, 2025

spencerschrock Jan 31, 2025

spencerschrock Jan 31, 2025

spencerschrock Jan 31, 2025

spencerschrock Jan 31, 2025

✨ 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

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