Merge pull request from GHSA-vg46-2rrj-3647

#11716 Fix NameVirtualHost HTML injection and deprecate t.w.r.ErrorPage in favor of t.w.pages
twisted · Oct 26, 2022 · f2f5e81 · f2f5e81
2 parents 50761f4 + b0545bc
commit f2f5e81
Show file tree

Hide file tree

Showing 16 changed files with 416 additions and 68 deletions.
diff --git a/docs/web/howto/web-in-60/error-handling.rst b/docs/web/howto/web-in-60/error-handling.rst
@@ -32,32 +32,20 @@ As in the previous examples, we'll start with :py:class:`Site <twisted.web.serve
 
 
 
-Next, we'll add one more import. :py:class:`NoResource <twisted.web.resource.NoResource>` is one of the pre-defined error
+Next, we'll add one more import. :py:class:`notFound <twisted.web.pages.notFound>` is one of the pre-defined error
 resources provided by Twisted Web. It generates the necessary 404 response code
-and renders a simple html page telling the client there is no such resource.
-
-
-
-
+and renders a simple HTML page telling the client there is no such resource.
 
 .. code-block:: python
 
-
-    from twisted.web.resource import NoResource
-
-
-
+    from twisted.web.pages import notFound
 
 Next, we'll define a custom resource which does some dynamic URL
 dispatch. This example is going to be just like
 the :doc:`previous one <dynamic-dispatch>` , where the path segment is
 interpreted as a year; the difference is that this time we'll handle requests
 which don't conform to that pattern by returning the not found response:
 
-
-
-
-
 .. code-block:: python
 
 
@@ -66,7 +54,7 @@ which don't conform to that pattern by returning the not found response:
             try:
                 year = int(name)
             except ValueError:
-                return NoResource()
+                return notFound()
             else:
                 return YearPage(year)
 
@@ -88,7 +76,7 @@ complete code for this example:
     from twisted.web.server import Site
     from twisted.web.resource import Resource
     from twisted.internet import reactor, endpoints
-    from twisted.web.resource import NoResource
+    from twisted.web.pages import notFound
 
     from calendar import calendar
 
@@ -100,14 +88,14 @@ complete code for this example:
         def render_GET(self, request):
             cal = calendar(self.year)
             return (b"<!DOCTYPE html><html><head><meta charset='utf-8'>"
-                    b"<title></title></head><body><pre>" + cal.encode('utf-8') + "</pre>")
+                    b"<title></title></head><body><pre>" + cal.encode('utf-8') + b"</pre>")
 
     class Calendar(Resource):
         def getChild(self, name, request):
             try:
                 year = int(name)
             except ValueError:
-                return NoResource()
+                return notFound()
             else:
                 return YearPage(year)
 

diff --git a/src/twisted/web/_auth/wrapper.py b/src/twisted/web/_auth/wrapper.py
@@ -21,7 +21,7 @@
 from twisted.logger import Logger
 from twisted.python.components import proxyForInterface
 from twisted.web import util
-from twisted.web.resource import ErrorPage, IResource
+from twisted.web.resource import IResource, _UnsafeErrorPage
 
 
 @implementer(IResource)
@@ -52,7 +52,7 @@ def generateWWWAuthenticate(scheme, challenge):
             return b" ".join([scheme, b", ".join(lst)])
 
         def quoteString(s):
-            return b'"' + s.replace(b"\\", br"\\").replace(b'"', br"\"") + b'"'
+            return b'"' + s.replace(b"\\", rb"\\").replace(b'"', rb"\"") + b'"'
 
         request.setResponseCode(401)
         for fact in self._credentialFactories:
@@ -125,7 +125,7 @@ def _authorizedResource(self, request):
             return UnauthorizedResource(self._credentialFactories)
         except BaseException:
             self._log.failure("Unexpected failure from credentials factory")
-            return ErrorPage(500, None, None)
+            return _UnsafeErrorPage(500, "Internal Error", "")
         else:
             return util.DeferredResource(self._login(credentials))
 
@@ -213,7 +213,7 @@ def _loginFailed(self, result):
                 "unexpected error",
                 failure=result,
             )
-            return ErrorPage(500, None, None)
+            return _UnsafeErrorPage(500, "Internal Error", "")
 
     def _selectParseHeader(self, header):
         """

diff --git a/src/twisted/web/_template_util.py b/src/twisted/web/_template_util.py
@@ -1034,9 +1034,9 @@ class _TagFactory:
     """
     A factory for L{Tag} objects; the implementation of the L{tags} object.
 
-    This allows for the syntactic convenience of C{from twisted.web.html import
-    tags; tags.a(href="linked-page.html")}, where 'a' can be basically any HTML
-    tag.
+    This allows for the syntactic convenience of C{from twisted.web.template
+    import tags; tags.a(href="linked-page.html")}, where 'a' can be basically
+    any HTML tag.
 
     The class is not exposed publicly because you only ever need one of these,
     and we already made it for you.

diff --git a/src/twisted/web/distrib.py b/src/twisted/web/distrib.py
@@ -124,9 +124,10 @@ def failed(self, failure):
         # XXX: Argh. FIXME.
         failure = str(failure)
         self.request.write(
-            resource.ErrorPage(
+            resource._UnsafeErrorPage(
                 http.INTERNAL_SERVER_ERROR,
                 "Server Connection Lost",
+                # GHSA-vg46-2rrj-3647 note: _PRE does HTML-escape the input.
                 "Connection to distributed server lost:" + util._PRE(failure),
             ).render(self.request)
         )
@@ -376,7 +377,7 @@ def getChild(self, name, request):
                 pw_shell,
             ) = self._pwd.getpwnam(username.decode(sys.getfilesystemencoding()))
         except KeyError:
