Merge lp:~spiv/bzr/bug-400535 into lp:~bzr/bzr/trunk-old

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This patch fixes a serious bug in 1.16 and 1.17rc1. cmd_serve was refactored in
May to split out a “serve_bzr” function that takes care of creating a
ChrootServer for the specified directory and then starting a TCP or inet server
with that backing transport. Unfortunately, the refactoring introduced a bug
where the ChrootServer is ignored, and the plain LocalTransport is served
directly.

Thankfully, this doesn't appear to cause any security holes because invalid
paths like "../../../foo" are rejected by the smart server request handler
immediately, and requests that would try to walk up the filesystem to find a
BzrDir get stopped by the BzrDir.open jail that was recently introduced.

It's possible I've missed something and there is a way to exploit this bug to
trick an existing smart request into revealing data from outside the designated
directory, perhaps in conjunction with a server-side plugin, but so far I
haven't found any.

So, this patch fixes the essentially trivial bug, and adds a rather complicated
cmd_serve blackbox test that to prevent it regressing.

I suspect this bug also causes <https://bugs.launchpad.net/bzr/+bug/398199>, or
is at least contributing to it.

Because the bug is so serious, and because the fix is so obviously correct, this
fix should be included in 1.17 as well as bzr.dev.

-Andrew.

[Robert Collins (lifeless)]

 review +1

If backing_urls is in the local frame, then I suggest including it in
the new hook, if its an attribute on self, the the new hook signature is
fine.

Do we need to issue a new 1.16.x as well?

-Rob

review: Approve

[Andrew Bennetts (spiv)]

Robert Collins wrote:
> Review: Approve
> review +1
>
> If backing_urls is in the local frame, then I suggest including it in
> the new hook, if its an attribute on self, the the new hook signature is
> fine.

It's derivable from the server_obj, but not particularly conveniently so. I'll
add it.

> Do we need to issue a new 1.16.x as well?

I think that would be good, although perhaps with 1.17 so near it's not such a
big deal? I guess it's up to the RM and those that would build the installers
if they think it's worth the effort. I'm wavering between being +1 and +0.5 on
issuing a new 1.16.x for this.

-Andrew.

[Robert Collins (lifeless)]

I'd err on the side of more-effort where security concerns are present.

-Rob

[Martin Pool (mbp)]

2009/7/20 Robert Collins <email address hidden>:
> I'd err on the side of more-effort where security concerns are present.

How about if we talk to eg package maintainers before making applying
that effort, and see if they actually want a 1.16.1, or if perhaps
they just want the relevant patch?

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Mon, 2009-07-20 at 04:30 +0000, Martin Pool wrote:
> 2009/7/20 Robert Collins <email address hidden>:
> > I'd err on the side of more-effort where security concerns are present.
>
> How about if we talk to eg package maintainers before making applying
> that effort, and see if they actually want a 1.16.1, or if perhaps
> they just want the relevant patch?

Wearing my DD hat I'd want both - I can check that the new version is
really just the patch, then upload it without having to add a new patch
to the package which I'd just have to remove later.

-Rob

[Wouter van Heyst (larstiq)]

On Mon, Jul 20, 2009 at 01:10:17AM -0000, Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/bug-400535 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This patch fixes a serious bug in 1.16 and 1.17rc1. cmd_serve was refactored in
> May to split out a “serve_bzr” function that takes care of creating a
> ChrootServer for the specified directory and then starting a TCP or inet server
> with that backing transport. Unfortunately, the refactoring introduced a bug
> where the ChrootServer is ignored, and the plain LocalTransport is served
> directly.

Aha. I guess that explains why my smart-server locations.conf [chroot-*:///srv/bzr/]
section mysteriously stopped working with 1.16, and instead triggets on [/srv/bzr/].

I do think the latter one is much much nicer on the admin. Can I request
you retain the config behaviour for this merge, or do you want a
seperate bug for that?

Wouter van Heyst

[Robert Collins (lifeless)]

New bug please.

_Rob

=== modified file 'NEWS'
--- NEWS	2009-07-17 05:32:50 +0000
+++ NEWS	2009-07-20 07:35:31 +0000
@@ -33,6 +33,9 @@
 * ``bzr mv`` no longer takes out branch locks, which allows it to work
   when the branch is readonly. (Robert Collins, #216541)
 
+* ``bzr serve`` once again applies a ``ChrootServer`` to the given
+  directory before serving it. (Andrew Bennetts, #400535)
+  
 * Fixed spurious "Source branch does not support stacking" warning when
   pushing. (Andrew Bennetts, #388908)
   

=== modified file 'bzrlib/smart/server.py'
--- bzrlib/smart/server.py	2009-06-10 03:56:49 +0000
+++ bzrlib/smart/server.py	2009-07-20 07:35:31 +0000
@@ -111,6 +111,8 @@
             pass
         for hook in SmartTCPServer.hooks['server_started']:
             hook(backing_urls, self.get_url())
+        for hook in SmartTCPServer.hooks['server_started_ex']:
+            hook(backing_urls, self)
         self._started.set()
         try:
             try:
@@ -214,6 +216,10 @@
             "where backing_url is a list of URLs giving the "
             "server-specific directory locations, and public_url is the "
             "public URL for the directory being served.", (0, 16), None))
+        self.create_hook(HookPoint('server_started_ex',
+            "Called by the bzr server when it starts serving a directory. "
+            "server_started is called with (backing_urls, server_obj).",
+            (1, 17), None))
         self.create_hook(HookPoint('server_stopped',
             "Called by the bzr server when it stops serving a directory. "
             "server_stopped is called with the same parameters as the "
@@ -313,7 +319,7 @@
     from bzrlib.transport.chroot import ChrootServer
     chroot_server = ChrootServer(transport)
     chroot_server.setUp()
-    t = get_transport(chroot_server.get_url())
+    transport = get_transport(chroot_server.get_url())
     if inet:
         smart_server = medium.SmartServerPipeStreamMedium(
             sys.stdin, sys.stdout, transport)
@@ -322,8 +328,7 @@
             host = medium.BZR_DEFAULT_INTERFACE
         if port is None:
             port = medium.BZR_DEFAULT_PORT
-        smart_server = SmartTCPServer(
-            transport, host=host, port=port)
+        smart_server = SmartTCPServer(transport, host=host, port=port)
         trace.note('listening on port: %s' % smart_server.port)
     # For the duration of this server, no UI output is permitted. note
     # that this may cause problems with blackbox tests. This should be

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2009-06-10 03:56:49 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2009-07-20 07:35:31 +0000
@@ -21,18 +21,22 @@
 import signal
 import subprocess
 import sys
+import thread
 import threading
 
 from bzrlib import (
     errors,
     osutils,
     revision as _mod_revision,
+    transport,
     )
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import BzrDir
 from bzrlib.errors import ParamikoNotPresent
-from bzrlib.smart import medium
+from bzrlib.smart import client, medium
+from bzrlib.smart.server import SmartTCPServer
 from bzrlib.tests import TestCaseWithTransport, TestSkipped
+from bzrlib.trace import mutter
 from bzrlib.transport import get_transport, remote
 
 
@@ -239,3 +243,77 @@
              % bzr_remote_path],
             self.command_executed)
 
+
+class TestCmdServeChrooting(TestCaseWithTransport):
+
+    def test_serve_tcp(self):
+        """'bzr serve' wraps the given --directory in a ChrootServer.
+
+        So requests that search up through the parent directories (like
+        find_repositoryV3) will give "not found" responses, rather than
+        InvalidURLJoin or jail break errors.
+        """
+        t = self.get_transport()
+        t.mkdir('server-root')
+        self.run_bzr_serve_then_func(
+            ['--port', '0', '--directory', t.local_abspath('server-root'),
+             '--allow-writes'],
+            self.when_server_started)
+        # The when_server_started method issued a find_repositoryV3 that should
+        # fail with 'norepository' because there are no repositories inside the
+        # --directory.
+        self.assertEqual(('norepository',), self.client_resp)
+        
+    def run_bzr_serve_then_func(self, serve_args, func, *func_args,
+            **func_kwargs):
+        """Run 'bzr serve', and run the given func in a thread once the server
+        has started.
+        
+        When 'func' terminates, the server will be terminated too.
+        """
+        # install hook
+        def on_server_start(backing_urls, tcp_server):
+            t = threading.Thread(
+                target=on_server_start_thread, args=(tcp_server,))
+            t.start()
+        def on_server_start_thread(tcp_server):
+            try:
+                # Run func
+                self.tcp_server = tcp_server
+                try:
+                    func(*func_args, **func_kwargs)
+                except Exception, e:
+                    # Log errors to make some test failures a little less
+                    # mysterious.
+                    mutter('func broke: %r', e)
+            finally:
+                # Then stop the server
+                mutter('interrupting...')
+                thread.interrupt_main()
+        SmartTCPServer.hooks.install_named_hook(
+            'server_started_ex', on_server_start,
+            'run_bzr_serve_then_func hook')
+        # start a TCP server
+        try:
+            self.run_bzr(['serve'] + list(serve_args))
+        except KeyboardInterrupt:
+            pass
+
+    def when_server_started(self):
+        # Connect to the TCP server and issue some requests and see what comes
+        # back.
+        client_medium = medium.SmartTCPClientMedium(
+            '127.0.0.1', self.tcp_server.port,
+            'bzr://localhost:%d/' % (self.tcp_server.port,))
+        smart_client = client._SmartClient(client_medium)
+        resp = smart_client.call('mkdir', 'foo', '')
+        resp = smart_client.call('BzrDirFormat.initialize', 'foo/')
+        try:
+            resp = smart_client.call('BzrDir.find_repositoryV3', 'foo/')
+        except errors.ErrorFromSmartServer, e:
+            resp = e.error_tuple
+        self.client_resp = resp
+        client_medium.disconnect()
+
+
+