-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
Change https://go.dev/cl/466756 mentions this issue: |
cc @golang/compiler @golang/runtime |
If 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 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 I am also curious as to how this cases would be different from something like
For the code above, the linker would dead-code away Vuln since it is not referenced. |
Suppose that
The difference here is that |
@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). |
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>
I have a slightly different take on the response for this. Suppose that only 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. |
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>
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>
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>
Consider the following illustrative program where
bvuln.Vuln
is a vulnerable symbol used at a vulnerable version: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. Sincedebug
is provablytrue
, this optimization step will conclude that 1) the code afterif-then
is unreachable and 2) the actualif-then
can be reduced to areturn
. The actual call toVuln
then can be optimized to a no-op. There are no instructions in the binary whose source isVuln
and hence there is no way currently to detect the existence ofVuln
.Why is this an issue?
The vulnerability can be caused precisely because
debug
is mistakenly set to true, so the code afterif-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
The text was updated successfully, but these errors were encountered: