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

Incomplete Regular Expression for Hostnames: Unescaped Dot Character #1600

Closed
ImanSharaf opened this issue Mar 31, 2023 · 13 comments · Fixed by #1669
Closed

Incomplete Regular Expression for Hostnames: Unescaped Dot Character #1600

ImanSharaf opened this issue Mar 31, 2023 · 13 comments · Fixed by #1669
Assignees
Labels
6) PR awaiting review josh/elar V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements V10 tmp code-review Temporary label for grouping code review or program code related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@ImanSharaf
Copy link
Collaborator

There is a vulnerability in some applications where the dot character is not properly escaped in regular expressions designed to validate hostnames. This oversight can lead to bypassing security checks, such as those for preventing attacks like request forgeries and malicious redirections. Incomplete checks may also cause undesirable behaviors when they accidentally succeed.
If an attacker successfully exploits this vulnerability, they may perform unauthorized actions like forging requests or redirecting users to malicious websites. This can lead to a range of security issues, including data breaches, compromised application functionality, and exposing users to additional risks.

Suggested ASVS check:
Verify that the application properly escapes special characters, such as the dot character, in regular expressions used for hostname validation to prevent security threats like request forgeries and malicious redirections.

Sample vulnerable code:

app.get('/some/path', function(req, res) {
    let url = req.param('url'),
        host = urlLib.parse(url).host;
    // BAD: the host of `url` may be controlled by an attacker
    let regex = /^((www|beta).)?example.com/;
    if (host.match(regex)) {
        res.redirect(url);
    }
});
@elarlang
Copy link
Collaborator

elarlang commented Mar 31, 2023

URL validation - We have requirement for validating URL's (5.1.5), so from the opened issue point of view - URL validation is just done incorrectly and the problem is covered.

Regex - If you don't escape data for regex, it's general problem (not host validation specific). I think this is something we should have but without context limitation (like hostname validation). Like in PHP, use function preg_quote when building for regular expressions dynamically.

Probably we can combine it with other issue: #1562

@ImanSharaf
Copy link
Collaborator Author

Thank you for your response. I believe #1562 is fundamentally different. In order to remove context limitation we can have this check:
Verify that the application properly escapes special characters, such as the dot character, in regular expressions for various input validation tasks

@elarlang
Copy link
Collaborator

But why to put the dot for special spotlight? It may take attention away from others or give an impression, that the dot is the only or at least most important symbol. You need to escape all of them:

. \ + * ? [ ^ ] $ ( ) { } = ! < > | : - #

@ImanSharaf
Copy link
Collaborator Author

We can write it more generic like this, the reason that I considered a special place for dot is that I have seen that issue in many clients and an attacker could easily bypass their filters.

Verify that the application properly escapes special characters in regular expressions for various input validation tasks

@elarlang
Copy link
Collaborator

Probably we can fine-tune it more, but it's a good input and for me good requirement to have in ASVS :)

@elarlang elarlang added 4) proposal for review Issue contains clear proposal for add/change something _5.0 - prep This needs to be addressed to prepare 5.0 josh/elar labels Mar 31, 2023
@ImanSharaf
Copy link
Collaborator Author

Thank you for your feedback! I'm glad that you find the proposed ASVS check valuable. Your input and suggestions have been instrumental in refining the requirement.

@elarlang elarlang added V10 tmp code-review Temporary label for grouping code review or program code related issues V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements labels Apr 5, 2023
@tghosth
Copy link
Collaborator

tghosth commented May 31, 2023

@ImanSharaf what do you think about:

Verify that the application uses slashes to correctly escape special characters being used in regular expressions to ensure they are not misinterpreted as control characters.

@elarlang
Copy link
Collaborator

Is it written somewhere that slash is the only escape character and it's not up to the configuration?

@tghosth
Copy link
Collaborator

tghosth commented May 31, 2023

Pretty sure slash is the only option:
https://learn.microsoft.com/en-us/dotnet/standard/base-types/character-escapes-in-regular-expressions

@elarlang
Copy link
Collaborator

For category it waits: #1643

@tghosth
Copy link
Collaborator

tghosth commented Jun 15, 2023

@elarlang are you ok for me to create a PR based on:

Verify that the application uses slashes to correctly escape special characters being used in regular expressions to ensure they are not misinterpreted as control characters.

I would currently put this in 5.2 "Sanitization and Sandboxing" but we can use that other issue #1643 to consider future categorization.

@elarlang
Copy link
Collaborator

@elarlang are you ok for me to create a PR based on:

yes

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed 4) proposal for review Issue contains clear proposal for add/change something labels Jun 15, 2023
@tghosth tghosth linked a pull request Jun 15, 2023 that will close this issue
@tghosth
Copy link
Collaborator

tghosth commented Jun 15, 2023

Opened #1699

@tghosth tghosth added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6) PR awaiting review josh/elar V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements V10 tmp code-review Temporary label for grouping code review or program code related issues _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants