From b522571fdb3b20989dbd97c8a79e28ecb1689b66 Mon Sep 17 00:00:00 2001 From: Matthew Seal Date: Sun, 18 Aug 2019 19:14:23 -0700 Subject: [PATCH 1/5] Secure write for connection file --- docs/changelog.rst | 11 ++++++++--- jupyter_client/_version.py | 2 +- jupyter_client/connect.py | 38 +++++++++++++++++++++++++++++--------- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index c677c94c7..81c601e23 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,11 @@ Changes in Jupyter Client ========================= +5.3.2 +===== + +- Important files creation now checks umask permissions (:ghpull:`469`). + 5.3.1 ===== @@ -16,7 +21,7 @@ Changes in Jupyter Client New Features: - Multiprocessing and Threading support (:ghpull:`437`) and (:ghpull:`450`) -- Setup package long_description (:ghpull:`411`) +- Setup package long_description (:ghpull:`411`) Changes: @@ -120,9 +125,9 @@ Breaking changes: - Define Jupyter protocol version 5.2, resolving ambiguity of ``cursor_pos`` field in the presence of unicode surrogate pairs. - + .. seealso:: - + :ref:`cursor_pos_unicode_note` - Add :meth:`Session.clone` for making a copy of a Session object diff --git a/jupyter_client/_version.py b/jupyter_client/_version.py index 9ba6113b0..7ee5f70d0 100644 --- a/jupyter_client/_version.py +++ b/jupyter_client/_version.py @@ -1,4 +1,4 @@ -version_info = (5, 3, 1) +version_info = (5, 3, 2) __version__ = '.'.join(map(str, version_info)) protocol_version_info = (5, 3) diff --git a/jupyter_client/connect.py b/jupyter_client/connect.py index 91efbc461..d244254c7 100644 --- a/jupyter_client/connect.py +++ b/jupyter_client/connect.py @@ -19,6 +19,7 @@ import tempfile import warnings from getpass import getpass +from contextlib import contextmanager import zmq @@ -34,6 +35,22 @@ from jupyter_core.paths import jupyter_data_dir, jupyter_runtime_dir +@contextmanager +def secure_write(fname): + """Opens a file in the most restricted pattern available for + writing content. This limits the file mode to `600` and yields + the resulting opened filed handle. + + Parameters + ---------- + + fname : unicode + The path to the file to write + """ + with os.fdopen(os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600), 'w') as f: + yield f + + def write_connection_file(fname=None, shell_port=0, iopub_port=0, stdin_port=0, hb_port=0, control_port=0, ip='', key=b'', transport='tcp', signature_scheme='hmac-sha256', kernel_name='' @@ -134,7 +151,10 @@ def write_connection_file(fname=None, shell_port=0, iopub_port=0, stdin_port=0, cfg['signature_scheme'] = signature_scheme cfg['kernel_name'] = kernel_name - with open(fname, 'w') as f: + # Only ever write this file as user read/writeable + # This would otherwise introduce a vulnerability as a file has secrets + # which would let others execute arbitrarily code as you + with secure_write(fname) as f: f.write(json.dumps(cfg, indent=2)) if hasattr(stat, 'S_ISVTX'): @@ -193,7 +213,7 @@ def find_connection_file(filename='kernel-*.json', path=None, profile=None): path = ['.', jupyter_runtime_dir()] if isinstance(path, string_types): path = [path] - + try: # first, try explicit name return filefind(filename, path) @@ -208,11 +228,11 @@ def find_connection_file(filename='kernel-*.json', path=None, profile=None): else: # accept any substring match pat = '*%s*' % filename - + matches = [] for p in path: matches.extend(glob.glob(os.path.join(p, pat))) - + matches = [ os.path.abspath(m) for m in matches ] if not matches: raise IOError("Could not find %r in %r" % (filename, path)) @@ -289,11 +309,11 @@ def tunnel_to_kernel(connection_info, sshserver, sshkey=None): class ConnectionFileMixin(LoggingConfigurable): """Mixin for configurable classes that work with connection files""" - + data_dir = Unicode() def _data_dir_default(self): return jupyter_data_dir() - + # The addresses for the communication channels connection_file = Unicode('', config=True, help="""JSON file in which to store connection info [default: kernel-.json] @@ -480,7 +500,7 @@ def write_connection_file(self): def load_connection_file(self, connection_file=None): """Load connection info from JSON dict in self.connection_file. - + Parameters ---------- connection_file: unicode, optional @@ -496,10 +516,10 @@ def load_connection_file(self, connection_file=None): def load_connection_info(self, info): """Load connection info from a dict containing connection info. - + Typically this data comes from a connection file and is called by load_connection_file. - + Parameters ---------- info: dict From d6e605186f5b4e79cf1ef2b5cff18420efb91a28 Mon Sep 17 00:00:00 2001 From: Matthew Seal Date: Mon, 19 Aug 2019 15:42:32 -0700 Subject: [PATCH 2/5] Added test and chmod catch for secure_write --- jupyter_client/connect.py | 12 +++++++++-- jupyter_client/tests/test_connect.py | 31 ++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/jupyter_client/connect.py b/jupyter_client/connect.py index d244254c7..294897fa5 100644 --- a/jupyter_client/connect.py +++ b/jupyter_client/connect.py @@ -47,8 +47,16 @@ def secure_write(fname): fname : unicode The path to the file to write """ - with os.fdopen(os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600), 'w') as f: - yield f + try: + with os.fdopen(os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600), 'w') as f: + yield f + finally: + try: + # Ensure existing files have their permissions changed + os.chmod(fname, 0o600) + except: + os.remove(fname) + raise def write_connection_file(fname=None, shell_port=0, iopub_port=0, stdin_port=0, hb_port=0, diff --git a/jupyter_client/tests/test_connect.py b/jupyter_client/tests/test_connect.py index efc2e75c9..2f70e5f8c 100644 --- a/jupyter_client/tests/test_connect.py +++ b/jupyter_client/tests/test_connect.py @@ -5,6 +5,9 @@ import json import os +import stat +import tempfile +import shutil from traitlets.config import Config from jupyter_core.application import JupyterApp @@ -14,6 +17,7 @@ from jupyter_client import connect, KernelClient from jupyter_client.consoleapp import JupyterConsoleApp from jupyter_client.session import Session +from jupyter_client.connect import secure_write class DummyConsoleApp(JupyterApp, JupyterConsoleApp): @@ -142,7 +146,7 @@ def test_find_connection_file_local(): abs_cf = os.path.abspath(cf) with open(cf, 'w') as f: f.write('{}') - + for query in ( 'test.json', 'test', @@ -160,7 +164,7 @@ def test_find_connection_file_relative(): abs_cf = os.path.abspath(cf) with open(cf, 'w') as f: f.write('{}') - + for query in ( os.path.join('.', 'subdir', 'test.json'), os.path.join('subdir', 'test.json'), @@ -199,3 +203,26 @@ def test_mixin_cleanup_random_ports(): assert not os.path.exists(filename) for name in dc._random_port_names: assert getattr(dc, name) == 0 + + +def test_secure_write(): + directory = tempfile.mkdtemp() + fname = os.path.join(directory, 'check_perms') + try: + with secure_write(fname) as f: + f.write('test 1') + mode = os.stat(fname).st_mode + assert '0600' == oct(stat.S_IMODE(mode)).replace('0o', '0') + with open(fname, 'r') as f: + assert f.read() == 'test 1' + + # Try changing file permissions ahead of time + os.chmod(fname, 0o755) + with secure_write(fname) as f: + f.write('test 2') + mode = os.stat(fname).st_mode + assert '0600' == oct(stat.S_IMODE(mode)).replace('0o', '0') + with open(fname, 'r') as f: + assert f.read() == 'test 2' + finally: + shutil.rmtree(directory) From 5631d6af41664d08af95f8c95ae247d8984932eb Mon Sep 17 00:00:00 2001 From: Matthew Seal Date: Sun, 25 Aug 2019 22:37:11 -0700 Subject: [PATCH 3/5] Added support for secure windows file writes --- jupyter_client/connect.py | 63 ++++++++++++++++++++++++---- jupyter_client/tests/test_connect.py | 39 +++++++++++++++-- setup.py | 1 + 3 files changed, 90 insertions(+), 13 deletions(-) diff --git a/jupyter_client/connect.py b/jupyter_client/connect.py index 294897fa5..3dfd6c211 100644 --- a/jupyter_client/connect.py +++ b/jupyter_client/connect.py @@ -35,6 +35,41 @@ from jupyter_core.paths import jupyter_data_dir, jupyter_runtime_dir +# TODO: Move to jupyter_core +def win32_restrict_file_to_user(fname): + """Secure a windows file to read-only access for the user. + Follows guidance from win32 library creator: + http://timgolden.me.uk/python/win32_how_do_i/add-security-to-a-file.html + + This method should be executed against an already generated file which + has no secrets written to it yet. + + Parameters + ---------- + + fname : unicode + The path to the file to secure + """ + import win32api + import win32security + import ntsecuritycon as con + + # everyone, _domain, _type = win32security.LookupAccountName("", "Everyone") + admins, _domain, _type = win32security.LookupAccountName("", "Administrators") + user, _domain, _type = win32security.LookupAccountName("", win32api.GetUserName()) + + sd = win32security.GetFileSecurity(fname, win32security.DACL_SECURITY_INFORMATION) + + dacl = win32security.ACL() + # dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con.FILE_ALL_ACCESS, everyone) + dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con.FILE_GENERIC_READ | con.FILE_GENERIC_WRITE, user) + dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con.FILE_ALL_ACCESS, admins) + + sd.SetSecurityDescriptorDacl(1, dacl, 0) + win32security.SetFileSecurity(fname, win32security.DACL_SECURITY_INFORMATION, sd) + + +# TODO: Move to jupyter_core @contextmanager def secure_write(fname): """Opens a file in the most restricted pattern available for @@ -48,15 +83,25 @@ def secure_write(fname): The path to the file to write """ try: - with os.fdopen(os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600), 'w') as f: - yield f - finally: - try: - # Ensure existing files have their permissions changed - os.chmod(fname, 0o600) - except: - os.remove(fname) - raise + os.remove(fname) + except IOError: + # Skip any issues with file not existing + pass + + # Touch file + fd = os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600) + os.close(fd) + + if os.name == 'nt': + # Python on windows does not respect the group and public bits for chmod, so we need + # to take additional steps to secure the contents. + win32_restrict_file_to_user(fname) + else: + # Enforce that the file got the requested permissions. + assert '0600' == oct(stat.S_IMODE(os.stat(fname).st_mode)).replace('0o', '0') + + with os.fdopen(os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600), 'w') as f: + yield f def write_connection_file(fname=None, shell_port=0, iopub_port=0, stdin_port=0, hb_port=0, diff --git a/jupyter_client/tests/test_connect.py b/jupyter_client/tests/test_connect.py index 2f70e5f8c..b64511690 100644 --- a/jupyter_client/tests/test_connect.py +++ b/jupyter_client/tests/test_connect.py @@ -5,6 +5,7 @@ import json import os +import re import stat import tempfile import shutil @@ -206,13 +207,44 @@ def test_mixin_cleanup_random_ports(): def test_secure_write(): + def fetch_win32_permissions(filename): + '''Extracts file permissions on windows using icacls''' + role_permissions = {} + for index, line in enumerate(os.popen("icacls %s" % filename).read().splitlines()): + if index == 0: + line = line.split(filename)[-1].strip().lower() + match = re.match(r'\s*([^:]+):\(([^\)]*)\)', line) + if match: + usergroup, permissions = match.groups() + usergroup = usergroup.lower().split('\\')[-1] + permissions = set(p.lower() for p in permissions.split(',')) + role_permissions[usergroup] = permissions + elif not line.strip(): + break + return role_permissions + + def check_user_only_permissions(fname): + # Windows has it's own permissions ACL patterns + if os.name == 'nt': + import win32api + username = win32api.GetUserName().lower() + permissions = fetch_win32_permissions(fname) + assert username in permissions + assert permissions[username] == set(['r', 'w']) + assert 'administrators' in permissions + assert permissions['administrators'] == set(['f']) + assert 'everyone' not in permissions + assert len(permissions) == 2 + else: + mode = os.stat(fname).st_mode + assert '0600' == oct(stat.S_IMODE(mode)).replace('0o', '0') + directory = tempfile.mkdtemp() fname = os.path.join(directory, 'check_perms') try: with secure_write(fname) as f: f.write('test 1') - mode = os.stat(fname).st_mode - assert '0600' == oct(stat.S_IMODE(mode)).replace('0o', '0') + check_user_only_permissions(fname) with open(fname, 'r') as f: assert f.read() == 'test 1' @@ -220,8 +252,7 @@ def test_secure_write(): os.chmod(fname, 0o755) with secure_write(fname) as f: f.write('test 2') - mode = os.stat(fname).st_mode - assert '0600' == oct(stat.S_IMODE(mode)).replace('0o', '0') + check_user_only_permissions(fname) with open(fname, 'r') as f: assert f.read() == 'test 2' finally: diff --git a/setup.py b/setup.py index 226c9fd1a..14e5e02d9 100644 --- a/setup.py +++ b/setup.py @@ -92,6 +92,7 @@ def run(self): 'pyzmq>=13', 'python-dateutil>=2.1', 'tornado>=4.1', + "pywin32 >=1.0 ; sys_platform == 'win32'" ], python_requires='>=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*, !=3.4.*', extras_require = { From b221664345eac3ec2571ac179add27488860657b Mon Sep 17 00:00:00 2001 From: Matthew Seal Date: Sun, 25 Aug 2019 22:56:06 -0700 Subject: [PATCH 4/5] Attempt to fix for python 2 --- jupyter_client/connect.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyter_client/connect.py b/jupyter_client/connect.py index 3dfd6c211..632c21a42 100644 --- a/jupyter_client/connect.py +++ b/jupyter_client/connect.py @@ -84,7 +84,7 @@ def secure_write(fname): """ try: os.remove(fname) - except IOError: + except (IOError, OSError): # Skip any issues with file not existing pass From f780fee82e3d9e286dd309b69002775c7f59f988 Mon Sep 17 00:00:00 2001 From: Matthew Seal Date: Tue, 27 Aug 2019 23:14:31 -0700 Subject: [PATCH 5/5] Cloned PR feedback from jupyter_server --- jupyter_client/connect.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/jupyter_client/connect.py b/jupyter_client/connect.py index 632c21a42..398e8b900 100644 --- a/jupyter_client/connect.py +++ b/jupyter_client/connect.py @@ -71,7 +71,7 @@ def win32_restrict_file_to_user(fname): # TODO: Move to jupyter_core @contextmanager -def secure_write(fname): +def secure_write(fname, binary=False): """Opens a file in the most restricted pattern available for writing content. This limits the file mode to `600` and yields the resulting opened filed handle. @@ -82,25 +82,27 @@ def secure_write(fname): fname : unicode The path to the file to write """ + mode = 'wb' if binary else 'w' + open_flag = os.O_CREAT | os.O_WRONLY | os.O_TRUNC try: os.remove(fname) except (IOError, OSError): - # Skip any issues with file not existing + # Skip any issues with the file not existing pass - - # Touch file - fd = os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600) - os.close(fd) if os.name == 'nt': # Python on windows does not respect the group and public bits for chmod, so we need # to take additional steps to secure the contents. + # Touch file pre-emptively to avoid editing permissions in open files in Windows + fd = os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600) + os.close(fd) + open_flag = os.O_WRONLY | os.O_TRUNC win32_restrict_file_to_user(fname) - else: - # Enforce that the file got the requested permissions. - assert '0600' == oct(stat.S_IMODE(os.stat(fname).st_mode)).replace('0o', '0') - with os.fdopen(os.open(fname, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600), 'w') as f: + with os.fdopen(os.open(fname, open_flag, 0o600), mode) as f: + if os.name != 'nt': + # Enforce that the file got the requested permissions before writing + assert '0600' == oct(stat.S_IMODE(os.stat(fname).st_mode)).replace('0o', '0') yield f