search

3rd-Eden / useragent

Useragent parser for Node.js, ported from browserscope.org
MIT License
899 stars 135 forks source link

320159: main part (work in progress) #140

Open ChALkeR opened 5 years ago

ChALkeR commented 5 years ago

@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 https://github.com/3rd-Eden/useragent/pull/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 commented 5 years ago

So, any thoughts about the approach?

ChALkeR commented 5 years ago

It looks like uap-core updated regexes and fixed at least some of the DoS issues in https://github.com/ua-parser/uap-core/commit/bf80eb25f940f2c2a4672681d753a8034839066c. 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 commented 5 years ago

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

ChALkeR commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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