[PATCH 0/3] server security

[Johannes Berg]

Today, bup has no security against clients that are intended to
e.g. only write new commits to the repository deleting all of the
currently present ones (say in 'bup on') scenarios. Also clients
can write arbitrary data directly to the pack files, corrupting
the repository. Clients for 'bup on' can even change the repo and
access a whole different one.

This series is an attempt to lock down things at least a bit. The
data validation is only opt-in right now, it needs to decompress
and calculate the sha1, it might be expensive for some. Though it
stands to reason we should turn it on by default, and then make
it possible to turn it off.

The other validations are used when possible, e.g. with 'bup on'
the client can only do what's needed for the command, and can't
change the repository.

johannes

[Johannes Berg]

When a client writes to a bup server, e.g. via 'bup on host ...'
where the server is local and the client is on the 'host', the
client today will create and compress objects, and then write
them to the server. This has a few problems if the client isn't
entirely trusted:
- compression could be invalid
- the object type could be invalid
- the sha1 given by the client could be wrong
- tree, commit or tag objects that reference other
objects could be with unsatisfied references

Add an option bup.server.validate-writes which, when turned on,
makes the server validate all the above things (refusing tags
though, since those are never written by the client.)

Perhaps we should consider turning it on by default, and then
people for whom it's too expensive can turn it off explicitly.

Signed-off-by: Johannes Berg <joha...@sipsolutions.net>
---
Documentation/bup-config.5.md | 16 ++++++++++++++++
Documentation/bup-server.1.md | 5 +++++
lib/bup/cmd/server.py | 25 +++++++++++++++++++++++++
lib/bup/git.py | 5 +++++
lib/bup/protocol.py | 4 +---
5 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/Documentation/bup-config.5.md b/Documentation/bup-config.5.md
index 426299c53c03..5d8b133ef308 100644
--- a/Documentation/bup-config.5.md
+++ b/Documentation/bup-config.5.md
@@ -49,6 +49,22 @@ bup.server.deduplicate-writes (default `true`)
If no value is set, and a `$BUP_DIR/dumb-server-mode` file exists,
then `bup` will act as if this setting were `false`.

+bup.server.validate-writes (default `false`)
+: When `true` `bup-server`(1) checks each incoming object is valid,
+ i.e. checks that it
+
+ - decompresses correctly,
+
+ - has a valid object type,
+
+ - its calculated object ID matches the object ID sent by the client,
+
+ - other objects referenced by commits and trees are present in the
+ repository (referential integrity).
+
+ When `false` the server trusts the client to send only valid
+ objects.
+
bup.split.files (default `legacy:13`)
: Method used to split data for deduplication, for example by `bup
save` or `bup split`. This determines the "granularity" of the
diff --git a/Documentation/bup-server.1.md b/Documentation/bup-server.1.md
index cacb47c5be5c..8686ad56a518 100644
--- a/Documentation/bup-server.1.md
+++ b/Documentation/bup-server.1.md
@@ -23,6 +23,11 @@ The server's resource usage can be limited by setting
`bup.server.deduplicate-writes` to `false`. See the description in
`bup-config(5)` for additional information.

+In addition, the server can be made more strict and allow only
+writing valid objects that have their references fulfilled by
+setting `bup.server.validate-writes` to `true. See the description
+in `bup-config(5)` for additional information.
+
# FILES

$BUP_DIR/bup-dumb-server
diff --git a/lib/bup/cmd/server.py b/lib/bup/cmd/server.py
index 29cba644d572..8a0928d568cb 100644
--- a/lib/bup/cmd/server.py
+++ b/lib/bup/cmd/server.py
@@ -1,10 +1,12 @@

import sys
+from binascii import unhexlify

from bup import options, protocol, git
from bup.io import byte_stream
from bup.repo import LocalRepo
from bup.helpers import Conn, debug2
+from bup.commit import parse_commit


optspec = """
@@ -23,6 +25,29 @@ def main(argv):
def __init__(self, repo_dir, server):
git.check_repo_or_die(repo_dir)
LocalRepo.__init__(self, repo_dir, server=server)
+ self.validate = self.config_get(b'bup.server.validate-writes', opttype='bool')
+
+ def just_write_encoded(self, sha, content):
+ if self.validate:
+ objtype, uncompressed = git._decode_packobj(content)
+ if git.calc_hash(objtype, uncompressed) != sha:
+ raise Exception("invalid object received\n")
+
+ if objtype == b'commit':
+ info = parse_commit(uncompressed)
+ for oidx in info.parents:
+ if not self.exists(unhexlify(oidx)):
+ raise Exception('received commit has unknown parent')
+ if not self.exists(unhexlify(info.tree)):
+ raise Exception('received commit has unknown tree')
+ elif objtype == b'tree':
+ for _, _, oidx in git.tree_iter(uncompressed):
+ if not self.exists(oidx):
+ raise Exception('received tree has unknown reference')
+ elif objtype == b'tag':
+ raise Exception('received tag object - not permitted')
+
+ return self._packwriter.just_write_encoded(sha, content)

with Conn(byte_stream(sys.stdin), byte_stream(sys.stdout)) as conn, \
protocol.Server(conn, ServerRepo) as server:
diff --git a/lib/bup/git.py b/lib/bup/git.py
index c517a66e6c96..b1e5f23ed849 100644
--- a/lib/bup/git.py
+++ b/lib/bup/git.py
@@ -946,6 +946,11 @@ class PackWriter:
self._write(sha, type, content)
self._pending_oids.add(sha)

+ def just_write_encoded(self, sha, content):
+ sz, crc = self._store.write((content, ), sha=sha)
+ self._pending_oids.add(sha)
+ return crc
+
def maybe_write(self, type, content):
"""Write an object to the pack file if not present and return its id."""
sha = calc_hash(type, content)
diff --git a/lib/bup/protocol.py b/lib/bup/protocol.py
index 65eeb575660c..99a3cedc9542 100644
--- a/lib/bup/protocol.py
+++ b/lib/bup/protocol.py
@@ -294,9 +294,7 @@ class Server:
self.conn.write(b'index %s\n' % name)
suggested.add(name)
continue
- # FIXME: figure out the right abstraction for this; or better yet,
- # make the protocol aware of the object type
- nw, crc = self.repo._packwriter._store.write((buf,), sha=shar)
+ crc = self.repo.just_write_encoded(shar, buf)
self._check(crcr, crc, 'object read: expected crc %d, got %d\n')
assert False # should be unreachable

--
2.53.0

[Johannes Berg]

When a directory is forced, the client cannot arbitrarily
select any directory, it's always forced into this one. This
can be used with force SSH commands, i.e. if

command="bup --bup-dir /some/where/ server --force-repo"

is specified for a given authorized SSH key, the client will
not be able to write outside of "/some/where/".

Also use it when we do "bup on" since the reverse:// protocol
doesn't normally init/change the repo, but a malicious remote
could actually attempt to do so. Now, with this, it cannot
escape the bup dir we wanted to use, i.e.

bup -d /some/where/ on <remote> ...

again will not write outside of "/some/where/", regardless of
what the remote tries to do (unmodified bup will not try to
change it at all.)

Signed-off-by: Johannes Berg <joha...@sipsolutions.net>
---

Documentation/bup-server.1.md | 9 +++++++++
lib/bup/cmd/on.py | 4 +++-
lib/bup/cmd/server.py | 5 +++++
3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/bup-server.1.md b/Documentation/bup-server.1.md
index 8686ad56a518..da3bdec3c172 100644
--- a/Documentation/bup-server.1.md
+++ b/Documentation/bup-server.1.md
@@ -28,6 +28,15 @@ writing valid objects that have their references fulfilled by

setting `bup.server.validate-writes` to `true. See the description

in `bup-config(5)` for additional information.

+# OPTIONS
+
+\--force-repo
+: Force using the bup repository given in the environment or the
+ global *-d*/*\--bup-dir* option. This can be useful for ssh forced
+ commands (*command="..."* in an authorized_keys file) as it forces the
+ connection to use a given bup repository; it cannot read from
+ or write to any other location on the filesystem.
+
# FILES

$BUP_DIR/bup-dumb-server
diff --git a/lib/bup/cmd/on.py b/lib/bup/cmd/on.py
index 1d52eda76826..f3f502201c8f 100644
--- a/lib/bup/cmd/on.py
+++ b/lib/bup/cmd/on.py
@@ -48,7 +48,9 @@ def main(argv):
argvs = b'\0'.join([b'bup'] + argv)
p.stdin.write(struct.pack('!I', len(argvs)) + argvs)
p.stdin.flush()
- sp = subprocess.Popen([path.exe(), b'server'],
+ # we already put BUP_DIR into the environment, which
+ # is inherited here
+ sp = subprocess.Popen([path.exe(), b'server', b'--force-repo'],
stdin=p.stdout, stdout=p.stdin)
p.stdin.close()
p.stdout.close()
diff --git a/lib/bup/cmd/server.py b/lib/bup/cmd/server.py
index 8a0928d568cb..40f517d21377 100644
--- a/lib/bup/cmd/server.py
+++ b/lib/bup/cmd/server.py
@@ -11,6 +11,9 @@ from bup.commit import parse_commit

optspec = """
bup server
+--
+Options:
+force-repo force the configured (environment, --bup-dir) repository to be used
"""

def main(argv):
@@ -23,6 +26,8 @@ def main(argv):

class ServerRepo(LocalRepo):
def __init__(self, repo_dir, server):
+ if opt.force_repo:
+ repo_dir = None
git.check_repo_or_die(repo_dir)
LocalRepo.__init__(self, repo_dir, server=server)

self.validate = self.config_get(b'bup.server.validate-writes', opttype='bool')

--
2.53.0

[Johannes Berg]

To restrict what a remote bup can do on a server, e.g. in
'bup on' scenarios, add a --mode argument to the server,
adding the following modes:
- read,
- append,
- read-append and
- unrestricted.

To make this reasonably safe in the face of future additions,
this employs an allowlist of permitted commands, and is based
on a filter put into the base protocol class.

For 'bup on', even restrict when a server is started/connected,
and only do that if the remote command is explicitly listed as
requiring append, read, or both (unrestricted is not used used
right now as remote 'gc', 'rm' or 'prune' etc. aren't supported
at all over the server).

Right now, append is almost equivalent to unrestricted, except
it doesn't allow making ref updates that would drop commits or
writing to the config file.

Note that the mode 'read-append' is not used by 'bup on', it
may however be useful in conjunction with SSH forced commands.

Signed-off-by: Johannes Berg <joha...@sipsolutions.net>
---

Documentation/bup-server.1.md | 19 ++++++++++++++++
lib/bup/client.py | 4 ++--
lib/bup/cmd/on.py | 28 ++++++++++++++++++++----
lib/bup/cmd/server.py | 13 ++++++++++-
lib/bup/protocol.py | 41 +++++++++++++++++++++++++++++------
5 files changed, 91 insertions(+), 14 deletions(-)

diff --git a/Documentation/bup-server.1.md b/Documentation/bup-server.1.md
index da3bdec3c172..a1875e788798 100644
--- a/Documentation/bup-server.1.md
+++ b/Documentation/bup-server.1.md
@@ -37,6 +37,25 @@ in `bup-config(5)` for additional information.

connection to use a given bup repository; it cannot read from

or write to any other location on the filesystem.

+\--mode=*mode*
+: Set the server mode, the following modes are accepted:
+
+ *unrestricted*: No restrictions, this is the default if this option
+ is not given.
+
+ *append*: Data can only be written to this repository, and refs can
+ be updated. (Obviously, object existence can be proven since indexes
+ are needed to save data to a repository.)
+
+ *read-append*: Data can be written to and read back.
+
+ *read*: Data can only be read.
+
+ **NOTE**: Currently, the server doesn't support any destructive
+ operations, so *unrestricted* is really identical to *read-append*,
+ but as this may change in the future there's a difference already
+ to avoid breaking setups.
+
# FILES

$BUP_DIR/bup-dumb-server
diff --git a/lib/bup/client.py b/lib/bup/client.py
index d6358a0d9529..8509255e1075 100644
--- a/lib/bup/client.py
+++ b/lib/bup/client.py
@@ -285,13 +285,13 @@ class Client:
ctx.enter_context(self._transport)
self.conn = self._transport.conn
self._available_commands = self._get_available_commands()
- self._require_command(b'init-dir')
- self._require_command(b'set-dir')
if self.dir:
self.dir = re.sub(br'[\r\n]', b' ', self.dir)
if create:
+ self._require_command(b'init-dir')
self.conn.write(b'init-dir %s\n' % self.dir)
else:
+ self._require_command(b'set-dir')
self.conn.write(b'set-dir %s\n' % self.dir)
self.check_ok()
self.cachedir = self._prep_cache(self.host, self.port, self.dir)
diff --git a/lib/bup/cmd/on.py b/lib/bup/cmd/on.py
index f3f502201c8f..0a645176be48 100644
--- a/lib/bup/cmd/on.py
+++ b/lib/bup/cmd/on.py
@@ -48,10 +48,30 @@ def main(argv):

argvs = b'\0'.join([b'bup'] + argv)
p.stdin.write(struct.pack('!I', len(argvs)) + argvs)
p.stdin.flush()

- # we already put BUP_DIR into the environment, which
- # is inherited here
- sp = subprocess.Popen([path.exe(), b'server', b'--force-repo'],
- stdin=p.stdout, stdout=p.stdin)
+
+ # for commands not listed here don't even execute the server
+ # (e.g. bup on <host> index ...)
+ cmdmodes = {
+ b'get': b'unrestricted',
+ b'save': b'append',
+ b'split': b'append',
+ b'tag': b'append',
+ b'join': b'read',
+ b'cat-file': b'read',
+ b'ftp': b'read',
+ b'ls': b'read',
+ b'margin': b'read',
+ b'meta': b'read',
+ b'config': b'unrestricted',
+ }
+ mode = cmdmodes.get(argv[0], None)
+
+ if mode is not None:

+ # we already put BUP_DIR into the environment, which
+ # is inherited here
+ sp = subprocess.Popen([path.exe(), b'server', b'--force-repo',

+ b'--mode=' + mode],
+ stdin=p.stdout, stdout=p.stdin)
p.stdin.close()
p.stdout.close()
# Demultiplex remote client's stderr (back to stdout/stderr).
diff --git a/lib/bup/cmd/server.py b/lib/bup/cmd/server.py
index 40f517d21377..1574e40f7fd6 100644
--- a/lib/bup/cmd/server.py
+++ b/lib/bup/cmd/server.py
@@ -14,6 +14,7 @@ bup server
--
Options:

force-repo force the configured (environment, --bup-dir) repository to be used

+mode= server mode (unrestricted, append, read-append, read)
"""

def main(argv):
@@ -54,6 +55,16 @@ def main(argv):

return self._packwriter.just_write_encoded(sha, content)

+ def _restrict(server, commands):
+ for fn in dir(server):
+ if getattr(fn, 'bup_server_command', False):
+ if not fn in commands:
+ del server.fn
+
+ modes = ['unrestricted', 'append', 'read-append', 'read']
+ if opt.mode is not None and opt.mode not in modes:
+ o.fatal("server: invalid mode")
+

with Conn(byte_stream(sys.stdin), byte_stream(sys.stdout)) as conn, \

- protocol.Server(conn, ServerRepo) as server:
+ protocol.Server(conn, ServerRepo, mode=opt.mode) as server:
server.handle()
diff --git a/lib/bup/protocol.py b/lib/bup/protocol.py
index 99a3cedc9542..705c970399f2 100644
--- a/lib/bup/protocol.py
+++ b/lib/bup/protocol.py
@@ -157,21 +157,46 @@ def _command(fn):
return fn

class Server:
- def __init__(self, conn, backend):
+ def __init__(self, conn, backend, mode=None):
self.conn = conn
self._backend = backend
- self._commands = self._get_commands()
+ self._only_ff_updates = mode is not None and mode != 'unrestricted'
+ self._commands = self._get_commands(mode or 'unrestricted')
self.suspended = False
self.repo = None
self._deduplicate_writes = True

- def _get_commands(self):
+ def _get_commands(self, mode):
+ # always allow these - even if set-dir may actually be
+ # a no-op (if --force-repo is given)
+ permitted = set([b'quit', b'help', b'set-dir', b'list-indexes',
+ b'send-index', b'config-get', b'config-list'])
+
+ read_cmds = set([b'read-ref', b'join', b'cat-batch',
+ b'refs', b'rev-list', b'resolve'])
+ append_cmds = set([b'receive-objects-v2', b'read-ref', b'update-ref',
+ b'init-dir'])
+
+ if mode == 'unrestricted':
+ permitted = None # all commands permitted
+ elif mode == 'append':
+ permitted.update(append_cmds)
+ elif mode == 'read-append':
+ permitted.update(read_cmds)
+ permitted.update(append_cmds)
+ elif mode == 'read':
+ permitted.update(read_cmds)
+ else:
+ assert False # should be caught elsewhere
+
commands = []
for name in dir(self):
fn = getattr(self, name)

if getattr(fn, 'bup_server_command', False):
- commands.append(name.replace('_', '-').encode('ascii'))
+ cmdname = name.replace('_', '-').encode('ascii')
+ if permitted is None or cmdname in permitted:
+ commands.append(cmdname)

return commands

@@ -308,9 +333,11 @@ class Server:
@_command
def update_ref(self, refname):
self.init_session()
- newval = self.conn.readline().strip()
- oldval = self.conn.readline().strip()
- self.repo.update_ref(refname, unhexlify(newval), unhexlify(oldval))
+ newval = unhexlify(self.conn.readline().strip())
+ oldval = unhexlify(self.conn.readline().strip())
+ if self._only_ff_updates:
+ assert (self.repo.read_ref(refname) or b'') == oldval
+ self.repo.update_ref(refname, newval, oldval)
self.conn.ok()

@_command
--
2.53.0