From: Ian Jackson Date: Mon, 4 Nov 2019 15:09:44 +0000 (+0000) Subject: make-secnet-sites: prefix names when writing sites file X-Git-Tag: v0.5.1~46 X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=secnet.git;a=commitdiff_plain;h=8f7d3ca86bd38eb5d79b0601669b96bab3a0c90b 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 --- diff --git a/make-secnet-sites b/make-secnet-sites index cd8f5db..4be0160 100755 --- 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 be70503..e3ced5a 100755 --- 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 "