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/vuln/vulncheck: missing empty inlined vulnerable functions #58509

Open
zpavlinovic opened this issue Feb 13, 2023 · 6 comments
Open

x/vuln/vulncheck: missing empty inlined vulnerable functions #58509

zpavlinovic opened this issue Feb 13, 2023 · 6 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Milestone

Comments

@zpavlinovic
Copy link
Contributor

zpavlinovic commented Feb 13, 2023

Consider the following illustrative program where bvuln.Vuln is a vulnerable symbol used at a vulnerable version:

package main
                        
import (
   "golang.org/bmod/bvuln"
)
        
func main() {
   bvuln.Vuln()
}
  
// ---------------------------------------------------------------
package bvuln
        
const debug = true
        
func Vuln() {
   if debug {
      return
    }
    ... // some important code, such as ACL check
}

What did you expect to see?

vulncheck and govulncheck binary analysis to detect Vuln.

What did you see instead?

Nothing detected.

The suspected and likely reason for this is that Vuln gets inlined and its whole body is deleted due to the constant propagation pass. Since debug is provably true, this optimization step will conclude that 1) the code after if-then is unreachable and 2) the actual if-then can be reduced to a return. The actual call to Vuln then can be optimized to a no-op. There are no instructions in the binary whose source is Vuln and hence there is no way currently to detect the existence of Vuln.

Why is this an issue?

The vulnerability can be caused precisely because debug is mistakenly set to true, so the code after if-then is never triggered. That could be code that, say, does some ACL checking.

Is there a potential fix?

The compiler could leave a no-op instruction in the binary whose parent is Vuln. This is done for some other constructs.

Thanks to @prattmic for bringing this up.

cc @jba

@zpavlinovic zpavlinovic added the vulncheck or vulndb Issues for the x/vuln or x/vulndb repo label Feb 13, 2023
@zpavlinovic zpavlinovic added this to the Unplanned milestone Feb 13, 2023
@gopherbot
Copy link

Change https://go.dev/cl/466756 mentions this issue: vulncheck/internal: remove noinline from unit tests

@zpavlinovic zpavlinovic changed the title x/vuln/vulncheck: missing fully inlined vulnerable functions x/vuln/vulncheck: missing empty inlined vulnerable functions Feb 14, 2023
@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 14, 2023
@prattmic
Copy link
Member

cc @golang/compiler @golang/runtime

@thanm
Copy link
Contributor

thanm commented Feb 14, 2023

If debug is true, then how can Vuln be a vulnerable function? Its body will be empty (even before the compiler writes out the inlinable body to the bvuln export data), I don't see any way for it to cause bad behavior.

For a given function, it seems possible that there will some versions of the function that are safe (no vulnerabilities), and then some versions that are unsafe. If I take the bvuln package as you've written it and publish version X, then it seems pretty clear that there isn't a vulnerability in function Vuln at version X.

Presumably if the "debug=true" version is published as version X, then the package author changes "debug" to false and publishes a new version X+1, then clearly in that new version Vuln needs to be flagged as bad (but you will also no longer have the 'inlined away' problem.

I am also curious as to how this cases would be different from something like

package main
const debug = false
func main() {
   if debug {
     bvuln.Vuln()    // Vuln not inlinable
   }
}

For the code above, the linker would dead-code away Vuln since it is not referenced.

@prattmic
Copy link
Member

prattmic commented Feb 15, 2023

If debug is true, then how can Vuln be a vulnerable function? Its body will be empty (even before the compiler writes out the inlinable body to the bvuln export data), I don't see any way for it to cause bad behavior.

Suppose that Vuln is named PanicIfBadGuy. Its purpose is to check whether the current request is malicious and stop processing if so. But in the latest release, the developer accidentally left debug = True, which bypasses all checks in PanicIfBadGuy and allows all requests through. PanicIfBadGuy is vulnerable because it doesn't perform the checks its documentation says it would.

I am also curious as to how this cases would be different from something like

package main
const debug = false
func main() {
   if debug {
     bvuln.Vuln()    // Vuln not inlinable
   }
}

The difference here is that main is vulnerable, not Vuln. Vuln still works fine, but main has a vulnerability because it is missing the call to Vuln. The x/vulndb report should list main as the vulnerable symbol rather than Vuln.

@thanm
Copy link
Contributor

thanm commented Feb 15, 2023

@mpratt thanks for the additional context, I think it makes a bit more sense now.

Not sure about the implications of adding extra nops for inolined funcs that are optimized entirely away though. There are places in the runtime where we're assuming that an inlined-away call is very low cost (for example, the hooks for static lock ranking).

gopherbot pushed a commit to golang/vuln that referenced this issue Feb 15, 2023
Inlining should be supported now.

Updates golang/go#58509

Change-Id: I43cdb59f7468538ede30ff993bf76bebf0424583
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/466756
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
@zpavlinovic
Copy link
Contributor Author

zpavlinovic commented Feb 15, 2023

I am also curious as to how this cases would be different from something like

package main
const debug = false
func main() {
   if debug {
     bvuln.Vuln()    // Vuln not inlinable
   }
}

For the code above, the linker would dead-code away Vuln since it is not referenced.

I have a slightly different take on the response for this. Suppose that only bvuln.Vuln is deemed vulnerable by a vulnerability database, i.e., main is not. I think it is fine not to report bvuln.Vuln here because it is in fact never called.

Ideally, the vulncheck binary analysis would also do a call-graph level reasoning similar to what the vulncheck source analysis is doing, since the goal of vulncheck is to report vulnerable functions/methods actually called by programs. But reconstructing the call graph from binaries is out of vulncheck's scope so the compiler optimization here is actually helping vulncheck's precision.

Update: actually, this is a nice example of where vulncheck's binary analysis is a bit more precise than the current source analysis. The latter uses a call graph algorithm that does not perform constant propagation and dead code elimination.

softdev050 added a commit to softdev050/Golangvuln that referenced this issue Apr 5, 2023
Inlining should be supported now.

Updates golang/go#58509

Change-Id: I43cdb59f7468538ede30ff993bf76bebf0424583
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/466756
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
sayjun0505 added a commit to sayjun0505/Golangvuln that referenced this issue Apr 8, 2023
Inlining should be supported now.

Updates golang/go#58509

Change-Id: I43cdb59f7468538ede30ff993bf76bebf0424583
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/466756
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
stanislavkononiuk added a commit to stanislavkononiuk/Golangvuln that referenced this issue Jun 26, 2023
Inlining should be supported now.

Updates golang/go#58509

Change-Id: I43cdb59f7468538ede30ff993bf76bebf0424583
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/466756
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
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. vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
None yet
Development

No branches or pull requests

4 participants