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

✨ New Probe: Memory safety #4499

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/probes.md
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,21 @@ If collaborators, members or owners have NOT participated in issues in the last
The probe returns 1 true outcome if the project has no workflows "write" permissions a the "job" level.


## memorysafe

**Lifecycle**: experimental

**Description**: 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.

**Outcomes**: For supported ecosystem, the probe returns OutcomeTrue per safe method or tool.
For supported ecosystem, the probe returns OutcomeFalse per unsafe code or method.
If the project has no supported ecosystems, the probe returns OutcomeNotApplicable.


## packagedWithAutomatedWorkflow

**Lifecycle**: stable
Expand Down
15 changes: 14 additions & 1 deletion internal/dotnet/csproj/csproj.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var errInvalidCsProjFile = errors.New("error parsing csproj file")
type PropertyGroup struct {
XMLName xml.Name `xml:"PropertyGroup"`
RestoreLockedMode bool `xml:"RestoreLockedMode"`
AllowUnsafeBlocks bool `xml:"AllowUnsafeBlocks"`
}

type Project struct {
Expand All @@ -32,6 +33,18 @@ type Project struct {
}

func IsRestoreLockedModeEnabled(content []byte) (bool, error) {
return isCsProjFilePropertyGroupEnabled(content, func(propertyGroup *PropertyGroup) bool {
return propertyGroup.RestoreLockedMode
})
}

func IsAllowUnsafeBlocksEnabled(content []byte) (bool, error) {
return isCsProjFilePropertyGroupEnabled(content, func(propertyGroup *PropertyGroup) bool {
return propertyGroup.AllowUnsafeBlocks
})
}

func isCsProjFilePropertyGroupEnabled(content []byte, predicate func(*PropertyGroup) bool) (bool, error) {
var project Project

err := xml.Unmarshal(content, &project)
Expand All @@ -40,7 +53,7 @@ func IsRestoreLockedModeEnabled(content []byte) (bool, error) {
}

for _, propertyGroup := range project.PropertyGroups {
if propertyGroup.RestoreLockedMode {
if predicate(&propertyGroup) {
return true, nil
}
}
Expand Down
5 changes: 4 additions & 1 deletion probes/entries.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"github.com/ossf/scorecard/v5/probes/hasUnverifiedBinaryArtifacts"
"github.com/ossf/scorecard/v5/probes/issueActivityByProjectMember"
"github.com/ossf/scorecard/v5/probes/jobLevelPermissions"
"github.com/ossf/scorecard/v5/probes/memorysafe"
"github.com/ossf/scorecard/v5/probes/packagedWithAutomatedWorkflow"
"github.com/ossf/scorecard/v5/probes/pinsDependencies"
"github.com/ossf/scorecard/v5/probes/releasesAreSigned"
Expand Down Expand Up @@ -173,7 +174,9 @@ var (
}

// Probes which don't use pre-computed raw data but rather collect it themselves.
Independent = []IndependentProbeImpl{}
Independent = []IndependentProbeImpl{
memorysafe.Run,
}
)

//nolint:gochecknoinits
Expand Down
41 changes: 41 additions & 0 deletions probes/memorysafe/def.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Copyright 2025 OpenSSF Scorecard Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

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.
Comment on lines +15 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

outcome:
- For supported ecosystem, the probe returns OutcomeTrue per safe method or tool.
- For supported ecosystem, the probe returns OutcomeFalse per unsafe code or method.
- If the project has no supported ecosystems, the probe returns OutcomeNotApplicable.
remediation:
onOutcome: False
effort: Medium
text:
- Visit the OpenSSF Memory Safety SIG guidance on how to make your project memory safe.
- Guidance for [Memory-Safe By Default Languages](https://github.com/ossf/Memory-Safety/blob/main/docs/best-practice-memory-safe-by-default-languages.md)
- Guidance for [Non Memory-Safe By Default Languages](https://github.com/ossf/Memory-Safety/blob/main/docs/best-practice-non-memory-safe-by-default-languages.md)
ecosystem:
languages:
- go
- c#
clients:
- github
Comment on lines +40 to +41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

    - github
    - gitlab
    - localdir

229 changes: 229 additions & 0 deletions probes/memorysafe/impl.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
// Copyright 2025 OpenSSF Scorecard Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package memorysafe

import (
"embed"
"fmt"
"go/parser"
"go/token"
"reflect"
"strings"

"github.com/ossf/scorecard/v5/checker"
"github.com/ossf/scorecard/v5/checks/fileparser"
"github.com/ossf/scorecard/v5/clients"
"github.com/ossf/scorecard/v5/finding"
"github.com/ossf/scorecard/v5/internal/dotnet/csproj"
"github.com/ossf/scorecard/v5/internal/probes"
)

//go:embed *.yml
var fs embed.FS

const (
Probe = "memorysafe"
)

type languageMemoryCheckConfig struct {
Desc string

funcPointers []func(client *checker.CheckRequest) ([]finding.Finding, error)
}

var languageMemorySafeSpecs = map[clients.LanguageName]languageMemoryCheckConfig{
clients.Go: {
funcPointers: []func(client *checker.CheckRequest) ([]finding.Finding, error){
checkGoUnsafePackage,
},
Desc: "Check if Go code uses the unsafe package",
},

clients.CSharp: {
funcPointers: []func(client *checker.CheckRequest) ([]finding.Finding, error){
checkDotnetAllowUnsafeBlocks,
},
Desc: "Check if C# code uses unsafe blocks",
},
}

func init() {
probes.MustRegisterIndependent(Probe, Run)
}

func Run(raw *checker.CheckRequest) (found []finding.Finding, probeName string, err error) {
prominentLangs := getRepositoryLanguageChecks(raw)
findings := []finding.Finding{}

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

Check warning on line 76 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L73-L76

Added lines #L73 - L76 were not covered by tests
Comment on lines +72 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

langFindings, err := langFunc(raw)
if err != nil {
return nil, Probe, fmt.Errorf("error while running function for language %s: %w", lang.Desc, err)
}
findings = append(findings, langFindings...)
}
}
return findings, Probe, nil
}

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),
})
Comment on lines +87 to +92
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return nil
}
if len(langs) == 0 {
return []languageMemoryCheckConfig{}
}
if len(langs) == 1 && langs[0].Name == clients.All {
return getAllLanguages()
}
ret := []languageMemoryCheckConfig{}
for _, language := range langs {
if lang, ok := languageMemorySafeSpecs[clients.LanguageName(strings.ToLower(string(language.Name)))]; ok {
ret = append(ret, lang)
}
}
return ret
}

