diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 1a1b8fa12..bd6c471ff 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,19 @@ Changelog ========= +3.1.60 +====== + +Security fixes for + +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-g5vv-9gxw-82hx + +If you can, also try and provide feedback on the upcoming v4 branch +https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. + +See the following for all changes. +https://github.com/gitpython-developers/GitPython/releases/tag/3.1.60 + 3.1.59 ====== diff --git a/git/config.py b/git/config.py index 6f26e58fc..e7f64f7b5 100644 --- a/git/config.py +++ b/git/config.py @@ -462,7 +462,8 @@ def string_decode(v: str) -> str: v = v[:-1] # END cut trailing escapes to prevent decode error - return v.encode(defenc).decode("unicode_escape") + escapes = {"b": "\b", "n": "\n", "r": "\r", "t": "\t", '"': '"', "\\": "\\"} + return re.sub(r"\\(.)", lambda match: escapes.get(match.group(1), match.group(0)), v) # END string_decode @@ -517,10 +518,12 @@ def string_decode(v: str) -> str: # Opens quoting and does not close: appears to start multi-line quoting. is_multi_line = True optval = string_decode(optval[1:]) - elif optval.find("\\", 1, -1) == -1 and optval.find('"', 1, -1) == -1: - # Opens and closes quoting. Single line, and all we need is quote removal. - optval = optval[1:-1] - # TODO: Handle other quoted content, especially well-formed backslash escapes. + elif re.search(r'(?:^|[^\\])(?:\\\\)*"', optval[1:-1]): + # Preserve malformed values containing unescaped quotes. + pass + else: + # Opens and closes quoting. + optval = string_decode(optval[1:-1]) # Preserves multiple values for duplicate optnames. cursect.add(optname, optval) @@ -706,7 +709,7 @@ def write_section(name: str, section_dict: _OMD) -> None: for v in values: value = self._value_to_string(v) - if any(char in value for char in '\n\t\b\\"'): + if any(char in value for char in '\n\t\b\\"#;') or value[:1].isspace() or value[-1:].isspace(): value = value.replace("\\", "\\\\").replace('"', '\\"') value = '"%s\\\n"' % value.replace("\n", "\\n").replace("\t", "\\t").replace("\b", "\\b") fp.write(("\t%s = %s\n" % (key, value)).encode(defenc)) @@ -768,6 +771,20 @@ def write(self) -> None: return # END stop if we have include files + sections: List[_OMD] = [self._defaults] + section: _OMD + stored_section: _OMD + values: List[Any] + raw_value: Any + for _, stored_section in self._sections.items(): + sections.append(stored_section) + for section in sections: + for key, values in section.items_all(): + if key != "__name__": + for raw_value in values: + if "\r" in self._value_to_string(raw_value) or "\x00" in self._value_to_string(raw_value): + raise ValueError("Git config values must not contain CR or NUL") + fp = self._file_or_files # We have a physical file on disk, so get a lock. diff --git a/git/diff.py b/git/diff.py index d1963b84f..f89f3126f 100644 --- a/git/diff.py +++ b/git/diff.py @@ -95,14 +95,35 @@ class DiffConstants(enum.Enum): :const:`git.INDEX` and :const:`Diffable.INDEX`, as well as :const:`Diffable.Index`. """ -_octal_byte_re = re.compile(rb"\\([0-9]{3})") - -def _octal_repl(matchobj: Match) -> bytes: - value = matchobj.group(1) - value = int(value, 8) - value = bytes(bytearray((value,))) - return value +def _unquote_path(path: bytes) -> bytes: + result = bytearray() + escapes = { + ord("a"): 7, + ord("b"): 8, + ord("f"): 12, + ord("n"): 10, + ord("r"): 13, + ord("t"): 9, + ord("v"): 11, + } + i = 0 + while i < len(path): + if path[i] != ord("\\") or i + 1 == len(path): + result.append(path[i]) + i += 1 + continue + if path[i + 1] in b"0123" and i + 3 < len(path) and all(c in b"01234567" for c in path[i + 2 : i + 4]): + result.append(int(path[i + 1 : i + 4], 8)) + i += 4 + continue + escaped = path[i + 1] + if escaped in escapes or escaped in b'\\"': + result.append(escapes.get(escaped, escaped)) + else: + result.extend(path[i : i + 2]) + i += 2 + return bytes(result) def decode_path(path: bytes, has_ab_prefix: bool = True) -> Optional[bytes]: @@ -110,9 +131,7 @@ def decode_path(path: bytes, has_ab_prefix: bool = True) -> Optional[bytes]: return None if path.startswith(b'"') and path.endswith(b'"'): - path = path[1:-1].replace(b"\\n", b"\n").replace(b"\\t", b"\t").replace(b'\\"', b'"').replace(b"\\\\", b"\\") - - path = _octal_byte_re.sub(_octal_repl, path) + path = _unquote_path(path[1:-1]) if has_ab_prefix: assert path.startswith(b"a/") or path.startswith(b"b/") diff --git a/git/util.py b/git/util.py index 02f57c132..b0593feea 100644 --- a/git/util.py +++ b/git/util.py @@ -858,10 +858,6 @@ class Actor: committers and authors or anything with a name and an email as mentioned in the git log entries.""" - # PRECOMPILED REGEX - name_only_regex = re.compile(r"<(.*)>") - name_email_regex = re.compile(r"(.*) <(.*?)>") - # ENVIRONMENT VARIABLES # These are read when creating new commits. env_author_name = "GIT_AUTHOR_NAME" @@ -906,18 +902,14 @@ def _from_string(cls, string: str) -> "Actor": :return: :class:`Actor` """ - m = cls.name_email_regex.search(string) - if m: - name, email = m.groups() - return Actor(name, email) - else: - m = cls.name_only_regex.search(string) - if m: - return Actor(m.group(1), None) - # Assume the best and use the whole string as name. - return Actor(string, None) - # END special case name - # END handle name/email matching + line = string.partition("\n")[0] + left_bracket = line.find("<") + right_bracket = line.find(">", left_bracket + 1) + if left_bracket >= 0 and right_bracket >= 0: + return Actor(line[:left_bracket].rstrip(), line[left_bracket + 1 : right_bracket]) + + # Assume the best and use the whole string as name. + return Actor(string, None) @classmethod def _main_actor( diff --git a/test/test_actor.py b/test/test_actor.py index 5e6635709..baf6545f1 100644 --- a/test/test_actor.py +++ b/test/test_actor.py @@ -27,6 +27,26 @@ def test_from_string_should_handle_just_name(self): self.assertEqual("Michael Trier", a.name) self.assertEqual(None, a.email) + def test_from_string_handles_unterminated_email_without_regex_backtracking(self): + value = "A" * 20_000 + " \n y "), Actor("x", "a")) + + def test_from_string_uses_git_delimiters(self): + for value, expected in ( + ("Name ", Actor("Name", "e>", Actor("Name", "email")), + ("Name", Actor("Name", "email")), + (" <>", Actor("", "")), + ("Name ", Actor("Name email>", None)), + ): + self.assertEqual(Actor._from_string(value), expected) + def test_should_display_representation(self): a = Actor._from_string("Michael Trier ") self.assertEqual('">', repr(a)) diff --git a/test/test_config.py b/test/test_config.py index d664fdb6f..28bb12043 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -14,6 +14,7 @@ import pytest from git import GitConfigParser +from git.compat import defenc from git.config import _OMD, cp from git.util import cwd, rmfile from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory @@ -170,7 +171,16 @@ def test_rewriting_multiline_value_does_not_create_option(self, rw_dir): @with_rw_directory def test_writer_escapes_special_characters_without_newline(self, rw_dir): config_path = osp.join(rw_dir, "config") - values = {"tab": "\tvalue\t", "backspace": "a\bb", "quote": 'a"b', "backslash": "a\\qb"} + values = { + "tab": "\tvalue\t", + "backspace": "a\bb", + "quote": 'a"b', + "backslash": "a\\qb", + "hash": "value#fragment", + "semicolon": "value;fragment", + "leading": " value", + "trailing": "value ", + } with GitConfigParser(config_path, read_only=False) as git_config: for key, value in values.items(): @@ -185,11 +195,72 @@ def test_writer_escapes_special_characters_without_newline(self, rw_dir): stdout=subprocess.PIPE, check=True, ).stdout, - value.encode() + b"\n", + value.encode(defenc) + b"\n", ) with open(config_path, "rb") as config_file: self.assertNotIn(b"\x08", config_file.read()) + @with_rw_directory + def test_writer_preserves_escaped_and_non_ascii_values_safely(self, rw_dir): + config_path = osp.join(rw_dir, "config") + with open(config_path, "wb") as config_file: + config_file.write( + ( + '[section]\nnewline = "first\\nsecond"\nquote = "a\\"b"\nbackslash = "a\\\\b"\n' + 'unicode = "café\\\\path"\n' + ).encode(defenc) + ) + + with GitConfigParser(config_path, read_only=False) as config: + config.set_value("unrelated", "key", "value") + + expected = { + "newline": "first\nsecond", + "quote": 'a"b', + "backslash": "a\\b", + "unicode": "café\\path", + } + with GitConfigParser(config_path, read_only=True) as config: + for key, value in expected.items(): + self.assertEqual( + config.get_value("section", key), + value, + "GitPython should preserve values when rewriting unrelated entries", + ) + self.assertEqual( + subprocess.run( + ["git", "config", "--file", config_path, "--get", "section.%s" % key], + stdout=subprocess.PIPE, + check=True, + ).stdout, + value.encode(defenc) + b"\n", + "git should read rewritten values with the same semantics", + ) + + with open(config_path, "rb") as config_file: + contents = config_file.read() + self.assertNotIn(b"\r", contents, "the writer should never emit carriage returns") + self.assertNotIn(b"\x00", contents, "the writer should never emit NUL bytes") + + for name, value in (("return", b"first\\rsecond"), ("nul", b"first\x00second")): + unsafe_path = osp.join(rw_dir, "%s-config" % name) + unsafe_contents = b'[section]\nvalue = "' + value + b'"\n' + with open(unsafe_path, "wb") as config_file: + config_file.write(unsafe_contents) + with self.assertRaisesRegex( + ValueError, + "CR or NUL", + msg="unsafe existing values should abort rewrites", + ): + with GitConfigParser(unsafe_path, read_only=False) as config: + config.set_value("unrelated", "key", "value") + with open(unsafe_path, "rb") as config_file: + self.assertEqual( + config_file.read(), + unsafe_contents, + "rejected rewrites should leave the original file unchanged", + ) + @with_rw_directory def test_set_value_rejects_config_injection(self, rw_dir): config_path = osp.join(rw_dir, "config") @@ -745,15 +816,14 @@ def test_config_with_quotes_with_whitespace_outside_value(self): self.assertEqual(cr.get("init", "defaultBranch"), "trunk") def test_config_with_quotes_containing_escapes(self): - """For now just suppress quote removal. But it would be good to interpret most of these.""" + """Interpret Git's quoted escapes without changing malformed values.""" cr = GitConfigParser(fixture_path("git_config_with_quotes_escapes"), read_only=True) - # These can eventually be supported by substituting the represented character. - self.assertEqual(cr.get("custom", "hasnewline"), R'"first\nsecond"') - self.assertEqual(cr.get("custom", "hasbackslash"), R'"foo\\bar"') - self.assertEqual(cr.get("custom", "hasquote"), R'"ab\"cd"') - self.assertEqual(cr.get("custom", "hastrailingbackslash"), R'"word\\"') - self.assertEqual(cr.get("custom", "hasunrecognized"), R'"p\qrs"') + self.assertEqual(cr.get("custom", "hasnewline"), "first\nsecond") + self.assertEqual(cr.get("custom", "hasbackslash"), R"foo\bar") + self.assertEqual(cr.get("custom", "hasquote"), 'ab"cd') + self.assertEqual(cr.get("custom", "hastrailingbackslash"), "word\\") + self.assertEqual(cr.get("custom", "hasunrecognized"), R"p\qrs") # It is less obvious whether and what to eventually do with this. self.assertEqual(cr.get("custom", "hasunescapedquotes"), '"ab"cd"e"') diff --git a/test/test_diff.py b/test/test_diff.py index d5e14f3de..92f3876c7 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -14,6 +14,7 @@ from git import NULL_TREE, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule from git.cmd import Git +from git.diff import decode_path from git.exc import UnsafeOptionError from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory @@ -324,6 +325,11 @@ def test_diff_patch_format(self): Diff._index_from_patch_format(self.rorepo, diff_proc) # END for each fixture + def test_decode_path_distinguishes_escaped_backslashes_from_octal_bytes(self): + self.assertEqual(decode_path(b'"foo\\\\899bar"', False), b"foo\\899bar") + self.assertEqual(decode_path(b'"foo\\\\123bar"', False), b"foo\\123bar") + self.assertEqual(decode_path(b'"foo\\123bar"', False), b"fooSbar") + def test_diff_with_spaces(self): data = StringProcessAdapter(fixture("diff_file_with_spaces")) diff_index = Diff._index_from_patch_format(self.rorepo, data)