advisory-database icon indicating copy to clipboard operation
advisory-database copied to clipboard

Rebuttal for GHSA-27rq-4943-qcwp (CVE-2022-29810)

Open picatz opened this issue 2 years ago • 3 comments

👋 Hello!

I work on the Product Security Team at HashiCorp that handles the vulnerability management and response process. I am reaching out on behalf of my team in response to the recently published GHSA-27rq-4943-qcwp (CVE-2022-29810).

The vulnerability description states:

The Hashicorp go-getter library before 1.5.11 could write SSH credentials into its logfile, exposing sensitive credentials to local users able to read the logfile.

The related pull request linked in the advisory added a new feature to the RedactURL function provided by go-getter, redacting the "sshkey" URL query parameter, in addition to the "password" URL query parameter. Before the change introduced in the PR, the function explicitly stated that only "password" would be redacted. Moreover, the go-getter library does not have a log file which SSH credentials, or anything else, can be written to.

We believe the advisory (and the CVE we plan to send a rebuttal for to NVD) is factually inaccurate, and should be removed. From previous interactions with NVD, a public rebuttal is required for updating CVE content, and this issue also serves that use case.

We are looking forward to your response, and please let us know if you require further clarification.

picatz avatar May 09 '22 20:05 picatz

Hi @picatz. We're looking at this now. Please could you help me understand a few details?

In the pull request description, it says "Redact SSH key from URL query parameter when printing the URL after a download error happens", which sounds a bit like it's fixing a bug (or possibly a vulnerability). However, if I have understood you correctly, it's not a bug in practice because RedactURL is a utility function that is never used in a security-sensitive way. For example, according to this comment, HashiCorp doesn't use RedactURL. So I think you're saying that this would only be a vulnerability in the hypothetical scenario where somebody pipes the output of RedactURL into a log file, but that's not how RedactURL is intended to be used, therefore it's not a vulnerability. Does that sound correct to you?

kevinbackhouse avatar May 17 '22 21:05 kevinbackhouse

sound correct but with the bot updated it seems to work well

marquisinpro avatar May 18 '22 07:05 marquisinpro

So I think you're saying that this would only be a vulnerability in the hypothetical scenario where somebody pipes the output of RedactURL into a log file, but that's not how RedactURL is intended to be used, therefore it's not a vulnerability. Does that sound correct to you?

👍 Correct, mostly! It's a bit nuanced, but I hope this context will help.

In https://github.com/hashicorp/go-getter/pull/319 an external contributor used the Redacted method, coming from the net/url package, for the first time in go-getter. This method was added in Go 1.15, breaking our usage in other places at the time.

https://github.com/hashicorp/go-getter/pull/336 attempted to fix this for our usage related to a Consul release, copying the method into the go-getter library, retaining the exact same functionality.

https://github.com/hashicorp/go-getter/pull/348 expanded the scope of the RedactURL function, which previously only redacted password from HTTP username/password combinations. Any other secrets that could exist as URL query parameters, or anywhere else in the URL, are not redacted by this function, as expected. Passing secrets this way is not preferred, but is supported.

To be clear here: an error from the go-getter library can contain the URL associated with it. It is up to the caller of the library what to do with the error for their situation.

picatz avatar May 19 '22 19:05 picatz

Hi @picatz,

I'm sorry I forgot about this issue for so long. I'd like to close this issue now, if that's ok with you. Here are my reasons:

  1. The CVE was issued by NIST so we (GitHub) don't have the power to revoke it (although I do understand that you opened this issue because a public rebuttal is required for NIST to update the CVE).
  2. I believe it's better to warn users just in case, even if the chance that they're affected is low, so it does no harm for the issue to have a CVE.
  3. Because this issue is in a library, whether a user is affected depends on how they use the library, so it's quite a subtle issue to explain. Therefore, for most users, it's easiest to just upgrade and not worry about it.
  4. The issue is almost a year old now, so many users have probably upgraded by now.

Of course, if the CVE gets revoked then we will also revoke it in our database. By the way, one of my colleagues suggested that you might have better luck getting it revoked if you try contacting MITRE rather than NVD.

Kind regards,

Kev

kevinbackhouse avatar Mar 09 '23 17:03 kevinbackhouse