diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a30cc5bcf0..a1eb3caf82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,6 +103,7 @@ jobs: cd src/cli pip install -r requirements-lint.txt flake8 . + bandit -r ./onefuzz/ black onefuzz examples tests --check isort --profile black ./onefuzz ./examples/ ./tests/ --check pytest -v tests @@ -208,6 +209,7 @@ jobs: pip install -r requirements-dev.txt pytest flake8 . + bandit -r ./__app__/ black ./__app__/ ./tests --check isort --profile black ./__app__/ ./tests --check mypy __app__ ./tests diff --git a/src/api-service/__app__/onefuzzlib/azure/storage.py b/src/api-service/__app__/onefuzzlib/azure/storage.py index a4bc403e83..f03e1eb6c2 100644 --- a/src/api-service/__app__/onefuzzlib/azure/storage.py +++ b/src/api-service/__app__/onefuzzlib/azure/storage.py @@ -86,7 +86,9 @@ def choose_account(storage_type: StorageType) -> str: # Use a random secondary storage account if any are available. This # reduces IOP contention for the Storage Queues, which are only available # on primary accounts - return random.choice(accounts[1:]) + # + # security note: this is not used as a security feature + return random.choice(accounts[1:]) # nosec @cached diff --git a/src/api-service/requirements-dev.txt b/src/api-service/requirements-dev.txt index 9265460e7e..4ccfdb543f 100644 --- a/src/api-service/requirements-dev.txt +++ b/src/api-service/requirements-dev.txt @@ -4,3 +4,4 @@ mypy isort vulture black +bandit diff --git a/src/ci/onefuzztypes.sh b/src/ci/onefuzztypes.sh index ae2a637b4a..110b668a24 100755 --- a/src/ci/onefuzztypes.sh +++ b/src/ci/onefuzztypes.sh @@ -14,6 +14,7 @@ python setup.py sdist bdist_wheel pip install -r requirements-lint.txt black ./onefuzztypes --check flake8 ./onefuzztypes +bandit -r ./onefuzztypes isort --profile black ./onefuzztypes --check mypy ./onefuzztypes --ignore-missing-imports pytest -v tests diff --git a/src/cli/onefuzz/api.py b/src/cli/onefuzz/api.py index 6d7b8094d2..44e7a4958b 100644 --- a/src/cli/onefuzz/api.py +++ b/src/cli/onefuzz/api.py @@ -64,9 +64,13 @@ def is_uuid(value: str) -> bool: A = TypeVar("A", bound=BaseModel) -def wsl_path(path: str) -> str: +def _wsl_path(path: str) -> str: if which("wslpath"): - return subprocess.check_output(["wslpath", "-w", path]).decode().strip() + # security note: path should always be a temporary path constructed by + # this library + return ( + subprocess.check_output(["wslpath", "-w", path]).decode().strip() # nosec + ) return path @@ -530,7 +534,9 @@ def _dbg_linux( dbg += ["--batch"] try: - return subprocess.run( + # security note: dbg is built from content coming from + # the server, which is trusted in this context. + return subprocess.run( # nosec dbg, stdout=subprocess.PIPE, stderr=subprocess.STDOUT ).stdout.decode(errors="ignore") except subprocess.CalledProcessError as err: @@ -539,7 +545,9 @@ def _dbg_linux( ) raise err else: - subprocess.call(dbg) + # security note: dbg is built from content coming from the + # server, which is trusted in this context. + subprocess.call(dbg) # nosec return None def _dbg_windows( @@ -561,11 +569,13 @@ def _dbg_windows( if debug_command: dbg_script = [debug_command, "qq"] with temp_file("db.script", "\r\n".join(dbg_script)) as dbg_script_path: - dbg += ["-cf", wsl_path(dbg_script_path)] + dbg += ["-cf", _wsl_path(dbg_script_path)] logging.debug("launching: %s", dbg) try: - return subprocess.run( + # security note: dbg is built from content coming from the server, + # which is trusted in this context. + return subprocess.run( # nosec dbg, stdout=subprocess.PIPE, stderr=subprocess.STDOUT ).stdout.decode(errors="ignore") except subprocess.CalledProcessError as err: @@ -575,7 +585,9 @@ def _dbg_windows( raise err else: logging.debug("launching: %s", dbg) - subprocess.call(dbg) + # security note: dbg is built from content coming from the + # server, which is trusted in this context. + subprocess.call(dbg) # nosec return None diff --git a/src/cli/onefuzz/debug.py b/src/cli/onefuzz/debug.py index 2723399c7d..c382d21a68 100644 --- a/src/cli/onefuzz/debug.py +++ b/src/cli/onefuzz/debug.py @@ -360,7 +360,12 @@ def download_files(self, job_id: UUID_EXPANSION, output: Directory) -> None: if not os.path.exists(outdir): os.makedirs(outdir) self.logger.info("downloading: %s", name) - subprocess.check_output([azcopy, "sync", to_download[name], outdir]) + # security note: the src for azcopy comes from the server which is + # trusted in this context, while the destination is provided by the + # user + subprocess.check_output( # nosec + [azcopy, "sync", to_download[name], outdir] + ) class DebugLog(Command): diff --git a/src/cli/onefuzz/job_templates/handlers.py b/src/cli/onefuzz/job_templates/handlers.py index ae9956c7d6..db4d3cf948 100644 --- a/src/cli/onefuzz/job_templates/handlers.py +++ b/src/cli/onefuzz/job_templates/handlers.py @@ -68,9 +68,12 @@ def _define_missing_containers( if container_type == container.type: seen = True if not seen: - assert isinstance(request.user_fields["project"], str) - assert isinstance(request.user_fields["name"], str) - assert isinstance(request.user_fields["build"], str) + if not isinstance(request.user_fields["project"], str): + raise TypeError + if not isinstance(request.user_fields["name"], str): + raise TypeError + if not isinstance(request.user_fields["build"], str): + raise TypeError container_name = _build_container_name( self.onefuzz, container_type, diff --git a/src/cli/onefuzz/rdp.py b/src/cli/onefuzz/rdp.py index 014210721e..2f1359ca80 100644 --- a/src/cli/onefuzz/rdp.py +++ b/src/cli/onefuzz/rdp.py @@ -25,7 +25,9 @@ def rdp_connect(ip: str, password: str, *, port: int) -> Generator: ) encrypted = ( - subprocess.check_output( + # security note: script includes content coming from the server, + # which is trusted in this context. + subprocess.check_output( # nosec ["powershell.exe", "-ExecutionPolicy", "Unrestricted", script] ) .decode() @@ -46,5 +48,7 @@ def rdp_connect(ip: str, password: str, *, port: int) -> Generator: os.chdir(tmpdir) cmd = ["mstsc.exe", FILENAME] logging.info("launching rdp: %s", " ".join(cmd)) - yield subprocess.call(cmd) + # security note: mstsc.exe is called using a config with data coming + # from the server, which is trusted in this context. + yield subprocess.call(cmd) # nosec os.chdir(previous) diff --git a/src/cli/onefuzz/ssh.py b/src/cli/onefuzz/ssh.py index 126eb0373e..7782926a0a 100644 --- a/src/cli/onefuzz/ssh.py +++ b/src/cli/onefuzz/ssh.py @@ -33,7 +33,7 @@ def get_local_tmp() -> Optional[str]: @contextmanager def temp_file( - filename: str, content: str, *, permissions: Optional[str] = None + filename: str, content: str, *, set_owner_only: bool = False ) -> Generator: with tempfile.TemporaryDirectory(dir=get_local_tmp()) as tmpdir: full_path = os.path.join(tmpdir, filename) @@ -42,8 +42,10 @@ def temp_file( with open(full_path, "w") as handle: handle.write(content) - if permissions is not None and platform.system() != "Windows": - subprocess.check_call(["chmod", permissions, full_path]) + if set_owner_only and platform.system() != "Windows": + # security note: full_path is created via callers using known static + # filenames within the newly created temporary file name + subprocess.check_call(["chmod", "600", full_path]) # nosec yield full_path @@ -59,7 +61,7 @@ def build_ssh_command( port: Optional[int] = None, command: Optional[str] = None, ) -> Generator: - with temp_file("id_rsa", private_key, permissions="600") as ssh_key: + with temp_file("id_rsa", private_key, set_owner_only=True) as ssh_key: cmd = [ "ssh", "onefuzz@%s" % ip, @@ -102,10 +104,14 @@ def ssh_connect( logging.info("launching ssh: %s", " ".join(cmd)) if call: - yield subprocess.call(cmd) + # security note: command includes user provided arguments + # intentionally + yield subprocess.call(cmd) # nosec return - with subprocess.Popen( + # security note: command includes user provided arguments + # intentionally + with subprocess.Popen( # nosec cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE, bufsize=0 ) as ssh: yield ssh diff --git a/src/cli/onefuzz/templates/ossfuzz.py b/src/cli/onefuzz/templates/ossfuzz.py index 7830ad4f22..25c2fe1b29 100644 --- a/src/cli/onefuzz/templates/ossfuzz.py +++ b/src/cli/onefuzz/templates/ossfuzz.py @@ -80,7 +80,9 @@ def _copy_all(self, src_sas: str, dst_sas: str) -> None: dst_sas, ] self.logger.info("copying base setup") - subprocess.check_output(cmd) + # security note: the source and destination container sas URLS are + # considerd trusted from the service + subprocess.check_output(cmd) # nosec def _copy_exe(self, src_sas: str, dst_sas: str, target_exe: File) -> None: files: List[File] = [target_exe] @@ -100,7 +102,9 @@ def _copy_exe(self, src_sas: str, dst_sas: str, target_exe: File) -> None: dst_url, ] self.logger.info("uploading %s", path) - subprocess.check_output(cmd) + # security note: the source and destination container sas URLS + # are considerd trusted from the service + subprocess.check_output(cmd) # nosec def libfuzzer( self, @@ -141,7 +145,10 @@ def libfuzzer( container_sas[name] = sas_url self.logger.info("uploading build artifacts") - subprocess.check_output( + + # security note: the container sas is considered trusted from the + # service + subprocess.check_output( # nosec [ "azcopy", "sync", @@ -151,7 +158,7 @@ def libfuzzer( "*fuzzer_seed_corpus.zip", ] ) - subprocess.check_output( + subprocess.check_output( # nosec [ "azcopy", "sync", diff --git a/src/cli/requirements-lint.txt b/src/cli/requirements-lint.txt index 27a97b8b59..9b029b51c0 100644 --- a/src/cli/requirements-lint.txt +++ b/src/cli/requirements-lint.txt @@ -4,3 +4,4 @@ pytest isort vulture black +bandit diff --git a/src/pytypes/requirements-lint.txt b/src/pytypes/requirements-lint.txt index 4568b5e405..1ec96780f9 100644 --- a/src/pytypes/requirements-lint.txt +++ b/src/pytypes/requirements-lint.txt @@ -5,3 +5,4 @@ isort pydantic vulture black +bandit