func getAllLanguages() []languageMemoryCheckConfig {
allLanguages := make([]languageMemoryCheckConfig, 0, len(languageMemorySafeSpecs))
for l := range languageMemorySafeSpecs {
allLanguages = append(allLanguages, languageMemorySafeSpecs[l])
}
return allLanguages
}

// Golang

func checkGoUnsafePackage(client *checker.CheckRequest) ([]finding.Finding, error) {
findings := []finding.Finding{}

if err := fileparser.OnMatchingFileContentDo(client.RepoClient, fileparser.PathMatcher{
Pattern: "*.go",
CaseSensitive: false,
}, goCodeUsesUnsafePackage, &findings, client.Dlogger); err != nil {
return nil, err
}
if len(findings) == 0 {
found, err := finding.NewWith(fs, Probe,
"Golang code does not use the unsafe package", nil, finding.OutcomeTrue)
Comment on lines +129 to +131
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return nil, fmt.Errorf("create finding: %w", err)
}

Check warning on line 134 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L133-L134

Added lines #L133 - L134 were not covered by tests
findings = append(findings, *found)
}
return findings, nil
}

func goCodeUsesUnsafePackage(path string, content []byte, args ...interface{}) (bool, error) {
findings, ok := args[0].(*[]finding.Finding)
if !ok {
// panic if it is not correct type
panic(fmt.Sprintf("expected type findings, got %v", reflect.TypeOf(args[0])))

Check warning on line 144 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L143-L144

Added lines #L143 - L144 were not covered by tests
}
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, "", content, parser.ImportsOnly)
if err != nil {
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])))

Check warning on line 152 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L151-L152

Added lines #L151 - L152 were not covered by tests
Comment on lines +149 to +152
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

dl.Warn(&checker.LogMessage{
Text: fmt.Sprintf("malformed golang file: %v", err),
})
return true, nil
}
for _, i := range f.Imports {
if i.Path.Value == `"unsafe"` {
found, err := finding.NewWith(fs, Probe,
"Golang code uses the unsafe package", &finding.Location{
Path: path,
}, finding.OutcomeFalse)
Comment on lines +163 to +165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return false, fmt.Errorf("create finding: %w", err)
}

Check warning on line 168 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L167-L168

Added lines #L167 - L168 were not covered by tests
*findings = append(*findings, *found)
}
}

return true, nil
}

// CSharp

func checkDotnetAllowUnsafeBlocks(client *checker.CheckRequest) ([]finding.Finding, error) {
findings := []finding.Finding{}

if err := fileparser.OnMatchingFileContentDo(client.RepoClient, fileparser.PathMatcher{
Pattern: "*.csproj",
CaseSensitive: false,
}, csProjAllosUnsafeBlocks, &findings, client.Dlogger); err != nil {
return nil, err
}
if len(findings) == 0 {
found, err := finding.NewWith(fs, Probe,
"C# code does not allow unsafe blocks", nil, finding.OutcomeTrue)
if err != nil {
return nil, fmt.Errorf("create finding: %w", err)
}

Check warning on line 192 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L191-L192

Added lines #L191 - L192 were not covered by tests
findings = append(findings, *found)
}
return findings, nil
}

func csProjAllosUnsafeBlocks(path string, content []byte, args ...interface{}) (bool, error) {
findings, ok := args[0].(*[]finding.Finding)
if !ok {
// panic if it is not correct type
panic(fmt.Sprintf("expected type findings, got %v", reflect.TypeOf(args[0])))

Check warning on line 202 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L201-L202

Added lines #L201 - L202 were not covered by tests
}
unsafe, err := csproj.IsAllowUnsafeBlocksEnabled(content)
if err != nil {
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])))

Check warning on line 209 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L208-L209

Added lines #L208 - L209 were not covered by tests
Comment on lines +206 to +209
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about moving type check

}

dl.Warn(&checker.LogMessage{
Text: fmt.Sprintf("malformed csproj file: %v", err),
})
return true, nil
}
if unsafe {
found, err := finding.NewWith(fs, Probe,
"C# code allows the use of unsafe blocks", &finding.Location{
Path: path,
}, finding.OutcomeFalse)
if err != nil {
return false, fmt.Errorf("create finding: %w", err)
}

Check warning on line 224 in probes/memorysafe/impl.go

View check run for this annotation

Codecov / codecov/patch

probes/memorysafe/impl.go#L223-L224

Added lines #L223 - L224 were not covered by tests
*findings = append(*findings, *found)
}

return true, nil
}
Loading
Loading