Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(xss): sanitize parameters #829

Merged
merged 1 commit into from
Feb 5, 2024
Merged

fix(xss): sanitize parameters #829

merged 1 commit into from
Feb 5, 2024

Conversation

mhenrixon
Copy link
Owner

No description provided.

@mhenrixon mhenrixon added the bug label Feb 5, 2024
@mhenrixon mhenrixon self-assigned this Feb 5, 2024
@mhenrixon mhenrixon merged commit ec3afd9 into main Feb 5, 2024
6 of 7 checks passed
@mhenrixon mhenrixon deleted the fix/xss-vulnerability branch February 5, 2024 15:15
@Earlopain
Copy link
Contributor

Earlopain commented Feb 6, 2024

For anyone stumbling across this, recent versions of sidekiq already protect against this (at least the trivial cases I could quickly come up with myself). The CSP is more restrictive since version 7.2.0, sidekiq/sidekiq#6074, released October 31, 2023.

@mhenrixon would you mind creating/publishing a CVE so that the ecosystem can be properly be notified of this issue? You should be able to do that directly through GitHub with their security advisories. This issue from sidekiq itself should serve as a pretty good base I think GHSA-h3r8-h5qw-4r35, basically the same thing.

@mhenrixon
Copy link
Owner Author

@mhenrixon would you mind creating/publishing a CVE

Never did before, any pointers?
I'd love to learn so that I can better contribute in the future.

@Earlopain
Copy link
Contributor

I'm fortunate enough to never have had to deal with this from this side myself yet. I did a quick search beforehand, and apparently you are able to directly request one from GitHub itself. There are other entities that can assign a CVE but to be honest that all seems a bit complicated to me.

I'm mostly basing this off of what I read at https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/creating-a-repository-security-advisory. This is also relevant https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/publishing-a-repository-security-advisory#requesting-a-cve-identification-number-optional

For you, this should be https://github.com/mhenrixon/sidekiq-unique-jobs/security/advisories/new. Fill that out, using the sidekiq advisory as a template, request a CVE somewhere on that advisory after submitting, and GitHub should take care of the rest. When all that is done (or somewhere inbetween, I don't think it matters much since the fix is already public) you can publish the advisory.

Sorry that I can't be of more use here, I'm usually just consuming these.

@mhenrixon
Copy link
Owner Author

Now I remember, I saw someone like to the correct form from their repository.

I'll do the same in the future.

@renchap
Copy link

renchap commented Feb 9, 2024

Could you consider backporting this to version 7, for the people that are still using Sidekiq 6?

Thanks :)

@mhenrixon
Copy link
Owner Author

Could you consider backporting this to version 7, for the people that are still using Sidekiq 6?

Thanks :)

Are you not protecting your sidekiq routes with some type of admin constraint? 🤣 I'll get it one as soon as possible.

@pboling
Copy link
Contributor

pboling commented Feb 12, 2024

I haven't seen a CVE for this yet, is one planned? @mhenrixon

@mhenrixon
Copy link
Owner Author

Not yet, got busy with a bunch of other stuff.

I figured that I could piggy the sidekiq one.

@pboling
Copy link
Contributor

pboling commented Feb 12, 2024

If I'm understanding this correctly...

We have already upgraded past the fix for sidekiq, as we are currently on sidekiq v7.1.6, and IIRC it was fixed in sidekiq v7.0.8. But we are on an old version of sidekiq-unique-jobs (upgrading now), and thus still had the vulnerability listed in this issue, I presume.

Does that make any sense? Is a fix for this gem not needed if we have already upgraded sidekiq? If not, then this gem needs its own separate CVE.

@Earlopain
Copy link
Contributor

If you are on sidekiq below version 7.2.0 and don't have a version of sidekiq-unique-jobs with this fix then you are vulnerable.

The change in 7.0.8 was for a similar XSS vulnerability like here, a fix in sidekiq doesn't fix it in sidekiq-unique-jobs. The change in sidekiq 7.2.0 was a preventive measure for similar problems, not in response to an actual issue and wasn't backported.

I agree that a CVE here would be necessary. It's not a problem if you are on latest sidekiq but like someone already mentioned they are on sidekiq 6.X which wasn't vulnerable to the original issue in sidekiq (and as such doesn't have the defensive fix) but does have the problem this PR fixed (as such the desire for a backport of this).

Even though everyone should have the admin dashboard behind a constraint, knowledge that the dashboard exists and getting a user to click a malicious link is good enough for exploitation.

@pboling
Copy link
Contributor

pboling commented Feb 12, 2024

Thanks for the explanation! I'll report it, unless someone beats me to it.

@pboling
Copy link
Contributor

pboling commented Feb 12, 2024

I tried. Looks like @mhenrixon is the hero we need, unless another collaborator / maintainer has access.

👉 🚨 https://github.com/mhenrixon/sidekiq-unique-jobs/security/advisories/new 🚨 👈

Even though everyone should have the admin dashboard behind a constraint, knowledge that the dashboard exists and getting a user to click a malicious link is good enough for exploitation.

This is an important note. Social hacking, where an attacker gets an authorized user to click on a malicious link, is often the easiest kind.

@mhenrixon
Copy link
Owner Author

This is an important note. Social hacking, where an attacker gets an authorized users to click on a malicious link, is often the easiest kind.

Looks like I had to enable advisories for it to work. It is enabled now

@pboling
Copy link
Contributor

pboling commented Feb 12, 2024

Thanks! Reporting...

mhenrixon added a commit that referenced this pull request Feb 12, 2024
mhenrixon added a commit that referenced this pull request Feb 12, 2024
mhenrixon added a commit that referenced this pull request Feb 12, 2024
* fix: backport xss and rce fixes to v7.1

This was fixed in #829 and #833

* chore(lint): lint'em real good

* chore: remove reek
@pboling
Copy link
Contributor

pboling commented Feb 13, 2024

@Earlopain new CVE requested for Reflected, Server-Side, Non-Self XSS . Thanks for your help identifying!

@mhenrixon
Copy link
Owner Author

mhenrixon commented Feb 13, 2024

Massive thank you to both @Earlopain and @pboling.

The CWE is now not only published but also fixed.

Couldn't have done it without you guys! The repository will be more secure moving forward.

GHSA-cmh9-rx85-xj38

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

Successfully merging this pull request may close these issues.

4 participants