An update on 0day CVE-2021-43798: Grafana directory traversal | Hacker News Reader

An update on 0day CVE-2021-43798: Grafana directory traversal

grafana.com

95 pointsep_jhu23 comments

Good ol path traversal https://github.com/grafana/grafana/commit/c798c0e958d15d9cc7...

This part is kind of interesting — it looks like gosec at least gave the opportunity to catch this but that was missed due a misunderstanding of what was in scope for filepath.Clean:

https://github.com/grafana/grafana/commit/c798c0e958d15d9cc7...

I mean, can you blame them?

The docs for filepath.Clean state the following:

    ...
    3. Eliminate each inner .. path name element (the parent directory)
       along with the non-.. element that precedes it.
    4. Eliminate .. elements that begin a rooted path:
       that is, replace "/.." by "/" at the beginning of a path,
       assuming Separator is '/'.

There is no mention that this function cannot be used for security, or that it does not safely ensure a path can't refer to a parent directory.

A quick reading of 3 and 4 will make you assume that a path has no ".."s after being "Clean"d. If you actually think about it more, you'll realize that of course it will leave ".." at the beginning of relative paths, but the docs do not make it clear, and I can understand why a programmer might reach for this.

There's no clearer function to reach for either, like "filepath.DirectoryContains(parent, path) bool".

A good example case for why testing your assumptions is usually a good thing.

Actually no. Testing even a fairly comprehensive set of test cases could have given a false sense of security and possibly missed some actual vulnerable cases.

Best thing would have been to either implement a function that does give the required guarantees or look for an existing library that explicitly does.

> There is no mention that this function cannot be used for security, or that it does not safely ensure a path can't refer to a parent directory.

It seems to me that both of those guarantees are something that would need to be explicitly stated if provided, and one wouldn't just assume that they hold by omission. In general if the docs were to list all postconditions that are not guaranteed to hold, it would take a while to write them :).

Just to be clear, I’m not blaming them. I should have clarified that I was thinking along the lines of “how could the tool/documentation have made it easier to recognize that gap?” — and I especially agree with your suggestion that there could be a solid library improvement with a purpose-built function which is clearly identified as the right way to address this relatively common need.

Important note: I mixed up CVE-2021-41090 and CVE-2021-43798 in the initial version of the blog post. While that has been corrected and a note added to the blog post, it still lead to some confusion.

The 0day is only for Grafana-the-software, not for the Grafana Agent.

Also important to note: While the overall course of events is clearly less than ideal, we still strongly believe that Jordy did us good. Mistakes happen, and the intention was good. Overall, Grafana is now more secure than it was last week.

I wrote a script today to try and exploit this on our Grafana 8.1.2 instance but couldn't. Using Oauth for auth and only got 302 redirects back to the login page. Anyone else able to exploit this with Oauth?

It might be because of path normalization by your http client. For example, with `curl` you will also need to use `--path-as-is` to correctly test traversal. Another reason could be path normalization by the reverse proxy/WAF.

> --path-as-is

> Tell curl to not handle sequences of /../ or /./ in the given URL path. Normally curl will squash or merge

> them according to standards but with

>this option set you tell it not to do that.

> Added in 7.42.0.

> 2021-12-03 08:42: Jordy tweets and deletes about “read arbitrary files on the host, no authentication needed” (Editor’s note: We were not aware of this until 2021-12-07.)

Doesn't quite sound like an "ethical hacker" to me.

I talked to Jordy about it. It was his first CVSS HIGH vulnerability and he was super happy & excited about it. While we would have preferred if things went differently, it was an honest mistake.

On balance, I still prefer if someone approaches us with good intentions and messes up a bit over someone simply dumping a 0day into the wild or into private circles. And this way, we at least had a tested patch in hand already and knew that Grafana Cloud was not affected.

And as per https://news.ycombinator.com/item?id=29495431 ... we all make mistakes.

Echoing this. It was a good find and well intended. We'll welcome any future submissions from Jordy.

As far as I can see the post does not mention the affected releases nor the versions to upgrade to.

Is 8.3.1 patched?

Yes. It's in the advisory: https://grafana.com/blog/2021/12/07/grafana-8.3.1-8.2.7-8.1....

Affects all 8.x releases

I just realized I'm still on 7.3 because I could never get the image renderer to work on 8.x (the thing that shows the graph in the alert email). Anyone got a Docker image that will render images out of the box?

[edit: Just installed 8.3. Alert conditions warn they are now "beta feature" and "could stop working in next version". Is Grafana removing features every release?]

Note: postmortem has a more dire meaning in non-tech circles (literally means "after death"). You want to say retrospective instead. I know it's a difference in culture.

I think people used to publish "postmortems" for projects that were literally over, either because they failed or because they were published long ago. At some point people started using it for outages, then security issues too.

I agree that the term is inadequate, I can just see how we got here.

> 2021-12-03: Release plan set: 2021-12-07 for private customer release, 2021-12-14 for public release

Does someone know why they were playing on sitting on the public release for a week after private release?

Seems that by doing this they allowed it to become a 0day.

> Customers get a week to upgrade under strict embargo

Seems that they can enforce an embargo with their private customers so that it won't become a 0day.

It became a 0day because the security researcher inadvertently kicked off public discussion.

I should clarify that “inadvertently” is the key word here. The 0day happened because of a number of factors detailed in the post, and it isn’t the fault of any one person.

Agreed. We will certainly improve processes on our end as a direct result of this, as well.

Reply on news.ycombinator.com