make-secnet-sites: prefix names when writing sites file

SUMMARY

make-secnet-sites must copy names (vpn, location and site names) from
the input sites file (which is not wholly trusted) to the secnet
config file.

Since secnet 0.5.0 we check these name strings for syntactic sanity.

However, they still live in a common namespace.  When secnet is
evaluating a part of the config file, it looks up through the
containing structures.

It is possible for a sites file to specify, say, a site called "dh" or
something.  These currently generate bad configuration files.

IMPLEMENTATION

There are 4 lines of code which look like a nascent attempt to fix
this problem but (i) they are wrong and (ii) the reserved list is not
actually used anywhere.

Instead, we fix this by prefixing vpn names with V, location names
with L, and site names with S.  These obviously won't clash with
conventional names, predefined (builtin) closures, etc.  This is fine
because sensible configurations do not refer to the individual
make-secnet-sites config key names directly.

We achieve this by replacing the relevant references to the node's
name with calls to a new kname() method on the `level' class.  The new
behaviour is made optional, for compatibility reasons, but is enabled
by default.

We must disable this feature in the make-secnet-site tests because we
have a fixed expected output.  But the tests with actual secnet are
oblivious, since "stest: Use top-level mss-generated `all-sites' key".

THREAT ANALYSIS

An attacker can't do anything very interesting with this. right now.
A malicious sites file can generate a configuration file which secnet
will reject.  This is a nuisance but right now we don't have a system
for automatically incorporating sites data.  So I am not treating this
as a vulnerability.

It's not a more serious problem because the attacker can only bind the
keys to dictionaries containing site information written by
make-secnet-sites.  When the secnet configuration consumer code looks
something up and gets a dictionary when it was expecting something
else, it is an error.

There are few places where a dictionary is expected:

Path lookups expect a dictionary, but the only interesting scopes that
the attacker can pollute are the per-site config dictionaries, which
do not refer to paths, only individual keys.  (The "vpn" and
"all-sites" constructs from make-secnet sites contain path references,
but all the attacker can do there is to rebind what is supposed to be
an entry in the root namespace, resulting in a path which cannot be
resolved because it looks for too many subcomponents.)

There aren't currently any conventional keys with dictionary values
other than site's "comm-info" where the only key is
"dedicated-interface-addr".  The attacker could define a location
"comm-info" with a site "dedicated-interface-addr", which would be
interpreted as by peer sites as a "dedicated-interface-addr" key
referrinng to a dictionary (an error), or a site "comm-info" which
wouldn't contain that subkey at all and would therefore have no
effect.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/make-secnet-sites b/make-secnet-sites
index cd8f5dbb6fa01f03bae571a765b2a81e3b41e384..4be0160f8e2d5a8564f8da3dc593d5464de563a2 100755 (executable)
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -71,6 +71,8 @@ sys.path.insert(1,"/usr/local/share/secnet")
 sys.path.insert(1,"/usr/share/secnet")
 import ipaddrset
 
+from argparseactionnoyes import ActionNoYes
+
 VERSION="0.1.18"
 
 from sys import version_info
@@ -212,16 +214,21 @@ def parse_args():
        global group
        global user
        global of
+       global key_prefix
 
        ap = argparse.ArgumentParser(description='process secnet sites files')
        ap.add_argument('--userv', '-u', action='store_true',
                        help='userv service fragment update mode')
+       ap.add_argument('--conf-key-prefix', action=ActionNoYes,
+                       default=True,
+                help='prefix conf file key names derived from sites data')
        ap.add_argument('--prefix', '-P', nargs=1,
                        help='set prefix')
        ap.add_argument('arg',nargs=argparse.REMAINDER)
        av = ap.parse_args()
        #print(repr(av), file=sys.stderr)
        service = 1 if av.userv else 0
+       key_prefix = av.conf_key_prefix
        if service:
                if len(av.arg)!=4:
                        print("Wrong number of arguments")
@@ -414,10 +421,13 @@ class level:
                        if self.allow_properties[i]:
                                self.indent(w,ind)
                                w.write("%s"%self.prop_out(i))
+       def kname(self):
+               return ((self.type[0].upper() if key_prefix else '')
+                       + self.name)
        def output_data(self,w,path):
                ind = 2*len(path)
                self.indent(w,ind)
-               w.write("%s {\n"%(self.name))
+               w.write("%s {\n"%(self.kname()))
                self.output_props(w,ind+2)
                if self.depth==1: w.write("\n");
                for c in self.children.values():
@@ -440,13 +450,14 @@ class vpnlevel(level):
                "Output flattened list of site names for this VPN"
                ind=2*(len(path)+1)
                self.indent(w,ind)
-               w.write("%s {\n"%(self.name))
+               w.write("%s {\n"%(self.kname()))
                for i in self.children.keys():
                        self.children[i].output_vpnflat(w,path+(self,))
                w.write("\n")
                self.indent(w,ind+2)
                w.write("all-sites %s;\n"%
-                       ','.join(self.children.keys()))
+                       ','.join(map(lambda i: i.kname(),
+                                    self.children.values())))
                self.indent(w,ind)
                w.write("};\n")
 
@@ -469,10 +480,10 @@ class locationlevel(level):
                # Python didn't support nested_scopes until version 2.1
                #
                #"/"+self.name+"/"+i
-               w.write("%s %s;\n"%(self.name,','.join(
+               w.write("%s %s;\n"%(self.kname(),','.join(
                        map(lambda x,path=path,self=self:
                            '/'.join([prefix+"vpn-data"] + list(map(
-                                   lambda i: i.name,
+                                   lambda i: i.kname(),
                                    path+(self,x)))),
                            self.children.values()))))
 
@@ -503,7 +514,7 @@ class sitelevel(level):
                ind=2*len(path)
                np='/'.join(map(lambda i: i.name, path))
                self.indent(w,ind)
-               w.write("%s {\n"%(self.name))
+               w.write("%s {\n"%(self.kname()))
                self.indent(w,ind+2)
                w.write("name \"%s\";\n"%(np,))
                self.output_props(w,ind+2)
@@ -669,8 +680,8 @@ def outputsites(w):
 
        # Flattened list of sites
        w.write(prefix+"all-sites %s;\n"%",".join(
-               map(lambda x:"%svpn/%s/all-sites"%(prefix,x),
-                       root.children.keys())))
+               map(lambda x:"%svpn/%s/all-sites"%(prefix,x.kname()),
+                       root.children.values())))
 
 line=0
 file=None

diff --git a/mtest/t-basic b/mtest/t-basic
index be705039c2cfb72c7aa5b7471d9db0b15ec4015b..e3ced5a9b76a507621f7a75173afd089d758d8d7 100755 (executable)
--- a/mtest/t-basic
+++ b/mtest/t-basic
@@ -2,7 +2,7 @@
 
 source mtest/common.tcl
 
-run-mss test-example/sites $tmp/out.conf
+run-mss --no-conf-key-prefix test-example/sites $tmp/out.conf
 
 set seddery { sed -n 's/^[ \t]*//; /^[^#]/p' }
 exec bash -c "