Merge pull request #23 from MegaManSec/master

Ensure that a variable is used in the path when using request_uri, and do not warn on proxy_pass_normalized if request_uri is used

Author: dvershinin

gixy/directives/directive.py

@@ -46,12 +46,12 @@ def parents(self):
     def variables(self):
         raise NotImplementedError()
 
-    def find_directive_in_scope(self, name):
-        """Find directive in the current scope"""
+    def find_directives_in_scope(self, name):
+        """Find directives in the current scope"""
         for parent in self.parents:
             directive = parent.some(name, flat=False)
             if directive:
-                return directive
+                yield directive
         return None
 
     def __str__(self):

gixy/plugins/proxy_pass_normalized.py

@@ -1,5 +1,6 @@
 import re
 import gixy
+import sys
 from gixy.plugins.plugin import Plugin
 
 class proxy_pass_normalized(Plugin):
@@ -12,8 +13,8 @@ class proxy_pass_normalized(Plugin):
     """
 
     summary = 'Detect path after host in proxy_pass (potential URL decoding issue)'
-    severity = gixy.severity.LOW
-    description = ("A slash immediately after the host in proxy_pass leads to the path being decoded and normalized before proxying downstream, leading to unexpected behavior related to encoded slashes.")
+    severity = gixy.severity.MEDIUM
+    description = ("A path (beginning with a slash) after the host in proxy_pass leads to the path being decoded and normalized before proxying downstream, leading to unexpected behavior related to encoded slashes like %2F..%2F. Likewise, the usage of 'rewrite ^ $request_uri;' without using '$1' or '$uri' (or another captured group) in the path of proxy_pass leads to double-encoding of paths.")
     help_url = 'https://joshua.hu/proxy-pass-nginx-decoding-normalizing-url-path-dangerous#nginx-proxy_pass'
     directives = ['proxy_pass']
 
@@ -22,23 +23,46 @@ def __init__(self, config):
         self.parse_uri_re = re.compile(r'(?P<scheme>[^?#/)]+://)?(?P<host>[^?#/)]+)(?P<path>/.*)?')
 
     def audit(self, directive):
-        proxy_pass_arg = directive.args[0]
-        if not proxy_pass_arg:
+        proxy_pass_args = directive.args
+        rewrite_fail = False
+        num_pattern = r'\$\d+'
+
+        if not proxy_pass_args:
             return
 
-        parsed = self.parse_uri_re.match(proxy_pass_arg)
+        parsed = self.parse_uri_re.match(proxy_pass_args[0])
 
         if not parsed:
             return
 
-        if not parsed.group('path'):
+        for rewrite in directive.find_directives_in_scope("rewrite"):
+            if hasattr(rewrite, 'pattern') and hasattr(rewrite, 'replace'):
+                if rewrite.pattern == '^' and rewrite.replace == '$request_uri':
+                    if parsed.group('path'):
+                        match = re.search(num_pattern, parsed.group('path'))
+                        if match or '$uri' in parsed.group('path'):
+                            return
+                        else:
+                            rewrite_fail = True
+                            break
+                    else:
+                        if not parsed.group('host'):
+                            return # ?!
+                        match = re.search(num_pattern, parsed.group('host'))
+                        if match or '$uri' in parsed.group('host'):
+                            return
+                        else:
+                            rewrite_fail = True
+                            break
+
+        if not parsed.group('path') and not rewrite_fail:
             return
 
         self.add_issue(
             severity=self.severity,
             directive=[directive, directive.parent],
             reason=(
-                "Found a slash (and possibly more) after the hostname in proxy_pass. "
-                "This can lead to path decoding issues."
+                "Found a path after the host in proxy_pass, without using $request_uri and a variable (such as $1 or $uri). "
+                "This can lead to path decoding issues or double-encoding issues."
             )
         )

gixy/plugins/try_files_is_evil_too.py

@@ -18,8 +18,8 @@ class try_files_is_evil_too(Plugin):
 
     def audit(self, directive):
         # search for open_file_cache ...; on the same or higher level
-        open_file_cache = directive.find_directive_in_scope("open_file_cache")
-        if not open_file_cache or open_file_cache.args[0] == "off":
+        open_file_cache = list(directive.find_directives_in_scope("open_file_cache"))
+        if not open_file_cache or open_file_cache[0].args[0] == "off":
             self.add_issue(
                 severity=gixy.severity.MEDIUM,
                 directive=[directive],

tests/plugins/simply/proxy_pass_normalized/missing_variable.conf

@@ -0,0 +1,7 @@
+location /b {
+  rewrite ^ $request_uri; # Sets the $1/$uri variable to the raw path
+  proxy_pass http://127.0.0.1:8000/; # No $1 or $uri or other variable, resulting in path being double-encoded
+}
+
+# Request received by nginx: /%2F
+# Request received by backend: /%252F

tests/plugins/simply/proxy_pass_normalized/missing_variable_fp.conf

@@ -0,0 +1,7 @@
+location /b {
+  rewrite ^ $request_uri; # Sets the $1/$uri variable to the raw path
+  proxy_pass http://127.0.0.1:8000/$1; # $1 used, resulting in path being passed as the raw path from the original request.
+}
+
+# Request received by nginx: /%2F
+# Request received by backend: /%2F # Possibly also receives //%2F)

tests/plugins/simply/proxy_pass_normalized/missing_variable_nopath.conf

@@ -0,0 +1,7 @@
+location /b {
+  rewrite ^ $request_uri; # Sets the $1/$uri variable to the raw path
+  proxy_pass http://127.0.0.1:8000; # No $1 or $uri or other variable in either host or path, resulting in path being double-encoded
+}
+
+# Request received by nginx: /%2F
+# Request received by backend: /%252F

tests/plugins/simply/proxy_pass_normalized/missing_variable_nopath_fp.conf

@@ -0,0 +1,8 @@
+location /b {
+  rewrite ^ $request_uri; # Sets the $1/$uri variable to the raw path
+  proxy_pass http://127.0.0.1:8000$1; # $1 used in host, resulting in path being passed as the raw path from the original request.
+}
+
+# Request received by nginx: /%2F
+# Request received by backend: /%2F # Possibly also receives //%2F)
+# This is actually no different than proxy_pass_path_fp.conf since $1 is not a path.

tests/plugins/simply/proxy_pass_normalized/proxy_pass_path.conf

@@ -1,3 +1,6 @@
 location / {
-  proxy_pass http://server/;
+  proxy_pass http://server/; # Path "/" used, resulting in proxy_pass urldecoding the path.
 }
+
+# Request received by nginx: /%2F
+# Request received by backend: //

tests/plugins/simply/proxy_pass_normalized/proxy_pass_path_fp.conf

@@ -1,3 +1,6 @@
 location / {
-  proxy_pass http://downstream;
+  proxy_pass http://downstream; # No rewrite rules, and no path used: no extra/missing urldecoding/urlencoding occurs.
 }
+
+# Request received by nginx: /%2F
+# Request received by backend: /%2F

tests/plugins/simply/proxy_pass_normalized/rewrite_with_return_fp.conf

@@ -0,0 +1,9 @@
+location /1/ {
+  rewrite ^ $request_uri; # Sets $1/$uri to raw path.
+  rewrite ^/1(/.*) $1 break; # Extracts everything after /1 and places it into $1/$uri.
+  return 400; # extremely important! # If rewrite rule does not break (e.g. //1/), return 400, otherwise the location-block will fall-through and proxy_pass will not actually happen at all.
+  proxy_pass http://127.0.0.1:8080/$1; # $1 used, resulting in path being passed as the raw path from the original request.
+}
+
+# Request received by nginx: /1/%2F
+# Request received by backend: /%2F (or possibly //%2F)