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

use full-string match to speed up aspnet regex match #2939

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
8 participants
@rvagg
Contributor

rvagg commented Feb 2, 2016

Fixes: #2936

C# duration match was almost complete enough to match full-strings, just needed to allow for the extra digits in microseconds. Getting to put in ^ $ resolves the speed problem reported @ #2936.

@Starefossen

This comment has been minimized.

Starefossen commented Feb 2, 2016

Original expression

/(\-)?(?:(\d*)[. ])?(\d+)\:(\d+)(?:\:(\d+)\.?(\d{3})?)?/

screen shot 2016-02-02 at 10 21 48

rvagg's fix

/^(\-)?(?:(\d*)[. ])?(\d+)\:(\d+)(?:\:(\d+)\.?(\d{3})?(?:\d*)?)?$/

screen shot 2016-02-02 at 10 21 38

There is essentially a duplicate 0 or more condition (* with a ?) at the end of this expression that can be optimised in the following two ways:

/^(\-)?(?:(\d*)[. ])?(\d+)\:(\d+)(?:\:(\d+)\.?(\d{3})?(?:\d+)?)?$/

screen shot 2016-02-02 at 10 28 19

/^(\-)?(?:(\d*)[. ])?(\d+)\:(\d+)(?:\:(\d+)\.?(\d{3})?(?:\d*))?$/

screen shot 2016-02-02 at 10 28 43

The last one can be simplified even more without loosing any functionality:

/^(\-)?(?:(\d*)[. ])?(\d+)\:(\d+)(?:\:(\d+)\.?(\d{3})?\d*)?$/

screen shot 2016-02-02 at 10 32 36

@rvagg

This comment has been minimized.

Contributor

rvagg commented Feb 2, 2016

smarty pants

updated with @Starefossen's regex

what's that tool @Starefossen?

@HPate-Riptide

This comment has been minimized.

HPate-Riptide commented Feb 2, 2016

@rvagg Looks like Regexper

@evilpacket

This comment has been minimized.

evilpacket commented Feb 2, 2016

Either fix seems acceptable from a security perspective. Both remove the extreme blocking behavior and take upwards of around 2.5 million characters to take 1 second of cpu time. so 👍

Thank you @rvagg and @Starefossen

@mj1856

This comment has been minimized.

Member

mj1856 commented Feb 2, 2016

Thanks!

@mj1856 mj1856 added the High Priority label Feb 2, 2016

@mj1856 mj1856 added this to the 2.12.0 milestone Feb 2, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Feb 2, 2016

Staging for next release. @ichernev - Due to the security implications, it may make sense to release this separately as 2.11.2, unless you're ready to merge the other PRs I've marked for 2.12.0. Your call.

@SomeKittens

This comment has been minimized.

SomeKittens commented Feb 3, 2016

Since this seems to be breaking builds left and right (hurrah, popularity!) my vote is to release it ASAP as a patch version.

@mj1856 mj1856 changed the title from use full-string match to speed up C# duration match to use full-string match to speed up aspnet regex match Feb 3, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Feb 3, 2016

Merged in 52a807b

@ichernev ichernev closed this Feb 3, 2016

ichernev added a commit that referenced this pull request Feb 3, 2016

Merge pull request #2939 from rvagg:regex-dos-no-more
use full-string match to speed up aspnet regex match

@mj1856 mj1856 modified the milestones: 2.11.2, 2.12.0 Feb 3, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Feb 3, 2016

This is available now in 2.11.2. Thanks @ichernev for getting this out.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Feb 3, 2016

Thank you @rvagg and @Starefossen for working on this. @joeybaker thank you for reporting. Is there any way I can get my email for the moment security advisories?

@joeybaker

This comment has been minimized.

joeybaker commented Feb 3, 2016

@ichernev FWIW, you might try adding https://www.npmjs.com/package/nsp to the tests. It would then fail the build if moment or any of its deps had a security vulnerability.

@nlf, @daviddias, or @evilpacket might know more about an email list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment