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/cmd/govulncheck: include the source code location of init #51575

Closed
hyangah opened this issue Mar 9, 2022 · 10 comments
Closed

x/vuln/cmd/govulncheck: include the source code location of init #51575

hyangah opened this issue Mar 9, 2022 · 10 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. vulncheck or vulndb Issues for the x/vuln or x/vulndb repo

Comments

@hyangah
Copy link
Contributor

hyangah commented Mar 9, 2022

In #51565
the govulncheck output is missing the exact source code location when init functions are involved.

$ govulncheck ./...
Findings for vulnerability: GO-2021-0113 (CVE-2021-38561):

Trace:
golang.org/x/text/language.MustParse (/Users/hakim/go/pkg/mod/golang.org/x/text@v0.3.6/cases/map.go:43:41)
golang.org/x/text/cases.init#1(...) (-)
golang.org/x/text/cases.init(...) (-)
golang.org/x/text/secure/precis.init(...) (-)
github.com/jackc/pgconn.init(...) (-)
golang.org/x/build/internal/relui/db.init(...) (-)

If the output presented the call site location of x/text/language.MustParse (in x/text/cases),
users could inspect how the problematic symbol is used more easily.

@gopherbot gopherbot added this to the Unreleased milestone Mar 9, 2022
@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 14, 2022
@heschi
Copy link
Contributor

heschi commented Mar 14, 2022

cc @jba @zpavlinovic

@zpavlinovic
Copy link
Contributor

Sorry for not responding to this earlier, we are actually still thinking what to do about inits. We cannot get their location in most cases as they often do not actually appear in the source code.

If the output presented the call site location of x/text/language.MustParse (in x/text/cases), users could inspect how the problematic symbol is used more easily.

I can see the trace has the location of where MustParse is called. It looks like it is pointing at this.

@timothy-king
Copy link
Contributor

Reading the stack trace from the bottom to the top I think this is saying:

package golang.org/x/build/internal/relui/db directly imports
package github.com/jackc/pgconn directly imports
package golang.org/x/text/secure/precis directly imports
package golang.org/x/text/cases which runs an initializer function
golang.org/x/text/cases.init( )at maps.go:40 which calls
golang.org/x/text/language.MustParse

Hope that makes sense. @hyangah given the above explanation let me ask, what do you wish was printed? Once we know that we can figure out what metadata could provide this/is feasible.

For reference, in the x/tools/go/ssa package that govulncheck is built from cases.init(...) means the synthetic init() function for the package. These are built during compilation and don't on their own have source positions. This function initializes package variables (which we have locations for), calls any declared/explicit init() functions in the source (many init() functions can be in the source of a package), e.g. like cases.init#1(...) in map.go, and calls the synthetic init functions of the direct package deps. For init() functions in the source we should have the decl and that should have the source position info. If not, I suspect it is an easy fix. If we don't have source position for how a package is imported, we may be able to iterate over the AST and find some location in the source it is imported. I don't know if it matters much which package import location is selected. Often there is package import is one per file. (@hyangah thoughts on which to present?)

@hyangah
Copy link
Contributor Author

hyangah commented Mar 15, 2022

For init() functions in the source we should have the decl and that should have the source position info. If not, I suspect it is an easy fix. If we don't have source position for how a package is imported, we may be able to iterate over the AST and find some location in the source it is imported. I don't know if it matters much which package import location is selected. Often there is package import is one per file.

@timothy-king Using the import statement location of the problematic package seems like a reasonable choice. I used a similar heuristic (iterating over ASTs to find import statements) when converting such govulncheck traces into LSP diagnostics messages where position info is necessary.

Some corner cases to think about though: in the following example,

...
golang.org/x/text/cases.init#1(...) (-)
golang.org/x/text/cases.init(...) (-)
golang.org/x/text/secure/precis.init(...) (-)
...
  • golang.org/x/text/cases.init(...) is presented with the import statement inside the source code of golang.org/x/text/secure/precis package.
  • what location info should be presented for golang.org/x/text/cases.init#1(...)? One option is the packacage statement.

When multiple files exist, arbitrarily choose one of them seems like a reasonable choice.

@zpavlinovic What do you think about placing the location info next the calling function name's row?
I thought that may look more consistent with dumps generated by dynamic tracing stack traces,
but static analysis trace may have different shapes.

If the position information was displayed with the calling function names,
that would look like the following:
(note: I made up the position info of language.MustParse in this example`.
Not sure if govulncheck internally keeps track of the info)

golang.org/x/text/language.MustParse (/modcache/golang.orgx/text@v0.3.6/language/tags.go:13:0)
golang.org/x/text/cases.init#1(...) (/modcache/golang.org/x/text@v0.3.6/cases/map.go:43:41)
golang.org/x/text/cases.init(...) (-)
golang.org/x/text/secure/precis.init(...) (-)
github.com/jackc/pgconn.init(...) (-)
golang.org/x/build/internal/relui/db.init(...) (-)

If the entry point is an init function, maybe the package statement location in the file where the problematic import happens is one option.

@zpavlinovic
Copy link
Contributor

@zpavlinovic What do you think about placing the location info next the calling function name's row? I thought that may look more consistent with dumps generated by dynamic tracing stack traces, but static analysis trace may have different shapes.

...

If I understand correctly, the suggestion is to put the next to each call the location of the function being called rather than the location of the call itself?

@jba @julieqiu @rolandshoemaker

@hyangah
Copy link
Contributor Author

hyangah commented Mar 16, 2022

@zpavlinovic That's correct.

I am not sure which one is better yet. When I click the link (some editors/terminals auto-linkify the file paths), I somehow expected it will land in a location (callsite of the next entry) inside the mentioned function (based on other crash or goroutine traces)
Maybe I will be used to this.

@zpavlinovic
Copy link
Contributor

zpavlinovic commented Mar 16, 2022

Perhaps we should show both: the location of the function and the call itself. Something like this.

X [loc of X]
   callsite to X [loc of callsite]
 ...
B [loc of B]
    callsite to B [loc of callsite]
A [loc of A]

Call site location is useful because a function A can call function B at several different spots. Another useful thing here is that both static and dynamic callsites can be handled the same way (currently we use a different syntax for dynamically resolved callsites using -->)

We could do away potentially with [loc of callsite] if we show callsite in its original source code representation (which might be tricky as we are working on SSA level). This way the users still get enough info to navigate around and we reduce clutter.

@zpavlinovic zpavlinovic changed the title x/exp/vulndb/govulncheck: include the source code location of init x/vuln/cmd/govulncheck: include the source code location of init Mar 24, 2022
@gopherbot gopherbot added the vulncheck or vulndb Issues for the x/vuln or x/vulndb repo label Mar 24, 2022
@zpavlinovic
Copy link
Contributor

Note: the new version of govulncheck is now in x/vuln/cmd/govulncheck. The previous version is not supported anymore and has been deleted.

@hyangah
Copy link
Contributor Author

hyangah commented Sep 14, 2022

In the new version of govulncheck, init is presented like this:

      #4: for function AppendBooleanElement
        github.com/hashicorp/vault/plugins/database/mongodb.init
            -
        go.mongodb.org/mongo-driver/mongo.init
            -
        go.mongodb.org/mongo-driver/x/mongo/driver/auth.init
            /Users/hakim/go/pkg/mod/go.mongodb.org/mongo-driver@v1.4.6/x/mongo/driver/auth/scram.go:35:32
        go.mongodb.org/mongo-driver/x/bsonx/bsoncore.AppendBooleanElement

When integrating govulncheck in linter or IDE, we still need to decide what location we can associate this info with. The natural place I can think of is the import & package statement.
For example, it would make our job and maybe user's job easier if the first entry github.com/hashicorp/vault/plugins/database/mongodb.init can be associated with the package statement of the file where go.mongodb.org/mongo-driver/mongo is imported, or the import statement.

@zpavlinovic zpavlinovic self-assigned this Sep 14, 2022
@gopherbot
Copy link

Change https://go.dev/cl/442499 mentions this issue: internal/govulncheck: add position information for inits and their calls

softdev050 added a commit to softdev050/Golangvuln that referenced this issue Apr 5, 2023
Every P.init function with no position is assigned the position of
some "package P" statement.

For P1.init calls P2.init case, position of the call is set to the
position of some "import P2" statement in P1.

For P.init calls P.init#d case, (P.init is implicit and) the position
of the call to P.init#d is the position of some "package P" statement.

Also refactors some code so that testing facilities can be reused and
fixes some outdated documentation.

Fixes golang/go#51575

Change-Id: Ib38dc396962353db8cc33e407ac7131a49d122c2
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/442499
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
sayjun0505 added a commit to sayjun0505/Golangvuln that referenced this issue Apr 8, 2023
Every P.init function with no position is assigned the position of
some "package P" statement.

For P1.init calls P2.init case, position of the call is set to the
position of some "import P2" statement in P1.

For P.init calls P.init#d case, (P.init is implicit and) the position
of the call to P.init#d is the position of some "package P" statement.

Also refactors some code so that testing facilities can be reused and
fixes some outdated documentation.

Fixes golang/go#51575

Change-Id: Ib38dc396962353db8cc33e407ac7131a49d122c2
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/442499
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
stanislavkononiuk added a commit to stanislavkononiuk/Golangvuln that referenced this issue Jun 26, 2023
Every P.init function with no position is assigned the position of
some "package P" statement.

For P1.init calls P2.init case, position of the call is set to the
position of some "import P2" statement in P1.

For P.init calls P.init#d case, (P.init is implicit and) the position
of the call to P.init#d is the position of some "package P" statement.

Also refactors some code so that testing facilities can be reused and
fixes some outdated documentation.

Fixes golang/go#51575

Change-Id: Ib38dc396962353db8cc33e407ac7131a49d122c2
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/442499
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
@golang golang locked and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
Status: Done
Development

No branches or pull requests

6 participants