diff --git a/src/mvt/android/artifacts/dumpsys_adb.py b/src/mvt/android/artifacts/dumpsys_adb.py index ac397cfb..c57cf8d6 100644 --- a/src/mvt/android/artifacts/dumpsys_adb.py +++ b/src/mvt/android/artifacts/dumpsys_adb.py @@ -35,6 +35,15 @@ class DumpsysADBArtifact(AndroidArtifact): indent = len(line) - len(line.lstrip()) if indent < cur_indent: # If the current line is less indented than the previous one, back out + if len(stack) <= 1: + # Dedenting below the outermost level means this is not the + # well-formed block the parser expects. Stop here rather than + # raise IndexError out of the module on the next line. + self.log.error( + "Unexpected indentation in ADB manager state, " + "stopping the parse of this section" + ) + break stack.pop() cur_indent = indent else: @@ -64,6 +73,12 @@ class DumpsysADBArtifact(AndroidArtifact): current_dict = stack[-1] if key == "}": + if len(stack) <= 1: + self.log.error( + "Unbalanced closing brace in ADB manager state, " + "stopping the parse of this section" + ) + break stack.pop() continue @@ -162,6 +177,26 @@ class DumpsysADBArtifact(AndroidArtifact): f"'{user_key['fingerprint']}'" ) + @staticmethod + def _find_state_end(content: bytes, open_brace: int) -> int: + """Find the unindented line closing the ADB manager state, or -1. + + Braces inside key comments or embedded keystore data are values, while + nested structural closing braces are indented. + """ + line_start = open_brace + while line_start < len(content): + line_end = content.find(b"\n", line_start) + if line_end == -1: + line_end = len(content) + line = content[line_start:line_end].removesuffix(b"\r") + if line_start > open_brace and line == b"}": + return line_start + if line.startswith((b"---------", b"DUMP OF SERVICE ")): + break + line_start = line_end + 1 + return -1 + def parse(self, content: bytes) -> None: """ Parse the Dumpsys ADB section @@ -181,18 +216,16 @@ class DumpsysADBArtifact(AndroidArtifact): self.log.error("Unable to find ADB manager state in dumpsys output") return - end_of_json = max(content.rfind(b"}\n"), content.rfind(b"}\r\n")) - if end_of_json == -1 or end_of_json <= start_of_json: - self.log.error("Unable to find complete ADB manager state in dumpsys output") + end_of_json = self._find_state_end(content, start_of_json + 1) + if end_of_json == -1: + self.log.error( + "Unable to find complete ADB manager state in dumpsys output" + ) return - # Exclude the final nested closing brace regardless of its line ending. - # The indented parser finishes the open debugging_manager at EOF. - inner_end = content.rfind(b"}", start_of_json + 2, end_of_json) - if inner_end == -1: - self.log.error("Unable to find complete ADB manager state in dumpsys output") - return - json_content = content[start_of_json + 2 : inner_end].rstrip() + # The brace that opens the state and the one that closes it are not part + # of the indented body. + json_content = content[start_of_json + 2 : end_of_json].rstrip() parsed = self.indented_dump_parser(json_content) if parsed.get("debugging_manager") is None: diff --git a/tests/android/test_artifact_dumpsys_adb.py b/tests/android/test_artifact_dumpsys_adb.py index 2fcd0bba..0e5b59f4 100644 --- a/tests/android/test_artifact_dumpsys_adb.py +++ b/tests/android/test_artifact_dumpsys_adb.py @@ -164,6 +164,67 @@ class TestDumpsysADBArtifact: assert da_adb.results[0]["user_keys"][0]["user"] == "host@example" assert da_adb.results[0]["adb_wifi"]["enabled"] == b"false" + ADB_STATE = ( + b"ADB MANAGER STATE (dumpsys adb):\n" + b"{\n" + b" debugging_manager={\n" + b" connected_to_adb=true\n" + b" user_keys=QUJDRA== host@example\n" + b" }\n" + b"}\n" + b"--------- 0.5s was the duration of 'dumpsys adb'\n" + ) + + def test_a_later_dumpsys_section_does_not_extend_the_adb_state(self): + # A bug report holds many sections. Looking for the last closing brace + # in the whole output pulled a later section into this one, which threw + # IndexError out of parse() and lost the ADB records entirely. + da_adb = DumpsysADBArtifact() + da_adb.parse( + self.ADB_STATE + b"DUMP OF SERVICE other:\n" + b" debugging_manager={\n" + b" connected_to_adb=false\n" + b" user_keys=RVZJTA== attacker@host\n" + b" }\n" + b"}\n" + ) + + assert len(da_adb.results) == 1 + assert [key["user"] for key in da_adb.results[0]["user_keys"]] == [ + "host@example" + ] + assert da_adb.results[0]["connected_to_adb"] is True + + def test_braces_in_key_comment_are_not_state_delimiters(self): + for user in (b"host{example", b"host}}example"): + da_adb = DumpsysADBArtifact() + da_adb.parse( + b"ADB MANAGER STATE (dumpsys adb):\n" + b"{\n" + b" debugging_manager={\n" + b" connected_to_adb=true\n" + b" user_keys=QUJDRA== " + user + b"\n }\n}\n" + ) + + assert len(da_adb.results) == 1 + assert da_adb.results[0]["user_keys"][0]["user"] == user.decode() + + def test_unbalanced_state_is_reported_rather_than_raising(self): + da_adb = DumpsysADBArtifact() + da_adb.parse( + b"ADB MANAGER STATE (dumpsys adb):\n" + b"{\n" + b" debugging_manager={\n" + b" connected_to_adb=true\n" + b" }\n" + b" }\n" + b"}\n" + ) + + # No exception, and whatever was read before the bad line is kept. + assert len(da_adb.results) == 1 + assert da_adb.results[0]["connected_to_adb"] is True + class TestDumpsysADBStateAlerts: def test_no_androidqf_context_preserves_existing_behavior(self):