{"thread":{"id":"52772","subject":"git-patch-id and syntactically significant whitespace","startedAt":"2020-02-10T16:41:25Z","lastAt":"2020-02-10T22:24:45Z","messageCount":2,"participants":["Konstantin Ryabitsev","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"391471","messageId":"20200210164115.x4gciujyjisivfgi@chatter.i7.local","threadId":"52772","inReplyTo":null,"subject":"git-patch-id and syntactically significant whitespace","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2020-02-10T16:41:15Z","receivedAt":"2020-02-10T16:41:25Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"Hello:\n\nGit patch-id is often used as convenient way to represent patches based \non their content. It accomplishes this by attempting to normalize a \npatch before producing a hash of the result -- most notably, by trimming \na lot of whitespace.\n\nUnfortunately, this does not work well with syntactically-significant \nwhitespace languages, notably Python and Make. For example, the \nfollowing two patches produce identical patch-id's, but one of them is \nactually malicious.\n\nBenign:\n\ndiff --git a/file1.py b/file1.py\nindex e574c49..6aa1937 100644\n--- a/file1.py\n+++ b/file1.py\n@@ -1,3 +1,13 @@\n #!/usr/bin/python\n\n+def is_logged_in(cookie):\n+    if cookie:\n+        print('User is logged in')\n+        return True\n+\n+    return False\n+\n+if is_logged_in(True):\n+    print('You are logged in')\n+\n print('Hello!')\n\nMalicious (\"return True\" is unindented, which results in is_logged_in() \nalways returning \"True\"):\n\ndiff --git a/file1.py b/file1.py\nindex e574c49..6aa1937 100644\n--- a/file1.py\n+++ b/file1.py\n@@ -1,3 +1,13 @@\n #!/usr/bin/python\n \n+def is_logged_in(cookie):\n+    if cookie:\n+        print('User is logged in')\n+    return True\n+\n+    return False\n+\n+if is_logged_in(True):\n+    print('You are logged in')\n+\n print('Hello!')\n\nThis mostly becomes a problem if we try to build any kind of patch \nindexing/retrieval systems that rely on patch-id to identify patches.  \nWhile this is not a high-impact problem by any means, it's not a \ntheoretical concern: git-format-patch includes functionality to provide \npatch dependencies via prerequisite-patch-id trailers [1]. An automated \nsystem attempting to auto-fetch dependencies can potentially retrieve \nand apply the malicious version of the patch.\n\nI'm not sure what the solution here is, since changing git-patch-id to \nnot discard whitespace is obviously going to defeat its entire purpose \nof \"not ever changing\". I mostly wanted to share my findings in case \nsomeone has thoughts on how to best approach this.\n\n-K\n\n[1] https://git-scm.com/docs/git-format-patch#_base_tree_information\n"},{"id":"391504","messageId":"20200210222443.GA623231@coredump.intra.peff.net","threadId":"52772","inReplyTo":"20200210164115.x4gciujyjisivfgi@chatter.i7.local","subject":"Re: git-patch-id and syntactically significant whitespace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-10T22:24:43Z","receivedAt":"2020-02-10T22:24:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 10, 2020 at 11:41:15AM -0500, Konstantin Ryabitsev wrote:\n\n> This mostly becomes a problem if we try to build any kind of patch \n> indexing/retrieval systems that rely on patch-id to identify patches.  \n> While this is not a high-impact problem by any means, it's not a \n> theoretical concern: git-format-patch includes functionality to provide \n> patch dependencies via prerequisite-patch-id trailers [1]. An automated \n> system attempting to auto-fetch dependencies can potentially retrieve \n> and apply the malicious version of the patch.\n> \n> I'm not sure what the solution here is, since changing git-patch-id to \n> not discard whitespace is obviously going to defeat its entire purpose \n> of \"not ever changing\". I mostly wanted to share my findings in case \n> someone has thoughts on how to best approach this.\n\nCan't you already have malicious patch-id collisions without the\nwhitespace thing? The patch-id also throws away line numbers, so a patch\nadding \"return 0\" in an innocent location could have the same patch-id\nas one adding it somewhere more dangerous. It's just a question of the\ncontext, but there's often enough boilerplate for two functions to look\nsimilar.\n\nThis is occasionally a problem for actual accidental collisions (in\npatch-ids, but also when merging). I can imagine it's probably not that\nhard for a determined attacker to make such a case intentionally.\n\n-Peff\n"}]}