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

Discourage unsafe regex #86

Closed
stefansundin opened this issue Jul 13, 2015 · 5 comments
Closed

Discourage unsafe regex #86

stefansundin opened this issue Jul 13, 2015 · 5 comments

Comments

@stefansundin
Copy link

Hi.

So I noticed that the regex we used for a product wasn't that secure.

We used origins(/https:\/\/example\.co/), but the problem with this regex is that it also allows example.com. So the simple fix is to add ^ in the beginning and $ to the end.

We actually just replaced the regex with a string, which fixed the issue for us, but for those that actually need a regex, I think it would be nice to explicitly mention to use the start and end tags in the README. Especially since most people probably just copy-paste and doesn't really think about what they are doing.

Also, the regex runs faster with a start boundary, even though it doesn't make it safer.

I think updating the example code with a regex to use ^ and $ might be enough. Maybe even mention it in the Origin section.

Thanks!

@andresriancho
Copy link

I can confirm that this is a common issue. Found exactly the same problem as reported by @stefansundin while performing a security assessment for an important tech company.

@stefansundin
Copy link
Author

Indeed, the issue is bigger than I originally thought because origins(/https:\/\/example\.com/) also allows the domain example.com.randomdomainname.co.uk too. To attack an website with an insecure regex you can just create a subdomain for it.

Because it is so widespread, I actually think it might be prudent to add the start and end tags automatically, optionally disabling this behavior with an option.

@cyu cyu closed this as completed in 3f51048 Oct 13, 2015
@stefansundin
Copy link
Author

👏

cyu added a commit that referenced this issue Oct 13, 2015
Build regex with end string anchor when string is used to configure
origin

[Fixes #86]
@cyu
Copy link
Owner

cyu commented Oct 13, 2015

@stefansundin I'm reluctant to add the ^$ automatically because it's a behavior I don't think developers would expect. Someone would have to peruse the code to figure out why their seemingly valid regex isn't working.

It just occurred to me though that what I can do is detect the presence of ^ and $ at the end, and log a stern warning if they aren't present in the regex. I'll add this as a follow up commit before pushing out an official version.

@stefansundin
Copy link
Author

Wonderful!

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

3 participants