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

x/vulndb: govulncheck surfaces function calls with trusted input (e.g., GO-2022-1039) #56099

Open
ghost opened this issue Oct 7, 2022 · 7 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo

Comments

@ghost
Copy link

ghost commented Oct 7, 2022

Report ID

GO-2022-1039

Suggestion/Comment

The content of the warning is:

calls regexp.MustCompile, which eventually calls regexp/syntax.Parse

My code does indeed call MustCompile, but all of the non-test call sites pass in a literal string to the regex. As such, the call is trusted. If I undestand this vulnerability report correctly, it effectively means we can't ever use regexes and get a clean bill of health from govulncheck.

package main

import (
	"fmt"
	"regexp"
)

var re = regexp.MustCompile("trusted")

func main() {
	fmt.Println(re.String())
}
@tatianab tatianab transferred this issue from golang/vulndb Oct 7, 2022
@gopherbot gopherbot added the vulncheck or vulndb Issues for the x/vuln or x/vulndb repo label Oct 7, 2022
@gopherbot gopherbot modified the milestones: Unreleased, vuln/unplanned Oct 7, 2022
@ghost
Copy link
Author

ghost commented Oct 7, 2022

I was able to confirm that upgrading to both go1.18.7 and go1.19.2 fixes this.

@tatianab
Copy link

tatianab commented Oct 7, 2022

Hi Yuval, thanks for filing an issue! You're absolutely correct that govulncheck will always surface this vulnerability (unless you update to the latest Go version, as you just did), whether the call is safe or not.

This is a tricky problem because there are so many ways that a certain call could be safe or unsafe, some of which are automatically detectable and some of which are not. So, for now, we mark calls as potentially vulnerable and leave it up to humans to decide if the vulnerability applies to their code.

I've transferred this issue to the main issue tracker, and the team will discuss this further. We're also open to ideas or suggestions.

@tatianab tatianab changed the title x/vulndb: suggestion regarding GO-2022-1039 x/vulndb: govulncheck surfaces function calls with trusted input (e.g., GO-2022-1039) Oct 7, 2022
@zpavlinovic
Copy link
Contributor

zpavlinovic commented Oct 7, 2022

As @tatianab pointed out, govulncheck will report this vulnerability whenever it concludes that regexp/syntax.Parse is called. It does not have a mechanism for figuring out if the call arguments are trusted.

Would some form of warning suppression help in your case? If so, would that be a code annotation, or a config file you'd be willing provide to govulncheck, or perhaps something entirely different?

@ghost
Copy link
Author

ghost commented Oct 7, 2022

Thanks for the quick reply, both!

Yes, warning suppression would be totally fine. I think the easiest from a user's perspective would be an annotation/comment directly in the code.

@zpavlinovic
Copy link
Contributor

Thank you for your feedback! This will help us in planning for future iterations of govulncheck.

@ghost
Copy link
Author

ghost commented Oct 7, 2022

In case it's helpful (re "we mark calls as potentially vulnerable and leave it up to humans to decide", above): my use case is that I wanted to run govulncheck in a GitHub action for pull requests. So having a human look over the vulnerability initially is totally fine (expected, even), but having to do that each time effectively turns the action into a false-alarm generator that we'd quickly learn to ignore.

One option would be for me to get the results as json (which I know govulncheck supports), and then examine that to effectively do my own suppressions. Those would probably have to be at the file level, but it'd be better than nothing. But, having it built into the tool would be better still. :-)

Thanks again!

@joedian joedian added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2022
@joedian joedian modified the milestones: vuln/unplanned, Backlog Oct 10, 2022
@zpavlinovic zpavlinovic added UX Issues that involve UXD/UXR input and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 12, 2022
@joedian joedian added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
None yet
Development

No branches or pull requests

5 participants
@zpavlinovic @tatianab @gopherbot @joedian and others