infinitepush: fix security concern

Summary: Check owner of a log file before writing to it. See comments for details

Test Plan: arc unit

Reviewers: #mercurial, simpkins

Reviewed By: simpkins

Subscribers: net-systems-diffs@fb.com, mjpieters, #sourcecontrol

Differential Revision: https://phabricator.intern.facebook.com/D4842933

Tasks: 17155924

Signature: t1:4842933:1491861137:e8a027fb7a930c0c5f553c75cb84214d24f66ce3

infinitepush/backupcommands.py

@@ -31,6 +31,8 @@ import os
 import random
 import re
 import socket
+import stat
+import subprocess
 import time
 
 from .bundleparts import (
@@ -53,7 +55,6 @@ from mercurial import (
 )
 
 from collections import defaultdict, namedtuple
-from hgext3rd.extutil import runshellcommand
 from mercurial.extensions import wrapfunction, unwrapfunction
 from mercurial.node import bin, hex, nullrev
 from mercurial.i18n import _
@@ -72,6 +73,10 @@ class backupstate(object):
     def empty(self):
         return not self.heads and not self.localbookmarks
 
+class WrongPermissionsException(Exception):
+    def __init__(self, logdir):
+        self.logdir = logdir
+
 restoreoptions = [
      ('', 'reporoot', '', 'root of the repo to restore'),
      ('', 'user', '', 'user who ran the backup'),
@@ -97,15 +102,26 @@ def backup(ui, repo, dest=None, **opts):
         background_cmd = ['hg', 'pushbackup']
         if dest:
             background_cmd.append(dest)
+        logfile = None
         logdir = ui.config('infinitepushbackup', 'logdir')
         if logdir:
+            # make newly created files and dirs non-writable
+            oldumask = os.umask(0o022)
             try:
                 try:
                     username = util.shortuser(ui.username())
                 except Exception:
                     username = 'unknown'
+
+                if not _checkcommonlogdir(logdir):
+                    raise WrongPermissionsException(logdir)
+
                 userlogdir = os.path.join(logdir, username)
                 util.makedirs(userlogdir)
+
+                if not _checkuserlogdir(userlogdir):
+                    raise WrongPermissionsException(userlogdir)
+
                 reporoot = repo.origroot
                 reponame = os.path.basename(reporoot)
 
@@ -113,10 +129,21 @@ def backup(ui, repo, dest=None, **opts):
                                                 'maxlognumber', 5)
                 _removeoldlogfiles(userlogdir, reponame, maxlogfilenumber)
                 logfile = _getlogfilename(logdir, username, reponame)
-                background_cmd.extend(('>>', logfile, '2>&1'))
             except (OSError, IOError) as e:
                 ui.warn(_('infinitepush backup log is disabled: %s\n') % e)
-        runshellcommand(' '.join(background_cmd), os.environ)
+            except WrongPermissionsException as e:
+                ui.warn(_('%s directory has incorrect permission, ' +
+                          'infinitepush backup logging will be disabled\n') %
+                        e.logdir)
+            finally:
+                os.umask(oldumask)
+
+        if not logfile:
+            logfile = os.devnull
+
+        with open(logfile, 'a') as f:
+            subprocess.Popen(background_cmd, shell=False, stdout=f,
+                             stderr=subprocess.STDOUT)
         return 0
 
     try:
@@ -660,6 +687,38 @@ def _removeoldlogfiles(userlogdir, reponame, maxlogfilenumber):
         for filename in existinglogfiles[maxlogfilenumber:]:
             os.unlink(os.path.join(userlogdir, filename))
 
+def _checkcommonlogdir(logdir):
+    '''Checks permissions of the log directory
+
+    We want log directory to actually be a directory, have restricting
+    deletion flag set (sticky bit)
+    '''
+
+    try:
+        st = os.stat(logdir)
+        return stat.S_ISDIR(st.st_mode) and st.st_mode & stat.S_ISVTX
+    except OSError:
+        # is raised by os.stat()
+        return False
+
+def _checkuserlogdir(userlogdir):
+    '''Checks permissions of the user log directory
+
+    We want user log directory to be writable only by the user who created it
+    and be owned by `username`
+    '''
+
+    try:
+        st = os.stat(userlogdir)
+        # Check that `userlogdir` is owned by `username`
+        if os.getuid() != st.st_uid:
+            return False
+        return ((st.st_mode & (stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH)) ==
+                stat.S_IWUSR)
+    except OSError:
+        # is raised by os.stat()
+        return False
+
 def _dictdiff(first, second):
     '''Returns new dict that contains items from the first dict that are missing
     from the second dict.

tests/test-infinitepush-backup-logging.t

@@ -19,19 +19,8 @@ Create log dir
 Setup infinitepush backup logging
   $ printf "\n[infinitepushbackup]\nlogdir=$TESTTMP/logs" >> .hg/hgrc
   $ mkcommit first
-  $ hg pushbackup --background
-  $ waitbgbackup
-  $ ls $TESTTMP/logs/test
-  client\d{8} (re)
 
-Set maxlognumber to 1, create a few fake log files and run pushbackup. Make sure
-outdated files are deleted
-  $ printf "\n[infinitepushbackup]\nmaxlognumber=1" >> .hg/hgrc
-  $ touch $TESTTMP/logs/test/client19700101
-  $ ls $TESTTMP/logs/test
-  client\d{8} (re)
-  client\d{8} (re)
+Check that logging fails because of wrong permissions
   $ hg pushbackup --background
+  $TESTTMP/logs directory has incorrect permission, infinitepush backup logging will be disabled
   $ waitbgbackup
-  $ ls $TESTTMP/logs/test
-  client\d{8} (re)