launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/secure-headers into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #96878 in Launchpad itself: "Launchpad session cookie is accessible from Javascript"
  https://bugs.launchpad.net/launchpad/+bug/96878

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/secure-headers/+merge/95158

This branch makes five HTTP security enhancements. 

 - Strict-Transport-Security: since we serve all vhosts except feeds
   and xmlrpc-private over SSL, we should force browsers to never make
   unencrypted connections to them. I've set the TTL at 30 days.
   <http://en.wikipedia.org/wiki/HTTP_Strict_Transport_Security>

 - X-Frame-Options: we currently have no defenses against clickjacking.
   I've set X-Frame-Options to SAMEORIGIN, to prevent framing from
   outside the site. Ideally we'd use DENY for most things, but we use
   iframes for help and forms and various other things.
   <https://developer.mozilla.org/en/The_X-FRAME-OPTIONS_response_header>

 - X-XSS-Protection: IE and Chromium support a mechanism to prevent
   some types of reflected XSS attacks. I've set it to block mode, so
   detection causes the browser to refuse the render.
   <http://blogs.msdn.com/b/ieinternals/archive/2011/01/31/controlling-the-internet-explorer-xss-filter-with-the-x-xss-protection-http-header.aspx>

 - X-Content-Type-Options: IE has a bad habit of ignoring Content-Type
   headers, sometimes detecting it from the content. Setting nosniff
   prevents this obviously perilous behaviour.
   <http://blogs.msdn.com/b/ie/archive/2008/09/02/ie8-security-part-vi-beta-2-update.aspx>

 - HttpOnly: our auth cookies can currently be read by JavaScript and
   transmitted to the attacker. Marking the cookie HttpOnly means that
   attackers can only use it to make requests for the duration of an
   XSS attack, and not steal it for later additional nefariousness.
   <https://www.owasp.org/index.php/HttpOnly>
-- 
https://code.launchpad.net/~wgrant/launchpad/secure-headers/+merge/95158
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/secure-headers into lp:launchpad.

=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py	2012-01-05 00:23:45 +0000
+++ lib/lp/services/webapp/servers.py	2012-02-29 12:19:24 +0000
@@ -558,6 +558,8 @@
 
     implements(IBasicLaunchpadRequest)
 
+    strict_transport_security = True
+
     def __init__(self, body_instream, environ, response=None):
         self.traversed_objects = []
         self._wsgi_keys = set()
@@ -569,6 +571,18 @@
         # Our response always vary based on authentication.
         self.response.setHeader('Vary', 'Cookie, Authorization')
 
+        # Prevent clickjacking and content sniffing attacks.
+        self.response.setHeader('X-Frame-Options', 'SAMEORIGIN')
+        self.response.setHeader('X-Content-Type-Options', 'nosniff')
+        self.response.setHeader('X-XSS-Protection', '1; mode=block')
+
+        if self.strict_transport_security:
+            # And tell browsers that we always use SSL unless we're on
+            # an insecure vhost.
+            # 2592000 = 30 days in seconds
+            self.response.setHeader(
+                'Strict-Transport-Security', 'max-age=2592000')
+
     @property
     def stepstogo(self):
         return StepsToGo(self)
@@ -1106,6 +1120,9 @@
     """Request type for a launchpad feed."""
     implements(lp.layers.FeedsLayer)
 
+    # Feeds is not served over SSL, so don't force SSL.
+    strict_transport_security = False
+
 
 # ---- apidoc
 
@@ -1410,7 +1427,9 @@
 
 class PrivateXMLRPCRequest(PublicXMLRPCRequest):
     """Request type for doing private XML-RPC in Launchpad."""
-    # For now, the same as public requests.
+    # For now, the same as public requests except that there's no SSL.
+
+    strict_transport_security = False
 
 
 # ---- Protocol errors

=== modified file 'lib/lp/services/webapp/session.py'
--- lib/lp/services/webapp/session.py	2011-12-30 06:14:56 +0000
+++ lib/lp/services/webapp/session.py	2012-02-29 12:19:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Support for browser-cookie sessions."""
@@ -8,8 +8,6 @@
 from cookielib import domain_match
 
 from lazr.uri import URI
-from storm.zope.interfaces import IZStorm
-from zope.component import getUtility
 from zope.session.http import CookieClientIdManager
 
 from lp.services.config import config
@@ -43,6 +41,7 @@
 
 ANNOTATION_KEY = 'lp.services.webapp.session.sid'
 
+
 class LaunchpadCookieClientIdManager(CookieClientIdManager):
 
     def __init__(self):
@@ -105,6 +104,9 @@
         cookie = request.response.getCookie(self.namespace)
         uri = URI(request.getURL())
 
+        # Forbid browsers from exposing it to JS.
+        cookie['HttpOnly'] = True
+
         # Set secure flag on cookie.
         if uri.scheme != 'http':
             cookie['secure'] = True

=== modified file 'lib/lp/services/webapp/tests/cookie-authentication.txt'
--- lib/lp/services/webapp/tests/cookie-authentication.txt	2012-01-14 12:47:39 +0000
+++ lib/lp/services/webapp/tests/cookie-authentication.txt	2012-02-29 12:19:24 +0000
@@ -39,7 +39,7 @@
     >>> browser.cookies.keys()
     ['launchpad_tests']
     >>> session_cookie_name = browser.cookies.keys()[0]
-    >>> browser.cookies.getinfo(session_cookie_name)['domain']
+    >>> browser.cookies
     '.launchpad.dev'
 
 If we visit another vhost in the domain, we remain logged in.

=== modified file 'lib/lp/services/webapp/tests/test_servers.py'
--- lib/lp/services/webapp/tests/test_servers.py	2011-12-24 17:49:30 +0000
+++ lib/lp/services/webapp/tests/test_servers.py	2012-02-29 12:19:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E1002
@@ -36,8 +36,10 @@
 from lp.services.webapp.publication import LaunchpadBrowserPublication
 from lp.services.webapp.servers import (
     ApplicationServerSettingRequestFactory,
+    FeedsBrowserRequest,
     LaunchpadBrowserRequest,
     LaunchpadTestRequest,
+    PrivateXMLRPCRequest,
     VHostWebServiceRequestPublicationFactory,
     VirtualHostRequestPublicationFactory,
     web_service_request_to_browser_request,
@@ -366,6 +368,38 @@
             retried_request.response.getHeader('Vary'),
             'Cookie, Authorization')
 
+    def test_baserequest_security_headers(self):
+        response = LaunchpadBrowserRequest(StringIO.StringIO(''), {}).response
+        self.assertEquals(
+            response.getHeader('X-Frame-Options'), 'SAMEORIGIN')
+        self.assertEquals(
+            response.getHeader('X-Content-Type-Options'), 'nosniff')
+        self.assertEquals(
+            response.getHeader('X-XSS-Protection'), '1; mode=block')
+        self.assertEquals(
+            response.getHeader(
+                'Strict-Transport-Security'), 'max-age=2592000')
+
+
+class TestFeedsBrowserRequest(TestCase):
+    """Tests for `FeedsBrowserRequest`."""
+
+    def test_not_strict_transport_security(self):
+        # Feeds are served over HTTP, so no Strict-Transport-Security
+        # header is sent.
+        response = FeedsBrowserRequest(StringIO.StringIO(''), {}).response
+        self.assertIs(None, response.getHeader('Strict-Transport-Security'))
+
+
+class TestPrivateXMLRPCRequest(TestCase):
+    """Tests for `PrivateXMLRPCRequest`."""
+
+    def test_not_strict_transport_security(self):
+        # Private XML-RPC is served over HTTP, so no Strict-Transport-Security
+        # header is sent.
+        response = PrivateXMLRPCRequest(StringIO.StringIO(''), {}).response
+        self.assertIs(None, response.getHeader('Strict-Transport-Security'))
+
 
 class TestLaunchpadBrowserRequestMixin:
     """Tests for `LaunchpadBrowserRequestMixin`.

=== modified file 'lib/lp/services/webapp/tests/test_session.py'
--- lib/lp/services/webapp/tests/test_session.py	2011-12-24 17:49:30 +0000
+++ lib/lp/services/webapp/tests/test_session.py	2012-02-29 12:19:24 +0000
@@ -1,12 +1,17 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-import unittest
-
-from lp.services.webapp.session import get_cookie_domain
-
-
-class GetCookieDomainTestCase(unittest.TestCase):
+from testtools import TestCase
+from testtools.matchers import Contains
+
+from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.services.webapp.session import (
+    LaunchpadCookieClientIdManager,
+    get_cookie_domain,
+    )
+
+
+class GetCookieDomainTestCase(TestCase):
 
     def test_base_domain(self):
         # Test that the base Launchpad domain gives a domain parameter
@@ -38,3 +43,15 @@
                          '.launchpad.dev')
         self.assertEqual(get_cookie_domain('bugs.launchpad.dev'),
                          '.launchpad.dev')
+
+
+class TestLaunchpadCookieClientIdManager(TestCase):
+
+    def test_httponly(self):
+        # Authentication cookies are marked as httponly, so JavaScript
+        # can't read them directly.
+        request = LaunchpadTestRequest()
+        LaunchpadCookieClientIdManager().setRequestId(request, 'some-id')
+        self.assertThat(
+            dict(request.response.getHeaders())['Set-Cookie'],
+            Contains('; httponly;'))