Merge lp:~mkanat/loggerhead/raw-prefix into lp:loggerhead

[Max Kanat-Alexander]

Add a --raw-prefix option for XSS protection on raw content.

This adds XSS protection to the raw-controller branch, by adding an option to serve raw content from a separate branch. I wanted this to be a separate landing so that it could get its own specific review. The docs I added for --raw-prefix should explain everything (and if they don't, then the docs should be updated).

[Martin Pool (mbp)]

That looks good, and it's a nice clean fix.

iirc, there are now 3 options

0- don't serve arbitrary content except as attachments
1- serve it within the standard domain (trust it not to be malicious)
2- serve it from a different domain (with --raw-prefix)

With this and it's dependency merged, what's the default, 0 or 1? I hope it's 0 and you need to opt in to 1.

review: Approve

[Max Kanat-Alexander (mkanat)]

The options are only 1 and 2, actually. The default is 1. I opted for convenience and discoverability over security in this case, because nearly every loggerhead installation doesn't have any need for security. That is, there's nothing dangerous that an attacker could do with an XSS on a loggerhead installation in most places, because loggerhead is not a container for any sensitive information. It doesn't even have cookies or a login system.

[Martin Pool (mbp)]

On 5 January 2011 12:39, Max Kanat-Alexander <email address hidden> wrote:
> The options are only 1 and 2, actually. The default is 1. I opted for convenience and discoverability over security in this case, because nearly every loggerhead installation doesn't have any need for security. That is, there's nothing dangerous that an attacker could do with an XSS on a loggerhead installation in most places, because loggerhead is not a container for any sensitive information. It doesn't even have cookies or a login system.

Fair enough.

--
Martin

=== modified file 'docs/serve-branches.rst'
--- docs/serve-branches.rst	2010-03-25 10:32:47 +0000
+++ docs/serve-branches.rst	2010-12-24 22:52:57 +0000
@@ -72,7 +72,24 @@
 
 .. cmdoption:: --use-cdn
    
-    Serve YUI javascript libraries from Yahoo!'s CDN.
+    Serve YUI javascript libraries from Yahoo!'s CDN.\
+    
+.. cmdoption:: --raw-prefix=RAW_PREFIX
+   
+    Serve raw files from a separate URL than the normal URL
+    that loggerhead runs on. This URL should have a different domain name
+    than your loggerhead installation normally has, but should resolve
+    to this same loggerhead installation. This is an important security
+    protection that you should use if you can, as it will protect you against
+    Cross-Site Scripting attacks from malicious content in repositories.
+
+    If you put %branch% into the URL, it will be replaced with the full
+    path to the current branch, replacing every / with _. This allows you
+    to have separate domain names for each branch, enhancing the protection
+    provided by this option.
+    
+    Unike ``--prefix``, this is a full URL. If you also use ``--prefix``, that
+    value should not be included at the end of this URL.
 
 .. cmdoption:: --allow-writes
    

=== modified file 'loggerhead/apps/transport.py'
--- loggerhead/apps/transport.py	2010-05-14 07:53:31 +0000
+++ loggerhead/apps/transport.py	2010-12-24 22:52:57 +0000
@@ -16,6 +16,7 @@
 #
 """Serve branches at urls that mimic a transport's file system layout."""
 
+import string
 import threading
 
 from bzrlib import branch, errors, urlutils
@@ -23,7 +24,7 @@
 from bzrlib.transport import get_transport
 from bzrlib.transport.http import wsgi
 
-from paste.request import path_info_pop
+from paste.request import path_info_pop, construct_url
 from paste import httpexceptions
 from paste import urlparser
 
@@ -49,6 +50,7 @@
         self._config = root._config
 
     def app_for_branch(self, branch):
+        self.check_raw_prefix(True)
         if not self.name:
             name = branch._get_nick(local=True)
             is_root = True
@@ -74,6 +76,7 @@
                 name = self.name
             else:
                 name = '/'
+            self.check_raw_prefix(False)
             return DirectoryUI(
                 environ['loggerhead.static.url'], self.transport, name)
         else:
@@ -85,6 +88,7 @@
             return BranchesFromTransportServer(new_transport, self.root, new_name)
 
     def app_for_bazaar_data(self, relpath):
+        self.check_raw_prefix(False)
         if relpath == '/.bzr/smart':
             root_transport = get_transport_for_thread(self.root.base)
             wsgi_app = wsgi.SmartWSGIApp(root_transport)
@@ -100,6 +104,43 @@
             else:
                 return urlparser.make_static(None, path)
 
+    def check_raw_prefix(self, is_branch):
+        raw_prefix = self._config.get_option('raw_prefix');
+        if raw_prefix is None:
+            return
+        environ = self._environ
+
+        # Replace %branch% with an escaped version of the branch name.
+        if '%branch%' in raw_prefix:
+            branch = environ['SCRIPT_NAME']
+            branch = branch.strip('/')
+            # Only allow numbers and letters in domain names. Convert
+            # everything else to an underscore.
+            domain_chars = string.ascii_letters + string.digits
+            branch = ['_' if i not in domain_chars else i for i in branch]
+            branch = ''.join(branch)
+            raw_prefix = raw_prefix.replace('%branch%', branch)
+
+        base_url = construct_url(environ, with_path_info=False,
+                                 with_query_string=False)
+        path = environ['PATH_INFO']
+
+        # The /raw controller may only be served over raw_prefix.
+        if is_branch and path.startswith('/raw'):
+            if not base_url.startswith(raw_prefix):
+                url_end = environ.get('SCRIPT_NAME','') \
+                          + environ.get('PATH_INFO', '')
+                if environ.get('QUERY_STRING'):
+                    url_end += '?' + environ['QUERY_STRING']
+                url_to = raw_prefix + url_end
+                raise httpexceptions.HTTPFound(url_to)
+        # No other URLs may be served over raw_prefix (besides
+        # static URLs) because they could associate session cookies
+        # or important authentication info with raw domain, and
+        # we don't want to allow that.
+        elif base_url.startswith(raw_prefix):
+            raise httpexceptions.HTTPNotFound()
+
     def check_serveable(self, config):
         value = config.get_user_option('http_serve')
         if value is None:
@@ -108,6 +149,10 @@
             raise httpexceptions.HTTPNotFound()
 
     def __call__(self, environ, start_response):
+        # check_raw_prefix needs this, and it's simpler to set it here
+        # then pass it around everywhere it's needed. check_raw_prefix
+        # is only called after this point.
+        self._environ = environ
         path = environ['PATH_INFO']
         try:
             b = branch.Branch.open_from_transport(self.transport)

=== modified file 'loggerhead/config.py'
--- loggerhead/config.py	2010-11-29 10:07:55 +0000
+++ loggerhead/config.py	2010-12-24 22:52:57 +0000
@@ -71,6 +71,9 @@
                       help="Print the software version and exit")
     parser.add_option("--use-cdn", action="store_true", dest="use_cdn",
                       help="Serve YUI from Yahoo!'s CDN")
+    parser.add_option("--raw-prefix", action='callback',
+                      callback=_fix_raw_prefix, type="string",
+                      help="Serve raw files from this different base URL.")
     parser.add_option("--cache-dir", dest="sql_dir",
                       help="The directory to place the SQL cache in")
     parser.add_option("--allow-writes", action="store_true",
@@ -86,6 +89,9 @@
     'critical': 50,
 }
 
+def _fix_raw_prefix(option, opt_str, value, parser):
+    parser.values.raw_prefix = value.rstrip('/')
+
 def _optparse_level_to_int_level(option, opt_str, value, parser):
     parser.values.log_level = _level_to_int_level(value)