From b2afce5c79b9305028e1ff7bb226a0ef3d9affc5 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Fri, 30 Jul 2021 23:59:12 -0400 Subject: [PATCH] Avoid breakage with paths with unusual names If file_path has any whitespace or shell metacharacters in it, then the invocation of subprocess.call would be likely to break (or even accidentally execute code, depending on how perverse the pathnames are). It's generally a good plan to avoid shell=True for subprocess.call where you can lay out the arguments deliberately in python. This one looks relatively straightforward (but note, i have not tested it, sorry!) Note that if a name has a `"` character in it, we still fail, out of safety reasons. in particular, we want to avoid command injection into the sqlite binary with particularly malicious names that look something like the following: ``` foo.db"; .shell touch should-not-exist; .nullvalue " ``` --- mvt/ios/modules/fs/base.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/mvt/ios/modules/fs/base.py b/mvt/ios/modules/fs/base.py index a93a770..4c2d145 100644 --- a/mvt/ios/modules/fs/base.py +++ b/mvt/ios/modules/fs/base.py @@ -50,13 +50,14 @@ class IOSExtraction(MVTModule): if not shutil.which("sqlite3"): raise DatabaseCorruptedError("Unable to recover without sqlite3 binary. Please install sqlite3!") + if '"' in file_path: + raise DatabaseCorruptedError(f"Database at path '{file_path}' is corrupted. unable to recover because it has a quotation mark (\") in its name.") bak_path = f"{file_path}.bak" shutil.move(file_path, bak_path) - cmd = f"sqlite3 {bak_path} '.clone {file_path}'" - ret = subprocess.call(cmd, shell=True, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + ret = subprocess.call(['sqlite3', bak_path, f'.clone "{file_path}"'], + stdout=subprocess.PIPE, stderr=subprocess.PIPE) if ret != 0: raise DatabaseCorruptedError("Recovery of database failed")