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/cmd/govulncheck: type propagation can produce confusing results #47121
Comments
@timothy-king (cc'ing myself to follow along and make my email filters happy) |
(Partly discussed offline) The issue happens because of VTA. VTA sees that EntryB calls EntryA with a specific input and hence concludes that There are several potential ways to go around this issue:
|
It is correct to report Some form of 3 seems the most appealing to me in the long run. Conceptually we could remove the "approx" in the report by locally knowing that the set of implementations of A fourth option could also be to make the report of the approx step clearer. (Probably requires tracking more data too.) |
I'm not sure this is correct, as shown by the fact that nothing is reported if This may touch on a previous decision we came to (as far as I remember), which was that if an entry point can take an interface which matches a vulnerable implementation, but does not actually receive that type anywhere in the analyzed code, we ignore it. |
I like (2) in theory, but I think in more complex applications it won't be as valuable. For instance in a case where we don't have this strange type propagation and there is a vulnerability in EntryA that has nothing to do with an interface, we probably do want to report the smaller chain. It seems like some combination of (1) and (2) might be viable, where entry points that contain other entry points as subtrees of their call graphs have VTA run independently, so that they aren't contaminating each others results? So for example in this case we'd slice the graph such that VTA is run twice on (EntryB->EntryA->F->...) and (EntryA->F). |
Not sure if I agree so let's try to figure out where the disagreement is. We need to allow flows of interfaces and func types. There is no guarantee this is doable in a single call sequence. Suppose we added:
This is vulnerable and again goes through What do you think a correct report should be for EntryC? (Maybe there is a something I am missing?) |
As I see it (and is currently implemented) We could say "this could be vulnerable, but only if you do something that you're not currently doing", but this seems to go against the whole point of call graph analysis, and VTA in particular. We know it isn't being called, so why create noise? I think this issue shows VTA is working as intended, but we're just shadowing the real result because the |
I think I understand. Here is my attempt at a summary:
Basically the idea is that I am not sure the author of Thoughts? Tangents:
|
This is a complex question, for which I think there are two different cases, whether they import the vulnerable module/package ( I think the former is obviously no, it would require The latter is more complex, there are a lot of things we could do to determine whether it is possible (i.e. by checking for things like your |
The vulnerability starting from EntryB will not be reported since we do not visit a function more than once to avoid analyzing too many paths. That is, we do a BFS search so we'll analyze EntryA when we start with entry points and when we see EntryA called from EntryB, we'll skip analyzing EntryA again. As @rolandshoemaker suggested, the two less invasive options we have right now are based on splitting entry points into two groups: one that are called from other entry points and those that are not. Then, either
or even
I've already experimented with 2) and there is some performance penalty but the implementation I have can be made faster IMO. |
Can graph based heuristics suffice if this is the main problem? Something like start BFS from each node starting from a topologically sorting of the entry points [according to some call graph]. Or weighted search and charge based on the topological order? |
Lets call entry points that are called by other entry points secondary entry points just to refer to them more easily.
IMO, this is ok for now since it is still a valid and clear trace that just might contain some redundancy.
|
Note: the new version of govulncheck is now in x/vuln/cmd/govulncheck. The previous version is not supported anymore and has been deleted. |
Type propagation appears to taint some functions in the call graph, causing them to be reported as vulnerable, sometimes shadowing the actual results.
The example below is a quite basic example of this. Because
VulnerableImplementation
flows fromEntryB
throughEntryA
, the latter is reported as vulnerable, shadowingEntryB
, since we try to only show a single finding per vulnerability. RemovingEntryB
causes nothing to be reported.I suspect this will also lead to more confusing results, where it is less obvious how the type is being propagated, tainting a function.
cc @zpavlinovic
The text was updated successfully, but these errors were encountered: