fix(rce): prevent remot code execution

[mhenrixon]

No description provided.

[Earlopain]

Sorry for the bother again. Is this what you were talking about in #829 (comment)? I'd assume so but the title/commit makes this sound way worse.

8.0.10 isn't pushed to rubygems yet.

[mhenrixon]

Sorry for the bother again. Is this what you were talking about in #829 (comment)? I'd assume so but the title/commit makes this sound way worse.

8.0.10 isn't pushed to rubygems yet.

It was way worse, redis doesn't filter anything according to @pboling who kindly provided a way to replicate the problem.

8.0.10 is unreleased because it contains no changes.

The fix was released as 8.0.9

[Earlopain]

I see, thanks for the info. My bad on reading the commit history wrong, I see now that it is like you said.

Great work from you and @pboling for identifying and fixing this problem.

[mhenrixon]

Also backported as https://github.com/mhenrixon/sidekiq-unique-jobs/releases/tag/v7.1.33

[pboling]

@Earlopain

I see, thanks for the info. My bad on reading the commit history wrong, I see now that it is like you said.

Great work from you and @pboling for identifying and fixing this problem.

I couldn't have done it without your multiple explanations of the context and the relevant issues. I'll submit you for credit on the CVE request, which you can accept if you like.

[Earlopain]

I'm not certain if this is actually an issue. Take these commands for example:

irb(main):002> Cache.redis.hset("hash", "]\"key\"", "value")
=> 1
irb(main):003> Cache.redis.hgetall("hash")
=> {"]\"key\""=>"value"}
irb(main):004> Cache.redis.hdel("hash", "]\"key\"")
=> 1
irb(main):005> Cache.redis.hgetall("hash")
=> {}

The key contains ] as well as " literals but redis/redis-client is taking them literally. https://redis.io/docs/management/security/ says:

String escaping and NoSQL injection
The Redis protocol has no concept of string escaping, so injection is impossible under normal circumstances using a normal client library. The protocol uses prefixed-length strings and is completely binary safe.

In addition, using the h function to facilitate escaping seems brittle. Its purpose has nothing to do with redis so anything it solves is by chance at best. There may be other special characters that have meaning in redis but are not relevant for escaping untrusted input for browser consumption.

I myself didn't manage to reproduce but if I did something wrong then I believe this should 100% be reported upstream to redis-client.

[pboling]

I see what you mean. Looking at this more soberly - It is the argument to the hdel command that is being injected from the path parameter here. The following queries that I hastily mistook as the result of an INFO command running were caused by the redirect back to the sidekiq /locks page gathering data for rendering itself.

I'll update the advisory.

[pboling]

This does still mean that a hacker could construct a URL that would result in deletion of targeted keys, but also there is no fix for that.

[Earlopain]

That's a relief, a 10/10 is quite severe (by definition). Thanks for double-checking.

I'm not certain what the layout of that hash is exactly, or if the key is dynamic in some way. I would expect it to only contain information about the status of locks, so the most you can do there is what you can already do through the web UI.

Regardless, you can do a bunch of stuff with the Web UI with plain sidekiq already like deleting complete queues so I don't think this should be a big deal. Unless you can delete arbitrary keys, that would be a bit of a problem. From a quick peek at the code that doesn't look possible to introduce arbitrary digests.

[pboling]

I think it is still a 10/10, since the XSS can expose sensitive cookies, session and local storage data.

I've corrected the advisory. Please give any feedback you have!

[Earlopain]

That looks better but it's definitely not a 10. The sidekiq equivalent is rated 8.3 which I think is still quite high but eh. I would suggest just copying most of the values from GHSA-h3r8-h5qw-4r35

While XSS is bad, it's not RCE levels kind of bad.

In general these reports with their severities fail to take into account that you are expected to put the dashboard behind a constraint. That's a separate issue though and not something I want to debate. While I don't personally agree, it's _fine, I guess.

