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

Replace directive is based on the to be replaced module path not the one which replacing it #256

Closed
SzekeresB opened this issue Nov 12, 2021 · 4 comments · Fixed by #257
Closed

Comments

@SzekeresB
Copy link

SzekeresB commented Nov 12, 2021

Thanks for creating an issue! Please fill out this form so we can be
sure to have all the information we need, and to minimize back and forth.

  • What are you trying to do?
    I'm trying to scan for dependency issues, add mitigation to them and rescan for result nancy repot seems incorrect.

Here's an example, so you could also recreate the issue.

Clone out a repo that contains indirect dependency, try to replace it with an equivalent fork, and look for the results.

Just to make it more visible I prepared an example.
The repo is: https://github.com/oauth2-proxy/oauth2-proxy
A good candidate for the demonstration is the v7.1.2 tag.
So the commands so far:

~ git clone https://github.com/oauth2-proxy/oauth2-proxy.git
~ cd oauth2-proxy
~ git checkout v7.1.2

So If everything is right you are in this commit:

commit 9d20b4e0e2210515219669bd42d99f653fc4201b (HEAD, tag: v7.1.2)
Merge: 7ebeecb fbe5743
Author: Joel Speed <Joel.speed@hotmail.co.uk>
Date:   Fri Apr 2 11:34:59 2021 +0100

    Merge pull request #1145 from oauth2-proxy/release-v7.1.2

    Update Changelog for release v7.1.2

Running nancy against it reveals an issue:

go list -json -deps | docker run --rm -i sonatypecommunity/nancy:v1.0.27 sleuth -o json

I'm not gonna copy past the full report the relevant part is it found the issue (that's good)
The issue we are looking for is :

 "vulnerable": [
        {
            "Coordinates": "pkg:golang/github.com/dgrijalva/jwt-go@3.2.0",
            "Reference": "https://ossindex.sonatype.org/component/pkg:golang/github.com/dgrijalva/jwt-go@3.2.0?utm_source=nancy-client\u0026utm_medium=integration\u0026utm_content=1.0.27",
            "Vulnerabilities": [
                {
                    "ID": "c16fb56d-9de6-4065-9fca-d2b4cfb13020",
                    "Title": "[CVE-2020-26160] jwt-go before 4.0.0-preview1 allows attackers to bypass intended access restrict...",
                    "Description": "jwt-go before 4.0.0-preview1 allows attackers to bypass intended access restrictions in situations with []string{} for m[\"aud\"] (which is allowed by the specification). Because the type assertion fails, \"\" is the value of aud. This is a security problem if the JWT token is presented to a service that lacks its own audience check.",
                    "CvssScore": "7.5",
                    "CvssVector": "CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N",
                    "Cve": "CVE-2020-26160",
                    "Reference": "https://ossindex.sonatype.org/vulnerability/c16fb56d-9de6-4065-9fca-d2b4cfb13020?component-type=golang\u0026component-name=github.com%2Fdgrijalva%2Fjwt-go\u0026utm_source=nancy-client\u0026utm_medium=integration\u0026utm_content=1.0.27",
                    "Excluded": false
                }
            ],
            "InvalidSemVer": false
        }
    ]

Let's make some mitigation, I would like to replace the github.com/dgrijalva/jwt-go dependency with github.com/golang-jwt/jwt/v4 v4.1.0.

For that, I add the replace directive but It would fail unless I also add the github.com/golang-jwt/jwt/v4 with a different version to the required part of the code. This issue is probably on the golang side nothing to do with Nancy.

So after applying the workaround to replace dgrijalva/jwt-go with github.com/golang-jwt/jwt/v4 in indirect dependiencies
The below golang error should disappear:

exit status 1: stderr: go: github.com/golang-jwt/jwt/v4@v4.1.0 used for two different module paths (github.com/dgrijalva/jwt-go and github.com/golang-jwt/jwt/v4)

This means the diff should look like this for go.mod:

diff --git a/go.mod b/go.mod
index 9cb43c0..97946a8 100644
--- a/go.mod
+++ b/go.mod
@@ -13,6 +13,7 @@ require (
        github.com/fsnotify/fsnotify v1.4.9
        github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32
        github.com/go-redis/redis/v8 v8.2.3
+       github.com/golang-jwt/jwt/v4 v4.0.0 // indirect
        github.com/google/uuid v1.2.0
        github.com/justinas/alice v1.2.0
        github.com/mbland/hmacauth v0.0.0-20170912233209-44256dfd4bfa
@@ -37,3 +38,5 @@ require (
        gopkg.in/square/go-jose.v2 v2.4.1
        k8s.io/apimachinery v0.19.3
 )
+
+replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt/v4 v4.1.0

Hit go mod tidy so go.sum would catch up.

Now we have mitigation in place so nancy should be fine, let's run it again.

go list -json -deps | docker run --rm -i sonatypecommunity/nancy:v1.0.27 sleuth -o json

The report reveals the following, it indeed does not have any vulnerability: "vulnerable": [] But these coordinates seems odd:

         {
            "Coordinates": "pkg:golang/github.com/dgrijalva/jwt-go@4.1.0",
            "Reference": "https://ossindex.sonatype.org/component/pkg:golang/github.com/dgrijalva/jwt-go@4.1.0?utm_source=nancy-client\u0026utm_medium=integration\u0026utm_content=1.0.27",
            "Vulnerabilities": [],
            "InvalidSemVer": false
        },

It is so odd that the github.com/dgrijalva/jwt-go only has preview-4.0.0 release as of today. So finding the vulnerability in the 4.1.0 would be an interesting achievement.

  • What feature or behavior is this required for?
    Normal functioning.
  • How could we solve this issue? (Not knowing is okay!)
    Probably fine tune the recently introduced replace directive a bit.
    Respect go mod replace directive #255
  • Anything else?
    I think the example that I provided covers everything.
    cc @bhamail / @DarthHater
@bhamail
Copy link
Contributor

bhamail commented Nov 12, 2021

Very interesting. To make sure I'm following correctly, this example implies an odd mismatch of path and version.

We would expect nancy to produce a purl path/version like:
github.com/golang-jwt/jwt/v4 4.1.0
but instead we are getting:
github.com/dgrijalva/jwt-go 4.1.0
No such release exists, so of course we find zarro boogs in the phantom release.

I think I see a solution -> when we encounter a replace directive, we need to also re-read the Path (not only the Version) from the replace block to cover this case where both the version AND the path are being replaced.

@SzekeresB
Copy link
Author

Yes, you followed it correctly.
I would also expect the github.com/golang-jwt/jwt/v4 4.1.0 in the coordinates of the report, not the github.com/dgrijalva/jwt-go 4.1.0.

The example was there so we could have a common understanding about the issue, and to help reproduce/check if the future fix is sufficient.

@bhamail
Copy link
Contributor

bhamail commented Nov 15, 2021

@SzekeresB Thanks for verifying. Could you give a quick look over PR #257 ?

@SzekeresB
Copy link
Author

@bhamail I looked it over. The PR is looks good for me, (but I'm not familiar with nancy's code). I even checked your branch out, built it and run some dependency check with it. The reports now looks good for me (I don't see anything odd with them).

Thanks for the fast fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants