Enhance security by adding safety checks for unpickling and ensuring …

index.html

@@ -124,7 +124,7 @@ <h2>Any Input</h2>
           <!-- </script>   -->
           <a class="mobile-show" href="https://asciinema.org/a/19519" target="_blank"><img src="https://asciinema.org/a/19519.png" /></a>
           <div class="mobile-hide">
-          <script id="asciicast-19519" src="https://asciinema.org/a/19519.js" async></script>
+          <script id="asciicast-19519" src="https://asciinema.org/a/19519.js" async integrity="sha384-REPLACE_ME" crossorigin="anonymous"></script>
           </div>
         </div>
       </div>
@@ -146,7 +146,7 @@ <h2>Arbitrary Commands</h2>
           <!-- <script type="text/javascript" src="https://asciinema.org/a/19520.js" id="asciicast-19520" async></script> -->
           <a class="mobile-show" href="https://asciinema.org/a/19520" target="_blank"><img src="https://asciinema.org/a/19520.png" /></a>
           <div class="mobile-hide">
-          <script id="asciicast-19520" src="https://asciinema.org/a/19520.js" async></script>
+          <script id="asciicast-19520" src="https://asciinema.org/a/19520.js" async integrity="sha384-REPLACE_ME" crossorigin="anonymous"></script>
           </div>
         </div>
       </div>
@@ -156,8 +156,8 @@ <h2>Arbitrary Commands</h2>
     Bootstrap core JavaScript
     ==================================================
     -->
-    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script>
-    <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/js/bootstrap.min.js"></script>
+    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js" integrity="sha384-REPLACE_ME" crossorigin="anonymous"></script>
+    <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/js/bootstrap.min.js" integrity="sha384-REPLACE_ME" crossorigin="anonymous"></script>
     <script src="assets/ZeroClipboard.min.js"></script>
     <script>
       $(window).resize(function() {

src/choose.py

@@ -49,6 +49,11 @@ def do_program(
 
 def get_line_objs() -> Dict[int, LineBase]:
     file_path = state_files.get_pickle_file_path()
+    # refuse to load pickles that are not safe (ownership/permissions)
+    if os.path.exists(file_path) and not state_files.is_state_file_safe(file_path):
+        output.append_error(LOAD_SELECTION_WARNING)
+        output.append_exit()
+        sys.exit(1)
     try:
         line_objs: Dict[int, LineBase] = pickle.load(open(file_path, "rb"))
     except (OSError, KeyError, pickle.PickleError):
@@ -74,6 +79,10 @@ def get_line_objs() -> Dict[int, LineBase]:
 def set_selections_from_pickle(
     selection_path: str, line_objs: Dict[int, LineBase]
 ) -> None:
+    if not state_files.is_state_file_safe(selection_path):
+        output.append_error(LOAD_SELECTION_WARNING)
+        output.append_exit()
+        sys.exit(1)
     try:
         selected_indices = pickle.load(open(selection_path, "rb"))
     except (OSError, KeyError, pickle.PickleError):

src/pathpicker/output.py

@@ -4,6 +4,7 @@
 # LICENSE file in the root directory of this source tree.
 import os
 import pickle
+import tempfile
 from typing import List, Tuple
 
 from pathpicker import logger, state_files
@@ -70,9 +71,13 @@ def append_if_invalid(line_objs: List[LineMatch]) -> None:
 def output_selection(line_objs: List[LineMatch]) -> None:
     file_path = state_files.get_selection_file_path()
     indices = [line.index for line in line_objs]
-    file = open(file_path, "wb")
-    pickle.dump(indices, file)
-    file.close()
+    dirpath = os.path.dirname(file_path)
+    with tempfile.NamedTemporaryFile(dir=dirpath, delete=False) as tf:
+        pickle.dump(indices, tf, protocol=pickle.HIGHEST_PROTOCOL)
+        tf.flush()
+        os.fsync(tf.fileno())
+    os.replace(tf.name, file_path)
+    os.chmod(file_path, 0o600)
 
 
 def get_editor_and_path() -> Tuple[str, str]:

src/pathpicker/parse.py

@@ -4,6 +4,7 @@
 # LICENSE file in the root directory of this source tree.
 import os
 import re
+import shutil
 import subprocess
 from functools import partial
 from typing import Callable, List, Match, NamedTuple, NewType, Optional, Pattern, Tuple
@@ -238,35 +239,37 @@ class RegexConfig(NamedTuple):
 # repository in which path resides (i.e. the current directory).
 # both git and hg have commands for this, so let's just use those.
 def get_repo_path() -> str:
-    proc = subprocess.Popen(
-        ["git rev-parse --show-toplevel"],
-        stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE,
-        shell=True,
-        universal_newlines=True,
-    )
-
-    stdout, stderr = proc.communicate()
-
-    # If there was no error return the output
-    if not stderr:
-        logger.add_event("using_git")
-        return stdout
-
-    proc = subprocess.Popen(
-        ["hg root"],
-        stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE,
-        shell=True,
-        universal_newlines=True,
-    )
-
-    stdout, stderr = proc.communicate()
+    git_path = shutil.which("git")
+    if git_path:
+        proc = subprocess.run(
+            [git_path, "rev-parse", "--show-toplevel"],
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            universal_newlines=True,
+            check=False,
+        )
+        stdout, stderr = proc.stdout, proc.stderr
+
+        # If there was no error return the output
+        if not stderr:
+            logger.add_event("using_git")
+            return stdout
+
+    hg_path = shutil.which("hg")
+    if hg_path:
+        proc = subprocess.run(
+            [hg_path, "root"],
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            universal_newlines=True,
+            check=False,
+        )
+        stdout, stderr = proc.stdout, proc.stderr
 
-    # If there was no error return the output
-    if not stderr:
-        logger.add_event("using_hg")
-        return stdout
+        # If there was no error return the output
+        if not stderr:
+            logger.add_event("using_hg")
+            return stdout
 
     # Not a git or hg repo, go with current dir as a default
     logger.add_event("used_outside_repo")

src/pathpicker/state_files.py

@@ -3,6 +3,7 @@
 # This source code is licensed under the MIT license found in the
 # LICENSE file in the root directory of this source tree.
 import os
+import stat
 from typing import List
 
 FPP_DIR = os.environ.get("FPP_DIR") or "~/.cache/fpp"
@@ -43,6 +44,23 @@ def get_logger_file_path() -> str:
     return os.path.expanduser(os.path.join(FPP_DIR, LOGGER_FILE))
 
 
+def is_state_file_safe(path: str) -> bool:
+    """
+    Basic safety checks before unpickling:
+      - file must exist and be owned by current uid
+      - not group/world-writable
+    """
+    try:
+        st = os.stat(path)
+    except OSError:
+        return False
+    if st.st_uid != os.getuid():
+        return False
+    if st.st_mode & (stat.S_IWGRP | stat.S_IWOTH):
+        return False
+    return True
+
+
 def get_all_state_files() -> List[str]:
     # keep this update to date! We do not include
     # the script output path since that gets cleaned automatically

src/process_input.py

@@ -5,6 +5,7 @@
 import os
 import pickle
 import sys
+import tempfile
 from typing import Dict, List
 
 from pathpicker import parse, state_files
@@ -59,8 +60,14 @@ def get_line_objs_from_lines(
 def do_program(flags: ScreenFlags) -> None:
     file_path = state_files.get_pickle_file_path()
     line_objs = get_line_objs(flags)
-    # pickle it so the next program can parse it
-    pickle.dump(line_objs, open(file_path, "wb"))
+    # write atomically and set restrictive permissions
+    dirpath = os.path.dirname(file_path)
+    with tempfile.NamedTemporaryFile(dir=dirpath, delete=False) as tf:
+        pickle.dump(line_objs, tf, protocol=pickle.HIGHEST_PROTOCOL)
+        tf.flush()
+        os.fsync(tf.fileno())
+    os.replace(tf.name, file_path)
+    os.chmod(file_path, 0o600)
 
 
 def usage() -> None: