Merge pull request #469 from jupyter/secureWriteCon

Secure write for connection file

Author: rgbkrk

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