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

Denial of Service (DoS) Reported in SYNK #6

Closed
pak-ferry opened this issue Nov 29, 2022 · 34 comments
Closed

Denial of Service (DoS) Reported in SYNK #6

pak-ferry opened this issue Nov 29, 2022 · 34 comments

Comments

@pak-ferry
Copy link

Hey there, I found this issue while running security scan with SYNK. And, turned out there's a vulnerability. Hopefully there's a way to resolve this issue. Thank you.

Here's the error logs:
Issues with no direct upgrade or patch: ✗ Denial of Service (DoS) [High Severity][https://security.snyk.io/vuln/SNYK-JS-DECODEURICOMPONENT-3149970] in decode-uri-component@0.2.0 introduced by @testing-library/jest-dom@5.16.4 > css@3.0.0 > source-map-resolve@0.6.0 > decode-uri-component@0.2.0 and 2 other path(s) No upgrade or patch available

@acode-x
Copy link

acode-x commented Nov 29, 2022

Have the same issue.

@NiklasPor
Copy link

FYI related GitHub advisory: GHSA-w573-4hg7-7wgq

@tiggem1993
Copy link

Hey I am also facing same issue.

@niji-DamienA
Copy link

Same issue !

@fionnachan
Copy link

saw it on GHSA-w573-4hg7-7wgq
please fix :( 🙏🙏🙏

@briandrutledge
Copy link

We saw this today also.....fix is needed ASAP. https://nvd.nist.gov/vuln/detail/CVE-2022-38900

@hello-alf
Copy link

Same issue Here!,
image

@brampurnot
Copy link

Same here!

@jhon-gomez-endava
Copy link

Any updates?

@josuevalrob
Copy link

%  yarn npm audit --all --recursive                
└─ decode-uri-component: 0.2.0
   ├─ Issue: decode-uri-component vulnerable to Denial of Service (DoS)
   ├─ URL: https://github.com/advisories/GHSA-w573-4hg7-7wgq
   ├─ Severity: low
   ├─ Vulnerable Versions: <=0.2.0
   ├─ Patched Versions: <0.0.0
   ├─ Via: lint-staged, jest-environment-jsdom, msw, @typescript-eslint/parser, eslint-config-airbnb-typescript, eslint-plugin-import, @storybook/addon-docs, @storybook/addon-essentials, @storybook/react, ts-jest

@NicholasJarr
Copy link

Same issue here!

@matz3
Copy link

matz3 commented Nov 30, 2022

If someone also faces this issue because of the css package: There is a maintained fork available which doesn't rely on "source-map-resolve" / "decode-uri-component": reworkcss/css#163 (comment)

matz3 added a commit to SAP/less-openui5 that referenced this issue Nov 30, 2022
This solves the issue of having outdated and potential insecure
transitive dependencies.

There are no known behavior changes, so this is considered a
non-breaking change / fix.

See: reworkcss/css#163
See: SamVerschueren/decode-uri-component#6
matz3 added a commit to SAP/less-openui5 that referenced this issue Nov 30, 2022
This solves the issue of having outdated and potential insecure
transitive dependencies.

There are no known behavior changes, so this is considered a
non-breaking change / fix.

See: reworkcss/css#163
See: SamVerschueren/decode-uri-component#6
matz3 added a commit to SAP/less-openui5 that referenced this issue Nov 30, 2022
This solves the issue of having outdated and potential insecure
transitive dependencies.

There are no known behavior changes, so this is considered a
non-breaking change / fix.

See: reworkcss/css#163
See: SamVerschueren/decode-uri-component#6
@Madhust
Copy link

Madhust commented Nov 30, 2022

Just wondering, whether this repo is maintained actively. In the npm registry, It's been updated 5 years back. Not sure, is there any collaborator or maintainer for this package? I'm certain thousands of projects CI are prevented by this. FYI folks, I tried to contact @SamVerschueren on twitter but no reply yet.

@SamVerschueren
Copy link
Owner

I'll try to take a look today and release a fix.

@xavisegura
Copy link

I'll try to take a look today and release a fix.

Really appreciated. Thank you!

@SamVerschueren
Copy link
Owner

I just released a fix under v0.2.1. There's still a bug that I found, but it's somewhat unrelated and I will try to release a patch for it later. At least the package shouldn't blow up anymore (I hope 😂 ).

Thanks everyone for chiming in and sorry for the late replies 🙏 .

@briandrutledge
Copy link

Hey @SamVerschueren what is the bug still existing? Just wanted to understand that before having my engineering team update. :)

@SamVerschueren
Copy link
Owner

The issue is that %ea%ba%5a%ba gets incorrectly decoded into while it should be %ea%baZ%ba. Now this is really an edge case I think but would be good if it got fixed properly.

If the Snyk report is keeping you from installing or using the package, upgrading is definitely safe to do.

@hello-alf
Copy link

Thanks @SamVerschueren, you saved my work

@esterfania
Copy link

Thanks, @SamVerschueren 😄

@SamVerschueren
Copy link
Owner

Just released version 0.2.2 with a proper fix where some decoded values got removed in the output.

For instance

  • %ea%ba%5a%ba -> %ea%ba%ba -> (basically dropping the %5a token)
    • After the fix this results in %ea%baZ%ba
  • %C3%5A%A5%AB -> %C3%A5%AB ->å%AB (basically dropping the %5A token here as well)
    • After the fix this results in %C3Z%A5%AB

@xavisegura
Copy link

Thanks a lot @SamVerschueren

aarjithn added a commit to aarjithn/query-string that referenced this issue Dec 2, 2022
@vinay3206
Copy link

Issue still exists in 0.2.2
https://security.snyk.io/package/npm/decode-uri-component/0.2.2

@SamVerschueren
Copy link
Owner

I'll have a look first thing in the morning. I don't really know how to test these vulnerabilities myself.

@viceice
Copy link

viceice commented Dec 2, 2022

I think the fixed version needs to be reported to snyk. the github report is already updated

GHSA-w573-4hg7-7wgq

maybe you also need to publish a security advisory inside this repo. 🤷‍♂️

@SamVerschueren
Copy link
Owner

So I looked at the security report for v0.2.2 and I don't know why they still report it but it's incorrect. The package handles that string correctly. I'll see what I can do for Snyk to pick up the patched version.

@SamVerschueren
Copy link
Owner

I don't know why that page indicates that version 0.2.2 is still affected because if you look deeper, it doesn't look like that's the case.

image

@SamVerschueren
Copy link
Owner

I'm in contact with Snyk to see what can be done here. I'll try to follow this up as close as possible.

@SamVerschueren
Copy link
Owner

I had a back-and-forth with someone from Snyk. He also saw that v0.2.2 doesn't get flagged anymore and suggested to wait until monday so I could run a new scan.

@SamVerschueren
Copy link
Owner

Oh, this page also doesn't show vulnerabilities anymore https://security.snyk.io/package/npm/decode-uri-component/0.2.2. So I guess we're good now? 🤷

@pak-ferry
Copy link
Author

Hey there Sam, I think you did great work right there. Thank you for getting back to us!

@romellem
Copy link

romellem commented Dec 6, 2022

Can anyone explain how this is a CVE? This seems like a bug rather than a vulnerability.

On certain inputs decodeComponents() could return a string, which means the call to decodeComponents(...).join('') throws an error. I don't see how that is related to a ReDoS attack.

@SamVerschueren
Copy link
Owner

It wasn't a vulnerability, it was just a bug like you said.

It was not flagged as ReDoS but as DoS in the sense that if you provided wrong input, you could make a server crash if the developer didn't have proper error handling.

This is just the thing with Snyk. People get paid per "vulnerability" they can find. Then you end up in situations like these where people get mad because their CI cant build the app anymore because of a stupid fake vulnerability...

@romellem
Copy link

romellem commented Dec 7, 2022

I totally misread, every time I saw those "DoS" reports from snyk, it was always "ReDoS," so just assumed it was the same lame type of report.

And wow, they are really stretching the definition of "Denial of Service." Thanks for releasing this new version. I really appreciate you helping with this, even (if I'm guessing) you probably haven't thought about this library much since you first released 5 years ago.

ryenus added a commit to ryenus/less-openui5 that referenced this issue Jan 16, 2023
This had been first attempted in 0abe66a but later reverted by 3b6c459
due to adobe/css-tools#77, reapplying since
the issue is now fixed.

---

This solves the issue of having outdated and potential insecure
transitive dependencies.

There are no known behavior changes, so this is considered a
non-breaking change / fix.

See: reworkcss/css#163
See: SamVerschueren/decode-uri-component#6
ryenus added a commit to ryenus/less-openui5 that referenced this issue Jan 16, 2023
This had been first attempted in 0abe66a but later reverted by 3b6c459
due to adobe/css-tools#77, reapplying since
the issue is now fixed.

---

This solves the issue of having outdated and potential insecure
transitive dependencies.

There are no known behavior changes, so this is considered a
non-breaking change / fix.

See: reworkcss/css#163
See: SamVerschueren/decode-uri-component#6
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

No branches or pull requests