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

Go: Add an example specific to domain names in missing-regexp-anchor #16220

Merged
merged 2 commits into from May 11, 2024

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Apr 15, 2024

It didn't seem clear what to do when the vulnerable regular expression is only for a hostname.

Some fixes ended up just inserting an end-anchor, and ignoring the beginning.
So e.g. the "fix" github.com$ could still be bypassed with e.g. totallynotafakegithub.com.

Copy link
Contributor

github-actions bot commented Apr 15, 2024

QHelp previews:

go/ql/src/Security/CWE-020/MissingRegexpAnchor.qhelp

Missing regular expression anchor

Sanitizing untrusted input with regular expressions is a common technique. However, it is error-prone to match untrusted input against regular expressions without anchors such as ^ or $. Malicious input can bypass such security checks by embedding one of the allowed patterns in an unexpected location.

Even if the matching is not done in a security-critical context, it may still cause undesirable behavior when the regular expression accidentally matches.

Recommendation

Use anchors to ensure that regular expressions match at the expected locations.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

package main

import (
	"errors"
	"net/http"
	"regexp"
)

func checkRedirect2(req *http.Request, via []*http.Request) error {
	// BAD: the host of `req.URL` may be controlled by an attacker
	re := "https?://www\\.example\\.com/"
	if matched, _ := regexp.MatchString(re, req.URL.String()); matched {
		return nil
	}
	return errors.New("Invalid redirect")
}

The check with the regular expression match is, however, easy to bypass. For example, the string http://example.com/ can be embedded in the query string component: http://evil-example.net/?x=http://example.com/.

Address these shortcomings by using anchors in the regular expression instead:

package main

import (
	"errors"
	"net/http"
	"regexp"
)

func checkRedirect2Good(req *http.Request, via []*http.Request) error {
	// GOOD: the host of `req.URL` cannot be controlled by an attacker
	re := "^https?://www\\.example\\.com/"
	if matched, _ := regexp.MatchString(re, req.URL.String()); matched {
		return nil
	}
	return errors.New("Invalid redirect")
}

A related mistake is to write a regular expression with multiple alternatives, but to only anchor one of the alternatives. As an example, the regular expression ^www\.example\.com|beta\.example\.com will match the host evil.beta.example.com because the regular expression is parsed as (^www\.example\.com)|(beta\.example\.com)/, so the second alternative beta\.example\.com is not anchored at the beginning of the string.

When checking for a domain name that may have subdomains, it is important to anchor the regular expression or ensure that the domain name is prefixed with a dot.

package main

import (
	"regexp"
)

func checkSubdomain(domain String) {
	// Checking strictly that the domain is `example.com`.
	re := "^example\\.com$"
	if matched, _ := regexp.MatchString(re, domain); matched {
		// domain is good.
	}

	// GOOD: Alternatively, check the domain is `example.com` or a subdomain of `example.com`.
	re2 := "(^|\\.)example\\.com$"

	if matched, _ := regexp.MatchString(re2, domain); matched {
		// domain is good.
	}
}

References

@erik-krogh erik-krogh marked this pull request as ready for review May 8, 2024 19:05
@erik-krogh erik-krogh requested a review from a team as a code owner May 8, 2024 19:05
owen-mc
owen-mc previously approved these changes May 9, 2024
go/ql/src/Security/CWE-020/MissingRegexpAnchor.qhelp Outdated Show resolved Hide resolved
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
@erik-krogh erik-krogh merged commit 0d814e0 into github:main May 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants