From 10907c8b04a3d6d391ad0b4f4076532c8868adfe Mon Sep 17 00:00:00 2001
From: Sauyon Lee <sauyon@github.com>
Date: Fri, 6 Dec 2019 17:33:57 -0800
Subject: [PATCH] IncompleteHostnameRegexp: disallow unescaped dot before TLD

---
 change-notes/1.24/analysis-go.md               |  1 +
 .../CWE-020/IncompleteHostnameRegexp.go        | 18 +++++++++---------
 .../CWE-020/IncompleteHostnameRegexp.ql        |  2 +-
 .../CWE-020/IncompleteHostnameRegexpGood.go    | 18 +++++++++---------
 .../IncompleteHostnameRegexp.expected          | 10 +++++-----
 .../IncompleteHostnameRegexp.go                | 18 +++++++++---------
 .../IncompleteHostnameRegexpGood.go            | 18 +++++++++---------
 7 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md
index 9c0963a4374..47b3505b67f 100644
--- a/change-notes/1.24/analysis-go.md
+++ b/change-notes/1.24/analysis-go.md
@@ -13,3 +13,4 @@
 |-----------------------------------------------------|------------------------------|-----------------------------------------------------------|
 | Reflected cross-site scripting (`go/reflected-xss`) | Fewer results | Untrusted input flowing into an HTTP header definition is no longer flagged, since this is often harmless. |
 | Useless assignment to field (`go/useless-assignment-to-field`) | Fewer false positives | The query now conservatively handles fields promoted through embedded pointer types. |
+| Incomplete regular expression for hostnames (`go/incomplete-hostname-regexp`) | More results | The query now flags unescaped dots before the TLD in a hostname regex. |
diff --git a/ql/src/Security/CWE-020/IncompleteHostnameRegexp.go b/ql/src/Security/CWE-020/IncompleteHostnameRegexp.go
index cccf148c55a..073c8555efc 100644
--- a/ql/src/Security/CWE-020/IncompleteHostnameRegexp.go
+++ b/ql/src/Security/CWE-020/IncompleteHostnameRegexp.go
@@ -1,16 +1,16 @@
 package main
 
 import (
-    "errors"
-    "regexp"
-    "net/http"
+	"errors"
+	"net/http"
+	"regexp"
 )
 
 func checkRedirect(req *http.Request, via []*http.Request) error {
-    // BAD: the host of `url` may be controlled by an attacker
-    re := "^((www|beta).)?example.com/"
-    if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
-        return nil
-    }
-    return errors.New("Invalid redirect")
+	// BAD: the host of `req.URL` may be controlled by an attacker
+	re := "^((www|beta).)?example.com/"
+	if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
+		return nil
+	}
+	return errors.New("Invalid redirect")
 }
diff --git a/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql b/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql
index 7a8559f041a..a9109e6a5ca 100644
--- a/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql
+++ b/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql
@@ -26,7 +26,7 @@ predicate isIncompleteHostNameRegexpPattern(string pattern, string hostPart) {
             "(?<!\\\\)[.]" +
             // immediately followed by a sequence of subdomains, perhaps with some regex characters mixed in,
             // followed by a known TLD
-            "([():|?a-z0-9-]+(\\\\)?[.]" + commonTLD() + ")" + ".*", 1)
+            "(([():|?a-z0-9-]+(\\\\)?[.])?" + commonTLD() + ")" + ".*", 1)
 }
 
 class Config extends DataFlow::Configuration {
diff --git a/ql/src/Security/CWE-020/IncompleteHostnameRegexpGood.go b/ql/src/Security/CWE-020/IncompleteHostnameRegexpGood.go
index 6842395b301..f918d5c6662 100644
--- a/ql/src/Security/CWE-020/IncompleteHostnameRegexpGood.go
+++ b/ql/src/Security/CWE-020/IncompleteHostnameRegexpGood.go
@@ -1,16 +1,16 @@
 package main
 
 import (
-    "errors"
-    "regexp"
-    "net/http"
+	"errors"
+	"net/http"
+	"regexp"
 )
 
 func checkRedirectGood(req *http.Request, via []*http.Request) error {
-    // GOOD: the host of `url` must be `example.com`, `www.example.com` or `beta.example.com`
-    re := "^((www|beta)\\.)?example.com/"
-    if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
-        return nil
-    }
-    return errors.New("Invalid redirect")
+	// GOOD: the host of `req.URL` must be `example.com`, `www.example.com` or `beta.example.com`
+	re := "^((www|beta)\\.)?example\\.com/"
+	if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
+		return nil
+	}
+	return errors.New("Invalid redirect")
 }
diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.expected b/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.expected
index 611637df31e..c0bdf6b96d1 100644
--- a/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.expected
+++ b/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.expected
@@ -1,9 +1,9 @@
 edges
-| IncompleteHostnameRegexp.go:11:11:11:39 | "^((www\|beta).)?example.com/" : string | IncompleteHostnameRegexp.go:12:41:12:42 | re |
+| IncompleteHostnameRegexp.go:11:8:11:36 | "^((www\|beta).)?example.com/" : string | IncompleteHostnameRegexp.go:12:38:12:39 | re |
 nodes
-| IncompleteHostnameRegexp.go:11:11:11:39 | "^((www\|beta).)?example.com/" : string | semmle.label | "^((www\|beta).)?example.com/" : string |
-| IncompleteHostnameRegexp.go:12:41:12:42 | re | semmle.label | re |
+| IncompleteHostnameRegexp.go:11:8:11:36 | "^((www\|beta).)?example.com/" : string | semmle.label | "^((www\|beta).)?example.com/" : string |
+| IncompleteHostnameRegexp.go:12:38:12:39 | re | semmle.label | re |
 | main.go:12:15:12:39 | `https://www.example.com` | semmle.label | `https://www.example.com` |
 #select
-| IncompleteHostnameRegexp.go:11:11:11:39 | "^((www\|beta).)?example.com/" : string | IncompleteHostnameRegexp.go:11:11:11:39 | "^((www\|beta).)?example.com/" : string | IncompleteHostnameRegexp.go:12:41:12:42 | re | This regular expression has an unescaped dot before ')?example.com', so it might match more hosts than expected when used $@. | IncompleteHostnameRegexp.go:12:41:12:42 | re | here |
-| main.go:12:15:12:39 | `https://www.example.com` | main.go:12:15:12:39 | `https://www.example.com` | main.go:12:15:12:39 | `https://www.example.com` | This regular expression has an unescaped dot before 'example.com', so it might match more hosts than expected when used $@. | main.go:12:15:12:39 | `https://www.example.com` | here |
+| IncompleteHostnameRegexp.go:11:8:11:36 | "^((www\|beta).)?example.com/" : string | IncompleteHostnameRegexp.go:11:8:11:36 | "^((www\|beta).)?example.com/" : string | IncompleteHostnameRegexp.go:12:38:12:39 | re | This regular expression has an unescaped dot before 'com', so it might match more hosts than expected when used $@. | IncompleteHostnameRegexp.go:12:38:12:39 | re | here |
+| main.go:12:15:12:39 | `https://www.example.com` | main.go:12:15:12:39 | `https://www.example.com` | main.go:12:15:12:39 | `https://www.example.com` | This regular expression has an unescaped dot before 'com', so it might match more hosts than expected when used $@. | main.go:12:15:12:39 | `https://www.example.com` | here |
diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.go b/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.go
index cccf148c55a..073c8555efc 100644
--- a/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.go
+++ b/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexp.go
@@ -1,16 +1,16 @@
 package main
 
 import (
-    "errors"
-    "regexp"
-    "net/http"
+	"errors"
+	"net/http"
+	"regexp"
 )
 
 func checkRedirect(req *http.Request, via []*http.Request) error {
-    // BAD: the host of `url` may be controlled by an attacker
-    re := "^((www|beta).)?example.com/"
-    if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
-        return nil
-    }
-    return errors.New("Invalid redirect")
+	// BAD: the host of `req.URL` may be controlled by an attacker
+	re := "^((www|beta).)?example.com/"
+	if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
+		return nil
+	}
+	return errors.New("Invalid redirect")
 }
diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexpGood.go b/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexpGood.go
index 6842395b301..f918d5c6662 100644
--- a/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexpGood.go
+++ b/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegexp/IncompleteHostnameRegexpGood.go
@@ -1,16 +1,16 @@
 package main
 
 import (
-    "errors"
-    "regexp"
-    "net/http"
+	"errors"
+	"net/http"
+	"regexp"
 )
 
 func checkRedirectGood(req *http.Request, via []*http.Request) error {
-    // GOOD: the host of `url` must be `example.com`, `www.example.com` or `beta.example.com`
-    re := "^((www|beta)\\.)?example.com/"
-    if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
-        return nil
-    }
-    return errors.New("Invalid redirect")
+	// GOOD: the host of `req.URL` must be `example.com`, `www.example.com` or `beta.example.com`
+	re := "^((www|beta)\\.)?example\\.com/"
+	if matched, _ := regexp.MatchString(re, req.URL.Host); matched {
+		return nil
+	}
+	return errors.New("Invalid redirect")
 }