[PATCH] python: avoid predictable/racy temp directories (and samba-tool gpo tidy-up)
Douglas Bagnall
douglas.bagnall at catalyst.net.nz
Wed Nov 7 00:00:26 UTC 2018
In a couple of places we have been using temporary files with imperfect
security.
In upgradeprovision code we have been using tempfile.mktemp which leaves
a gap between the time it chooses the name and the time you use it.
In samba-tool gpo, we have used "/tmp" by default, which, though
convenient, is sort of uncool.
These patches fix those problems. In fixing the samba-tool case, I
couldn't help noticing I was dealing with the same code in three
different places, which led to an additional deduplication patch, thence
a now-we-have-a-place-for-this-weird-mixin patch.
To get the old behaviour in samba-tool, you can use
samba-tool gpo --tmpdir=/tmp
otherwise you get a securely created directory of your own with a name
like /tmp/tmpWh31Le. The directory name is announced to the user and it
is not cleaned up, as I get the impression that people sometimes want to
examine or reuse the contents. If that is wrong we should probably also
delete the directory in the case that --tmpdir is not used.
cheers,
Douglas
-------------- next part --------------
From b0406d063d00cfe930475412c136ac8a4557f842 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Fri, 2 Nov 2018 09:02:15 +1300
Subject: [PATCH 1/4] python/upgradehelpers: use mkstemp, not mktemp
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
python/samba/upgradehelpers.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/python/samba/upgradehelpers.py b/python/samba/upgradehelpers.py
index 672268af4df..40120bd7190 100644
--- a/python/samba/upgradehelpers.py
+++ b/python/samba/upgradehelpers.py
@@ -814,14 +814,15 @@ def print_provision_ranges(dic, limit_print, dest, samdb_path, invocationid):
obj["max"], id)
if ldif != "":
- file = tempfile.mktemp(dir=dest, prefix="usnprov", suffix=".ldif")
+ fd, file = tempfile.mkstemp(dir=dest, prefix="usnprov", suffix=".ldif")
print()
print("To track the USNs modified/created by provision and upgrade proivsion,")
print(" the following ranges are proposed to be added to your provision sam.ldb: \n%s" % ldif)
print("We recommend to review them, and if it's correct to integrate the following ldif: %s in your sam.ldb" % file)
print("You can load this file like this: ldbadd -H %s %s\n" %(str(samdb_path), file))
ldif = "dn: @PROVISION\nprovisionnerID: %s\n%s" % (invocationid, ldif)
- open(file, 'w').write(ldif)
+ os.write(fd, ldif)
+ os.close(fd)
def int64range2str(value):
--
2.17.1
From 55f4e91847d5485809a63297114e591f0a946310 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed, 7 Nov 2018 11:43:26 +1300
Subject: [PATCH 2/4] samba-tool gpo: do not use predictable temp directory
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
python/samba/netcmd/gpo.py | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py
index 4d5fc883876..0e6e2626c25 100644
--- a/python/samba/netcmd/gpo.py
+++ b/python/samba/netcmd/gpo.py
@@ -18,7 +18,7 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
-
+from __future__ import print_function
import os
import samba.getopt as options
import ldb
@@ -939,7 +939,9 @@ class cmd_fetch(Command):
# Copy GPT
if tmpdir is None:
- tmpdir = "/tmp"
+ tmpdir = tempfile.mkdtemp()
+ print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
+ file=self.outf)
if not os.path.isdir(tmpdir):
raise CommandError("Temoprary directory '%s' does not exist" % tmpdir)
@@ -1017,7 +1019,9 @@ class cmd_backup(Command):
# Copy GPT
if tmpdir is None:
- tmpdir = "/tmp"
+ tmpdir = tempfile.mkdtemp()
+ print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
+ file=self.outf)
if not os.path.isdir(tmpdir):
raise CommandError("Temoprary directory '%s' does not exist" % tmpdir)
@@ -1168,7 +1172,9 @@ class cmd_create(Command):
# Create GPT
if tmpdir is None:
- tmpdir = "/tmp"
+ tmpdir = tempfile.mkdtemp()
+ print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
+ file=self.outf)
if not os.path.isdir(tmpdir):
raise CommandError("Temporary directory '%s' does not exist" % tmpdir)
self.tmpdir = tmpdir
--
2.17.1
From ca37191f45fd58f33a800a5ed8ed9e7ccd0d7cf6 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed, 7 Nov 2018 11:57:13 +1300
Subject: [PATCH 3/4] samba-tool gpo: add helper method for tmpdir construction
A few of the gpo commands use an identical temporary directory structure
that can be constructed using shared code.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
python/samba/netcmd/gpo.py | 115 +++++++++++++++++--------------------
1 file changed, 54 insertions(+), 61 deletions(-)
diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py
index 0e6e2626c25..484a33952ca 100644
--- a/python/samba/netcmd/gpo.py
+++ b/python/samba/netcmd/gpo.py
@@ -371,7 +371,44 @@ def create_directory_hier(conn, remotedir):
conn.mkdir(path)
-class cmd_listall(Command):
+class GPOCommand(Command):
+ def construct_tmpdir(self, tmpdir, gpo):
+ """Ensure that the temporary directory structure used in fetch,
+ backup, create, and restore is consistent.
+
+ If --tmpdir is used the named directory must be present, which may
+ contain a 'policy' subdirectory, but 'policy' must not itself have
+ a subdirectory with the gpo name. The policy and gpo directories
+ will be created.
+
+ If --tmpdir is not used, a temporary directory is securely created.
+ """
+ if tmpdir is None:
+ tmpdir = tempfile.mkdtemp()
+ print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
+ file=self.outf)
+
+ if not os.path.isdir(tmpdir):
+ raise CommandError("Temporary directory '%s' does not exist" % tmpdir)
+
+ localdir = os.path.join(tmpdir, "policy")
+ if not os.path.isdir(localdir):
+ os.mkdir(localdir)
+
+ gpodir = os.path.join(localdir, gpo)
+ if os.path.isdir(gpodir):
+ raise CommandError(
+ "GPO directory '%s' already exists, refusing to overwrite" % gpodir)
+
+ try:
+ os.mkdir(gpodir)
+ except (IOError, OSError) as e:
+ raise CommandError("Error creating teporary GPO directory", e)
+
+ return tmpdir, gpodir
+
+
+class cmd_listall(GPOCommand):
"""List all GPOs."""
synopsis = "%prog [options]"
@@ -408,7 +445,7 @@ class cmd_listall(Command):
self.outf.write("\n")
-class cmd_list(Command):
+class cmd_list(GPOCommand):
"""List GPOs for an account."""
synopsis = "%prog <username> [options]"
@@ -525,7 +562,7 @@ class cmd_list(Command):
self.outf.write(" %s %s\n" % (g[0], g[1]))
-class cmd_show(Command):
+class cmd_show(GPOCommand):
"""Show information for a GPO."""
synopsis = "%prog <gpo> [options]"
@@ -573,7 +610,7 @@ class cmd_show(Command):
self.outf.write("\n")
-class cmd_getlink(Command):
+class cmd_getlink(GPOCommand):
"""List GPO Links for a container."""
synopsis = "%prog <container_dn> [options]"
@@ -620,7 +657,7 @@ class cmd_getlink(Command):
self.outf.write("No GPO(s) linked to DN=%s\n" % container_dn)
-class cmd_setlink(Command):
+class cmd_setlink(GPOCommand):
"""Add or update a GPO link to a container."""
synopsis = "%prog <container_dn> <gpo> [options]"
@@ -710,7 +747,7 @@ class cmd_setlink(Command):
cmd_getlink().run(container_dn, H, sambaopts, credopts, versionopts)
-class cmd_dellink(Command):
+class cmd_dellink(GPOCommand):
"""Delete GPO link from a container."""
synopsis = "%prog <container_dn> <gpo> [options]"
@@ -749,7 +786,7 @@ class cmd_dellink(Command):
cmd_getlink().run(container_dn, H, sambaopts, credopts, versionopts)
-class cmd_listcontainers(Command):
+class cmd_listcontainers(GPOCommand):
"""List all linked containers for a GPO."""
synopsis = "%prog <gpo> [options]"
@@ -785,7 +822,7 @@ class cmd_listcontainers(Command):
self.outf.write("No Containers using GPO %s\n" % gpo)
-class cmd_getinheritance(Command):
+class cmd_getinheritance(GPOCommand):
"""Get inheritance flag for a container."""
synopsis = "%prog <container_dn> [options]"
@@ -829,7 +866,7 @@ class cmd_getinheritance(Command):
self.outf.write("Container has GPO_INHERIT\n")
-class cmd_setinheritance(Command):
+class cmd_setinheritance(GPOCommand):
"""Set inheritance flag on a container."""
synopsis = "%prog <container_dn> <block|inherit> [options]"
@@ -883,7 +920,7 @@ class cmd_setinheritance(Command):
raise CommandError("Error setting inheritance state %s" % inherit_state, e)
-class cmd_fetch(Command):
+class cmd_fetch(GPOCommand):
"""Download a GPO."""
synopsis = "%prog <gpo> [options]"
@@ -938,23 +975,8 @@ class cmd_fetch(Command):
raise CommandError("Error connecting to '%s' using SMB" % dc_hostname)
# Copy GPT
- if tmpdir is None:
- tmpdir = tempfile.mkdtemp()
- print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
- file=self.outf)
- if not os.path.isdir(tmpdir):
- raise CommandError("Temoprary directory '%s' does not exist" % tmpdir)
-
- localdir = os.path.join(tmpdir, "policy")
- if not os.path.isdir(localdir):
- os.mkdir(localdir)
-
- gpodir = os.path.join(localdir, gpo)
- if os.path.isdir(gpodir):
- raise CommandError("GPO directory '%s' already exists, refusing to overwrite" % gpodir)
-
+ tmpdir, gpodir = self.construct_tmpdir(tmpdir, gpo)
try:
- os.mkdir(gpodir)
copy_directory_remote_to_local(conn, sharepath, gpodir)
except Exception as e:
# FIXME: Catch more specific exception
@@ -962,7 +984,7 @@ class cmd_fetch(Command):
self.outf.write('GPO copied to %s\n' % gpodir)
-class cmd_backup(Command):
+class cmd_backup(GPOCommand):
"""Backup a GPO."""
synopsis = "%prog <gpo> [options]"
@@ -1018,23 +1040,9 @@ class cmd_backup(Command):
raise CommandError("Error connecting to '%s' using SMB" % dc_hostname)
# Copy GPT
- if tmpdir is None:
- tmpdir = tempfile.mkdtemp()
- print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
- file=self.outf)
- if not os.path.isdir(tmpdir):
- raise CommandError("Temoprary directory '%s' does not exist" % tmpdir)
-
- localdir = os.path.join(tmpdir, "policy")
- if not os.path.isdir(localdir):
- os.mkdir(localdir)
-
- gpodir = os.path.join(localdir, gpo)
- if os.path.isdir(gpodir):
- raise CommandError("GPO directory '%s' already exists, refusing to overwrite" % gpodir)
+ tmpdir, gpodir = self.construct_tmpdir(tmpdir, gpo)
try:
- os.mkdir(gpodir)
backup_directory_remote_to_local(conn, sharepath, gpodir)
except Exception as e:
# FIXME: Catch more specific exception
@@ -1113,7 +1121,7 @@ class cmd_backup(Command):
return entities
-class cmd_create(Command):
+class cmd_create(GPOCommand):
"""Create an empty GPO."""
synopsis = "%prog <displayname> [options]"
@@ -1171,25 +1179,10 @@ class cmd_create(Command):
unc_path = "\\\\%s\\sysvol\\%s\\Policies\\%s" % (realm, realm, gpo)
# Create GPT
- if tmpdir is None:
- tmpdir = tempfile.mkdtemp()
- print("Using temporary directory %s (use --tmpdir to change)" % tmpdir,
- file=self.outf)
- if not os.path.isdir(tmpdir):
- raise CommandError("Temporary directory '%s' does not exist" % tmpdir)
- self.tmpdir = tmpdir
-
- localdir = os.path.join(tmpdir, "policy")
- if not os.path.isdir(localdir):
- os.mkdir(localdir)
-
- gpodir = os.path.join(localdir, gpo)
+ self.tmpdir, gpodir = self.construct_tmpdir(tmpdir, gpo)
self.gpodir = gpodir
- if os.path.isdir(gpodir):
- raise CommandError("GPO directory '%s' already exists, refusing to overwrite" % gpodir)
try:
- os.mkdir(gpodir)
os.mkdir(os.path.join(gpodir, "Machine"))
os.mkdir(os.path.join(gpodir, "User"))
gpt_contents = "[General]\r\nVersion=0\r\n"
@@ -1415,7 +1408,7 @@ class cmd_restore(cmd_create):
raise CommandError("Failed to restore: %s" % e)
-class cmd_del(Command):
+class cmd_del(GPOCommand):
"""Delete a GPO."""
synopsis = "%prog <gpo> [options]"
@@ -1491,7 +1484,7 @@ class cmd_del(Command):
self.outf.write("GPO %s deleted.\n" % gpo)
-class cmd_aclcheck(Command):
+class cmd_aclcheck(GPOCommand):
"""Check all GPOs have matching LDAP and DS ACLs."""
synopsis = "%prog [options]"
--
2.17.1
From 23a6a6c44a8af0d9e2af4c5a091bab17972cf431 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed, 7 Nov 2018 12:15:12 +1300
Subject: [PATCH 4/4] samba-tool gpo: convert pseudo-method into method
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
python/samba/netcmd/gpo.py | 47 +++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py
index 484a33952ca..f1f1e985e61 100644
--- a/python/samba/netcmd/gpo.py
+++ b/python/samba/netcmd/gpo.py
@@ -61,16 +61,6 @@ from samba.gp_parse.gp_inf import GptTmplInfParser
from samba.gp_parse.gp_aas import GPAasParser
-def samdb_connect(ctx):
- '''make a ldap connection to the server'''
- try:
- ctx.samdb = SamDB(url=ctx.url,
- session_info=system_session(),
- credentials=ctx.creds, lp=ctx.lp)
- except Exception as e:
- raise CommandError("LDAP connection to %s failed " % ctx.url, e)
-
-
def attr_default(msg, attrname, default):
'''get an attribute from a ldap msg with a default'''
if attrname in msg:
@@ -407,6 +397,15 @@ class GPOCommand(Command):
return tmpdir, gpodir
+ def samdb_connect(self):
+ '''make a ldap connection to the server'''
+ try:
+ self.samdb = SamDB(url=self.url,
+ session_info=system_session(),
+ credentials=self.creds, lp=self.lp)
+ except Exception as e:
+ raise CommandError("LDAP connection to %s failed " % self.url, e)
+
class cmd_listall(GPOCommand):
"""List all GPOs."""
@@ -431,7 +430,7 @@ class cmd_listall(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
msg = get_gpo_info(self.samdb, None)
@@ -469,7 +468,7 @@ class cmd_list(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = self.samdb.search(expression='(&(|(samAccountName=%s)(samAccountName=%s$))(objectClass=User))' %
@@ -586,7 +585,7 @@ class cmd_show(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = get_gpo_info(self.samdb, gpo)[0]
@@ -635,7 +634,7 @@ class cmd_getlink(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = self.samdb.search(base=container_dn, scope=ldb.SCOPE_BASE,
@@ -686,7 +685,7 @@ class cmd_setlink(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
gplink_options = 0
if disabled:
@@ -772,7 +771,7 @@ class cmd_dellink(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
# Check if valid GPO
try:
@@ -811,7 +810,7 @@ class cmd_listcontainers(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
msg = get_gpo_containers(self.samdb, gpo)
if len(msg):
@@ -847,7 +846,7 @@ class cmd_getinheritance(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = self.samdb.search(base=container_dn, scope=ldb.SCOPE_BASE,
@@ -898,7 +897,7 @@ class cmd_setinheritance(GPOCommand):
self.url = dc_url(self.lp, self.creds, H)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = self.samdb.search(base=container_dn, scope=ldb.SCOPE_BASE,
expression="(objectClass=*)",
@@ -951,7 +950,7 @@ class cmd_fetch(GPOCommand):
dc_hostname = netcmd_finddc(self.lp, self.creds)
self.url = dc_url(self.lp, self.creds, dc=dc_hostname)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = get_gpo_info(self.samdb, gpo)[0]
except Exception:
@@ -1020,7 +1019,7 @@ class cmd_backup(GPOCommand):
dc_hostname = netcmd_finddc(self.lp, self.creds)
self.url = dc_url(self.lp, self.creds, dc=dc_hostname)
- samdb_connect(self)
+ self.samdb_connect()
try:
msg = get_gpo_info(self.samdb, gpo)[0]
except Exception:
@@ -1163,7 +1162,7 @@ class cmd_create(GPOCommand):
dc_hostname = cldap_ret.pdc_dns_name
self.url = dc_url(self.lp, self.creds, dc=dc_hostname)
- samdb_connect(self)
+ self.samdb_connect()
msg = get_gpo_info(self.samdb, displayname=displayname)
if msg.count > 0:
@@ -1439,7 +1438,7 @@ class cmd_del(GPOCommand):
dc_hostname = netcmd_finddc(self.lp, self.creds)
self.url = dc_url(self.lp, self.creds, dc=dc_hostname)
- samdb_connect(self)
+ self.samdb_connect()
# Check if valid GPO
try:
@@ -1515,7 +1514,7 @@ class cmd_aclcheck(GPOCommand):
dc_hostname = netcmd_finddc(self.lp, self.creds)
self.url = dc_url(self.lp, self.creds, dc=dc_hostname)
- samdb_connect(self)
+ self.samdb_connect()
msg = get_gpo_info(self.samdb, None)
--
2.17.1
More information about the samba-technical
mailing list