-            return resource.NoResource()
+            return resource._UnsafeNoResource()
         if sub:
             twistdsock = os.path.join(pw_dir, self.userSocketName)
             rs = ResourceSubscription("unix", twistdsock)
@@ -385,5 +386,5 @@ def getChild(self, name, request):
         else:
             path = os.path.join(pw_dir, self.userDirName)
             if not os.path.exists(path):
-                return resource.NoResource()
+                return resource._UnsafeNoResource()
             return static.File(path)
diff --git a/src/twisted/web/newsfragments/11716.bugfix b/src/twisted/web/newsfragments/11716.bugfix
@@ -0,0 +1 @@
+twisted.web.vhost.NameVirtualHost no longer echoes HTML received in the Host header without escaping it (CVE-2022-39348, GHSA-vg46-2rrj-3647).
diff --git a/src/twisted/web/newsfragments/11716.feature b/src/twisted/web/newsfragments/11716.feature
@@ -0,0 +1 @@
+The twisted.web.pages.errorPage, notFound, and forbidden each return an IResource that displays an HTML error pages safely rendered using twisted.web.template.
diff --git a/src/twisted/web/newsfragments/11716.removal b/src/twisted/web/newsfragments/11716.removal
@@ -0,0 +1 @@
+The twisted.web.resource.ErrorPage, NoResource, and ForbiddenResource classes have been deprecated in favor of new implementations twisted.web.pages module because they permit HTML injection.
diff --git a/src/twisted/web/pages.py b/src/twisted/web/pages.py
@@ -0,0 +1,134 @@
+# -*- test-case-name: twisted.web.test.test_pages -*-
+# Copyright (c) Twisted Matrix Laboratories.
+# See LICENSE for details.
+
+"""
+Utility implementations of L{IResource}.
+"""
+
+__all__ = (
+    "errorPage",
+    "notFound",
+    "forbidden",
+)
+
+from typing import cast
+
+from twisted.web import http
+from twisted.web.iweb import IRenderable, IRequest
+from twisted.web.resource import IResource, Resource
+from twisted.web.template import renderElement, tags
+
+
+class _ErrorPage(Resource):
+    """
+    L{_ErrorPage} is a resource that responds to all requests with a particular
+    (parameterized) HTTP status code and an HTML body containing some
+    descriptive text. This is useful for rendering simple error pages.
+
+    @see: L{twisted.web.pages.errorPage}
+
+    @ivar _code: An integer HTTP status code which will be used for the
+        response.
+
+    @ivar _brief: A short string which will be included in the response body as
+        the page title.
+
+    @ivar _detail: A longer string which will be included in the response body.
+    """
+
+    def __init__(self, code: int, brief: str, detail: str) -> None:
+        super().__init__()
+        self._code: int = code
+        self._brief: str = brief
+        self._detail: str = detail
+
+    def render(self, request: IRequest) -> object:
+        """
+        Respond to all requests with the given HTTP status code and an HTML
+        document containing the explanatory strings.
+        """
+        request.setResponseCode(self._code)
+        request.setHeader(b"content-type", b"text/html; charset=utf-8")
+        return renderElement(
+            request,
+            # cast because the type annotations here seem off; Tag isn't an
+            # IRenderable but also probably should be? See
+            # https://github.com/twisted/twisted/issues/4982
+            cast(
+                IRenderable,
+                tags.html(
+                    tags.head(tags.title(f"{self._code} - {self._brief}")),
+                    tags.body(tags.h1(self._brief), tags.p(self._detail)),
+                ),
+            ),
+        )
+
+    def getChild(self, path: bytes, request: IRequest) -> Resource:
+        """
+        Handle all requests for which L{_ErrorPage} lacks a child by returning
+        this error page.
+
+        @param path: A path segment.
+
+        @param request: HTTP request
+        """
+        return self
+
+
+def errorPage(code: int, brief: str, detail: str) -> IResource:
+    """
+    Build a resource that responds to all requests with a particular HTTP
+    status code and an HTML body containing some descriptive text. This is
+    useful for rendering simple error pages.
+
+    The resource dynamically handles all paths below it. Use
+    L{IResource.putChild()} override specific path.
+
+    @param code: An integer HTTP status code which will be used for the
+        response.
+
+    @param brief: A short string which will be included in the response
+        body as the page title.
+
+    @param detail: A longer string which will be included in the
+        response body.
+
+    @returns: An L{IResource}
+    """
+    return _ErrorPage(code, brief, detail)
+
+
+def notFound(
+    brief: str = "No Such Resource",
+    message: str = "Sorry. No luck finding that resource.",
+) -> IResource:
+    """
+    Generate an L{IResource} with a 404 Not Found status code.
+
+    @see: L{twisted.web.pages.errorPage}
+
+    @param brief: A short string displayed as the page title.
+
+    @param brief: A longer string displayed in the page body.
+
+    @returns: An L{IResource}
+    """
+    return _ErrorPage(http.NOT_FOUND, brief, message)
+
+
+def forbidden(
+    brief: str = "Forbidden Resource", message: str = "Sorry, resource is forbidden."
+) -> IResource:
+    """
+    Generate an L{IResource} with a 403 Forbidden status code.
+
+    @see: L{twisted.web.pages.errorPage}
+
+    @param brief: A short string displayed as the page title.
+
+    @param brief: A longer string displayed in the page body.
+
+    @returns: An L{IResource}
+    """
+    return _ErrorPage(http.FORBIDDEN, brief, message)