Merge pull request #469 from jupyter/secureWriteCon

Secure write for connection file

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

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)

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,77 @@
 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, 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.
+
+    Parameters
+    ----------
+
+    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 the file not existing
+        pass
+
+    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)
+
+    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
+
+
 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 +206,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 +268,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 +283,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 +364,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-<pid>.json]
@@ -480,7 +555,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 +571,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

jupyter_client/tests/test_connect.py

@@ -5,6 +5,10 @@
 
 import json
 import os
+import re
+import stat
+import tempfile
+import shutil
 
 from traitlets.config import Config
 from jupyter_core.application import JupyterApp
@@ -14,6 +18,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 +147,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 +165,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 +204,56 @@ 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():
+    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')
+        check_user_only_permissions(fname)
+        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')
+        check_user_only_permissions(fname)
+        with open(fname, 'r') as f:
+            assert f.read() == 'test 2'
+    finally:
+        shutil.rmtree(directory)

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   = {