git-patch-id and syntactically significant whitespace

Commit Message

Konstantin Ryabitsev Feb. 10, 2020, 4:41 p.m. UTC

Hello:

Git patch-id is often used as convenient way to represent patches based 
on their content. It accomplishes this by attempting to normalize a 
patch before producing a hash of the result -- most notably, by trimming 
a lot of whitespace.

Unfortunately, this does not work well with syntactically-significant 
whitespace languages, notably Python and Make. For example, the 
following two patches produce identical patch-id's, but one of them is 
actually malicious.

Benign:


This mostly becomes a problem if we try to build any kind of patch 
indexing/retrieval systems that rely on patch-id to identify patches.  
While this is not a high-impact problem by any means, it's not a 
theoretical concern: git-format-patch includes functionality to provide 
patch dependencies via prerequisite-patch-id trailers [1]. An automated 
system attempting to auto-fetch dependencies can potentially retrieve 
and apply the malicious version of the patch.

I'm not sure what the solution here is, since changing git-patch-id to 
not discard whitespace is obviously going to defeat its entire purpose 
of "not ever changing". I mostly wanted to share my findings in case 
someone has thoughts on how to best approach this.

-K

[1] https://git-scm.com/docs/git-format-patch#_base_tree_information

Comments

Jeff King Feb. 10, 2020, 10:24 p.m. UTC | #1

On Mon, Feb 10, 2020 at 11:41:15AM -0500, Konstantin Ryabitsev wrote:

> This mostly becomes a problem if we try to build any kind of patch 
> indexing/retrieval systems that rely on patch-id to identify patches.  
> While this is not a high-impact problem by any means, it's not a 
> theoretical concern: git-format-patch includes functionality to provide 
> patch dependencies via prerequisite-patch-id trailers [1]. An automated 
> system attempting to auto-fetch dependencies can potentially retrieve 
> and apply the malicious version of the patch.
> 
> I'm not sure what the solution here is, since changing git-patch-id to 
> not discard whitespace is obviously going to defeat its entire purpose 
> of "not ever changing". I mostly wanted to share my findings in case 
> someone has thoughts on how to best approach this.

Can't you already have malicious patch-id collisions without the
whitespace thing? The patch-id also throws away line numbers, so a patch
adding "return 0" in an innocent location could have the same patch-id
as one adding it somewhere more dangerous. It's just a question of the
context, but there's often enough boilerplate for two functions to look
similar.

This is occasionally a problem for actual accidental collisions (in
patch-ids, but also when merging). I can imagine it's probably not that
hard for a determined attacker to make such a case intentionally.

-Peff

Patch

diff --git a/file1.py b/file1.py
index e574c49..6aa1937 100644
--- a/file1.py
+++ b/file1.py
@@ -1,3 +1,13 @@ 
 #!/usr/bin/python

+def is_logged_in(cookie):
+    if cookie:
+        print('User is logged in')
+        return True
+
+    return False
+
+if is_logged_in(True):
+    print('You are logged in')
+
 print('Hello!')

Malicious ("return True" is unindented, which results in is_logged_in() 
always returning "True"):

diff --git a/file1.py b/file1.py
index e574c49..6aa1937 100644
--- a/file1.py
+++ b/file1.py
@@ -1,3 +1,13 @@ 
 #!/usr/bin/python
 
+def is_logged_in(cookie):
+    if cookie:
+        print('User is logged in')
+    return True
+
+    return False
+
+if is_logged_in(True):
+    print('You are logged in')
+
 print('Hello!')