[pboling]

@Earlopain I appreciate the feedback. I think that sidekiq advisory was calculated incorrectly for User Interaction, which is perhaps where we disagree about the lack-of-default-auth-constraint being considered here, but I did change two other values to match, which lowered the score slightly.

Specifically:

• User Interaction is not required in order to trigger this attack. An attacker with a crafted URL could make unlimited requests of an unprotected server, without another user having to do anything. The "User" in the "User Interaction" metric is not the attacker, but a second user. In this case there wouldn't be any sensitive data to steal, but it could trigger the 500 error over and over, resulting in degradation in service availability.
• Scope: When I thought there was an RCE, this was clearly "Changed", but that was incorrect, and so it is "Unchanged", same as the sidekiq vulnerability.
• Availability: Same case as User Interaction, but I have updated to "Low", same as the sidekiq vulnerability, because it seems unlikely to be able to cause significant damage to availability via the 500 error.

2.3.3. Assume Vulnerable Configurations
The explanation of Attack Complexity in CVSS v3.0 considers “the presence of certain system configuration settings”. This text has been removed from CVSS v3.1. If a specific configuration is required for an attack to succeed, the vulnerable component should be scored assuming it is in that configuration, providing it is a reasonable configuration. Unreasonable configurations are those that deliberately place the target in a vulnerable state, e.g., by disabling security features, or that conflict with documented configuration guidance, e.g., by using a non-default configuration that a product vendor explicitly states should never be used.

Ref

I agree that this could easily be parsed the way you are interpreting it, but I hold that it can also be interpreted the other way, for a scenario like ours where the default configuration is the unprotected one. The title of the section seems to imply a preference for assuming a vulnerable configuration so long as it is not un-reasonable, giving the example, that it would be unreasonable to assume the vulnerable configuration if it is not default, and not recommended, which is not quite our case.

[Earlopain]

I'm not an expert on this, I haven't read the actual definitions. I just find it laughable that the difference between RCE and XSS is 0.6 points. But that has nothing to do with you, if that is indeed how this works then I'm just shaking my head in defeat at the process in general.

Anyways, I believe @mhenrixon still needs to request a CVE for this to actually be picked up by tools like dependabot.

I believe the report should be compacted a bit beforehand. I don't think I've seen such details before, including POC.
Take these as an example:

• GHSA-68xg-gqqm-vgj8
• GHSA-3hhc-qp5v-9p2j
• GHSA-wq4h-7r42-5hrr
• GHSA-hq7p-j377-6v63

They don't go into nearly as much detail. I would find it fine as initial disclosure but not something consumers should see in the end. I would instead instead link to the relevant PRs as reference where interested users can get more information. Particularly the commit with the fix should be there.

[mhenrixon]

Appreciate the conversation, I did request the CVE already but unsure how it works after.

Everywhere I look is simply overwhelming information. Find it hard to narrow things down.

Next time we can perhaps be a bit briefer.

[Earlopain]

Eh, perhaps they'll trim it down a bit, or it updates transparently. What's done is done. From what I read, just waiting now should do the job.

Unfortunatly there's no indicator that you've already done so, my bad. At least on the GitHub advisory side these things do update, they are simply stored in git and you can even do PRs against that.

[pboling]

It says the CVE process can take a few days, and since it was just requested a few hours ago, we may have a few days wait.

[pboling]

@mhenrixon @Earlopain others who did the eval for the official CVE regraded it as a 7.1 🤷 I kind of appreciate that it is a lower number, but it exposes a difficulty of the grading scheme. This vulnerability is a bit more serious than the similar one that affected sidekiq, but it has a higher score. Oh well. I've updated the score for this one to match what was officially released in the CVE.

[Earlopain]

I'm not quite sure why it would be rated differently to the one in sidekiq but am glad that it is lower overall. I've opened a PR github/advisory-database#3546 to update the github version as well.