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

320159: main part (work in progress) #140

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Apr 3, 2019

@3rd-Eden This is basically a set of monkey-patch fixes to the regexes by doing some basic modifications to make them less prune to DoS attacks.

It fixes most of the issues (I hope), but is labelled as work in progress because it still breaks something. See attached lists and the code comments.

This is based on top of #138, but we can rebase it out of here (though that would require resolving conflicts).

Also /cc @davisjam.

We could try to finish fixing them in this PR, and then try to submit to uap-core first before merging this?

Hopefully, monkey patching won't be needed at all, but this branch is needed nevertheless, because we need to test those regexes.

Discussing in my private fork didn't work, hence opening it up here for feedback on the specific regex modifications and the approach.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Apr 15, 2019

So, any thoughts about the approach?

@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 23, 2019

It looks like uap-core updated regexes and fixed at least some of the DoS issues in ua-parser/uap-core@bf80eb2.
This has to be reevaluated.

They use safe-regex which does not actually guarantee safety, though.

@3rd-Eden A re-sync with uap-core would be helpful, I guess. Combined with #137.

@3rd-Eden
Copy link
Owner

@ChALkeR Did you want to me to update the master branch with latest set of uap-core?

@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 23, 2019

@3rd-Eden Yes, that would be good, I guess? That fixes at least some known EDA.

Do you want a PR for that? I assumed that it could be easier to just run that locally than review a PR, but I can definitely file a PR with just regex updates if you would like me to,

@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 23, 2019

Just #137 is not effective (does not protect against some EDA) and just uap-core update is not effective (still includes a whole bunch of IDA) -- I have testcases against both.

But combined #137 and uap-core update might be effective -- I will try to review the regexes tomorrow to see if any regex fixes are actually needed after #137 and uap-core update or not.

@mastermatt
Copy link

FYI: as of May 19th, Snyk is classifying this lib as having a high severity vulnerability for this issue.
https://snyk.io/vuln/SNYK-JS-USERAGENT-174737

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

Successfully merging this pull request may close these issues.

None yet

3 participants