Merge lp:~spiv/bzr/bug-400535 into lp:~bzr/bzr/trunk-old
- bug-400535
- Merge into trunk-old
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | not available | ||||
Proposed branch: | lp:~spiv/bzr/bug-400535 | ||||
Merge into: | lp:~bzr/bzr/trunk-old | ||||
Diff against target: | 162 lines | ||||
To merge this branch: | bzr merge lp:~spiv/bzr/bug-400535 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Approve | ||
Review via email: mp+9031@code.launchpad.net |
Commit message
Description of the change
Andrew Bennetts (spiv) wrote : | # |
Robert Collins (lifeless) wrote : | # |
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
Andrew Bennetts (spiv) wrote : | # |
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) wrote : | # |
I'd err on the side of more-effort where security concerns are present.
-Rob
Martin Pool (mbp) 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?
--
Martin <http://
Robert Collins (lifeless) wrote : | # |
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) wrote : | # |
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-
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) wrote : | # |
New bug please.
_Rob
Preview Diff
1 | === modified file 'NEWS' | |||
2 | --- NEWS 2009-07-17 05:32:50 +0000 | |||
3 | +++ NEWS 2009-07-20 07:35:31 +0000 | |||
4 | @@ -33,6 +33,9 @@ | |||
5 | 33 | * ``bzr mv`` no longer takes out branch locks, which allows it to work | 33 | * ``bzr mv`` no longer takes out branch locks, which allows it to work |
6 | 34 | when the branch is readonly. (Robert Collins, #216541) | 34 | when the branch is readonly. (Robert Collins, #216541) |
7 | 35 | 35 | ||
8 | 36 | * ``bzr serve`` once again applies a ``ChrootServer`` to the given | ||
9 | 37 | directory before serving it. (Andrew Bennetts, #400535) | ||
10 | 38 | |||
11 | 36 | * Fixed spurious "Source branch does not support stacking" warning when | 39 | * Fixed spurious "Source branch does not support stacking" warning when |
12 | 37 | pushing. (Andrew Bennetts, #388908) | 40 | pushing. (Andrew Bennetts, #388908) |
13 | 38 | 41 | ||
14 | 39 | 42 | ||
15 | === modified file 'bzrlib/smart/server.py' | |||
16 | --- bzrlib/smart/server.py 2009-06-10 03:56:49 +0000 | |||
17 | +++ bzrlib/smart/server.py 2009-07-20 07:35:31 +0000 | |||
18 | @@ -111,6 +111,8 @@ | |||
19 | 111 | pass | 111 | pass |
20 | 112 | for hook in SmartTCPServer.hooks['server_started']: | 112 | for hook in SmartTCPServer.hooks['server_started']: |
21 | 113 | hook(backing_urls, self.get_url()) | 113 | hook(backing_urls, self.get_url()) |
22 | 114 | for hook in SmartTCPServer.hooks['server_started_ex']: | ||
23 | 115 | hook(backing_urls, self) | ||
24 | 114 | self._started.set() | 116 | self._started.set() |
25 | 115 | try: | 117 | try: |
26 | 116 | try: | 118 | try: |
27 | @@ -214,6 +216,10 @@ | |||
28 | 214 | "where backing_url is a list of URLs giving the " | 216 | "where backing_url is a list of URLs giving the " |
29 | 215 | "server-specific directory locations, and public_url is the " | 217 | "server-specific directory locations, and public_url is the " |
30 | 216 | "public URL for the directory being served.", (0, 16), None)) | 218 | "public URL for the directory being served.", (0, 16), None)) |
31 | 219 | self.create_hook(HookPoint('server_started_ex', | ||
32 | 220 | "Called by the bzr server when it starts serving a directory. " | ||
33 | 221 | "server_started is called with (backing_urls, server_obj).", | ||
34 | 222 | (1, 17), None)) | ||
35 | 217 | self.create_hook(HookPoint('server_stopped', | 223 | self.create_hook(HookPoint('server_stopped', |
36 | 218 | "Called by the bzr server when it stops serving a directory. " | 224 | "Called by the bzr server when it stops serving a directory. " |
37 | 219 | "server_stopped is called with the same parameters as the " | 225 | "server_stopped is called with the same parameters as the " |
38 | @@ -313,7 +319,7 @@ | |||
39 | 313 | from bzrlib.transport.chroot import ChrootServer | 319 | from bzrlib.transport.chroot import ChrootServer |
40 | 314 | chroot_server = ChrootServer(transport) | 320 | chroot_server = ChrootServer(transport) |
41 | 315 | chroot_server.setUp() | 321 | chroot_server.setUp() |
43 | 316 | t = get_transport(chroot_server.get_url()) | 322 | transport = get_transport(chroot_server.get_url()) |
44 | 317 | if inet: | 323 | if inet: |
45 | 318 | smart_server = medium.SmartServerPipeStreamMedium( | 324 | smart_server = medium.SmartServerPipeStreamMedium( |
46 | 319 | sys.stdin, sys.stdout, transport) | 325 | sys.stdin, sys.stdout, transport) |
47 | @@ -322,8 +328,7 @@ | |||
48 | 322 | host = medium.BZR_DEFAULT_INTERFACE | 328 | host = medium.BZR_DEFAULT_INTERFACE |
49 | 323 | if port is None: | 329 | if port is None: |
50 | 324 | port = medium.BZR_DEFAULT_PORT | 330 | port = medium.BZR_DEFAULT_PORT |
53 | 325 | smart_server = SmartTCPServer( | 331 | smart_server = SmartTCPServer(transport, host=host, port=port) |
52 | 326 | transport, host=host, port=port) | ||
54 | 327 | trace.note('listening on port: %s' % smart_server.port) | 332 | trace.note('listening on port: %s' % smart_server.port) |
55 | 328 | # For the duration of this server, no UI output is permitted. note | 333 | # For the duration of this server, no UI output is permitted. note |
56 | 329 | # that this may cause problems with blackbox tests. This should be | 334 | # that this may cause problems with blackbox tests. This should be |
57 | 330 | 335 | ||
58 | === modified file 'bzrlib/tests/blackbox/test_serve.py' | |||
59 | --- bzrlib/tests/blackbox/test_serve.py 2009-06-10 03:56:49 +0000 | |||
60 | +++ bzrlib/tests/blackbox/test_serve.py 2009-07-20 07:35:31 +0000 | |||
61 | @@ -21,18 +21,22 @@ | |||
62 | 21 | import signal | 21 | import signal |
63 | 22 | import subprocess | 22 | import subprocess |
64 | 23 | import sys | 23 | import sys |
65 | 24 | import thread | ||
66 | 24 | import threading | 25 | import threading |
67 | 25 | 26 | ||
68 | 26 | from bzrlib import ( | 27 | from bzrlib import ( |
69 | 27 | errors, | 28 | errors, |
70 | 28 | osutils, | 29 | osutils, |
71 | 29 | revision as _mod_revision, | 30 | revision as _mod_revision, |
72 | 31 | transport, | ||
73 | 30 | ) | 32 | ) |
74 | 31 | from bzrlib.branch import Branch | 33 | from bzrlib.branch import Branch |
75 | 32 | from bzrlib.bzrdir import BzrDir | 34 | from bzrlib.bzrdir import BzrDir |
76 | 33 | from bzrlib.errors import ParamikoNotPresent | 35 | from bzrlib.errors import ParamikoNotPresent |
78 | 34 | from bzrlib.smart import medium | 36 | from bzrlib.smart import client, medium |
79 | 37 | from bzrlib.smart.server import SmartTCPServer | ||
80 | 35 | from bzrlib.tests import TestCaseWithTransport, TestSkipped | 38 | from bzrlib.tests import TestCaseWithTransport, TestSkipped |
81 | 39 | from bzrlib.trace import mutter | ||
82 | 36 | from bzrlib.transport import get_transport, remote | 40 | from bzrlib.transport import get_transport, remote |
83 | 37 | 41 | ||
84 | 38 | 42 | ||
85 | @@ -239,3 +243,77 @@ | |||
86 | 239 | % bzr_remote_path], | 243 | % bzr_remote_path], |
87 | 240 | self.command_executed) | 244 | self.command_executed) |
88 | 241 | 245 | ||
89 | 246 | |||
90 | 247 | class TestCmdServeChrooting(TestCaseWithTransport): | ||
91 | 248 | |||
92 | 249 | def test_serve_tcp(self): | ||
93 | 250 | """'bzr serve' wraps the given --directory in a ChrootServer. | ||
94 | 251 | |||
95 | 252 | So requests that search up through the parent directories (like | ||
96 | 253 | find_repositoryV3) will give "not found" responses, rather than | ||
97 | 254 | InvalidURLJoin or jail break errors. | ||
98 | 255 | """ | ||
99 | 256 | t = self.get_transport() | ||
100 | 257 | t.mkdir('server-root') | ||
101 | 258 | self.run_bzr_serve_then_func( | ||
102 | 259 | ['--port', '0', '--directory', t.local_abspath('server-root'), | ||
103 | 260 | '--allow-writes'], | ||
104 | 261 | self.when_server_started) | ||
105 | 262 | # The when_server_started method issued a find_repositoryV3 that should | ||
106 | 263 | # fail with 'norepository' because there are no repositories inside the | ||
107 | 264 | # --directory. | ||
108 | 265 | self.assertEqual(('norepository',), self.client_resp) | ||
109 | 266 | |||
110 | 267 | def run_bzr_serve_then_func(self, serve_args, func, *func_args, | ||
111 | 268 | **func_kwargs): | ||
112 | 269 | """Run 'bzr serve', and run the given func in a thread once the server | ||
113 | 270 | has started. | ||
114 | 271 | |||
115 | 272 | When 'func' terminates, the server will be terminated too. | ||
116 | 273 | """ | ||
117 | 274 | # install hook | ||
118 | 275 | def on_server_start(backing_urls, tcp_server): | ||
119 | 276 | t = threading.Thread( | ||
120 | 277 | target=on_server_start_thread, args=(tcp_server,)) | ||
121 | 278 | t.start() | ||
122 | 279 | def on_server_start_thread(tcp_server): | ||
123 | 280 | try: | ||
124 | 281 | # Run func | ||
125 | 282 | self.tcp_server = tcp_server | ||
126 | 283 | try: | ||
127 | 284 | func(*func_args, **func_kwargs) | ||
128 | 285 | except Exception, e: | ||
129 | 286 | # Log errors to make some test failures a little less | ||
130 | 287 | # mysterious. | ||
131 | 288 | mutter('func broke: %r', e) | ||
132 | 289 | finally: | ||
133 | 290 | # Then stop the server | ||
134 | 291 | mutter('interrupting...') | ||
135 | 292 | thread.interrupt_main() | ||
136 | 293 | SmartTCPServer.hooks.install_named_hook( | ||
137 | 294 | 'server_started_ex', on_server_start, | ||
138 | 295 | 'run_bzr_serve_then_func hook') | ||
139 | 296 | # start a TCP server | ||
140 | 297 | try: | ||
141 | 298 | self.run_bzr(['serve'] + list(serve_args)) | ||
142 | 299 | except KeyboardInterrupt: | ||
143 | 300 | pass | ||
144 | 301 | |||
145 | 302 | def when_server_started(self): | ||
146 | 303 | # Connect to the TCP server and issue some requests and see what comes | ||
147 | 304 | # back. | ||
148 | 305 | client_medium = medium.SmartTCPClientMedium( | ||
149 | 306 | '127.0.0.1', self.tcp_server.port, | ||
150 | 307 | 'bzr://localhost:%d/' % (self.tcp_server.port,)) | ||
151 | 308 | smart_client = client._SmartClient(client_medium) | ||
152 | 309 | resp = smart_client.call('mkdir', 'foo', '') | ||
153 | 310 | resp = smart_client.call('BzrDirFormat.initialize', 'foo/') | ||
154 | 311 | try: | ||
155 | 312 | resp = smart_client.call('BzrDir.find_repositoryV3', 'foo/') | ||
156 | 313 | except errors.ErrorFromSmartServer, e: | ||
157 | 314 | resp = e.error_tuple | ||
158 | 315 | self.client_resp = resp | ||
159 | 316 | client_medium.disconnect() | ||
160 | 317 | |||
161 | 318 | |||
162 | 319 |
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.