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

Add cargo auditable deps extractor for Rust bins to SCALIBR #377

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

copybara-service[bot]
Copy link

Add cargo auditable deps extractor for Rust bins to SCALIBR

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 8, 2025

@another-rex @oliverchang fyi, might want to link this with google/osv-scanner#1332 if that's not already happened internally

Copy link

@Shnatsel Shnatsel left a comment

Choose a reason for hiding this comment

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

Hey, author of cargo auditable here! 👋

I've skimmed the PR and left a couple of notes, but by and large it looks good. I'm excited to see cargo auditable data becoming accessible to osv-scanner!


// TODO(b/279138598): Research: Maybe on windows all files have the executable bit set.
// Either windows .exe or unix executable bit should be set.
if filepath.Ext(path) != ".exe" && fileInfo.Mode()&0111 == 0 {
Copy link

Choose a reason for hiding this comment

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

.dll files can also built in Rust with cargo auditable, although Rust shared libraries are less common than Rust executables. It is probably a good idea to load DLL files as well.

Choose a reason for hiding this comment

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

Thanks for the review! This sounds like a great point, I went ahead and added the exception for .dlls as well, thanks!

Comment on lines 152 to 148
// Cargo auditable also tracks build dependencies which we don't want to report.
if dep.Kind == rustaudit.Runtime {
inventory = append(inventory, &extractor.Inventory{
Name: dep.Name,
Version: dep.Version,
Locations: []string{input.Path},
})
}
Copy link

Choose a reason for hiding this comment

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

This is probably a reasonable default, although it would be nice to make this configurable eventually (not necessarily in this PR).

The use case I had in mind for this was e.g. protobuf codegen or a Rust proc macro emitting wrong memory-unsafe code which ends up in the final binary, even though the problematic code was only a build-dependency. But these issues are admittedly rare.

Choose a reason for hiding this comment

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

Good idea, I hadn't considered that possibility so far. I went ahead and added in the suggested configuration. Thanks!

@copybara-service copybara-service bot force-pushed the test_713069568 branch 5 times, most recently from c2a68f5 to b22b686 Compare January 15, 2025 19:04
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.

3 participants