{"thread":{"id":"59130","subject":"[PATCH] tree-walk: disallow overflowing modes","startedAt":"2023-01-21T09:36:29Z","lastAt":"2023-01-26T11:36:06Z","messageCount":9,"participants":["René Scharfe","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470836","messageId":"d673fde7-7eb2-6306-86b6-1c1a4c988ee8@web.de","threadId":"59130","inReplyTo":null,"subject":"[PATCH] tree-walk: disallow overflowing modes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-21T09:36:09Z","receivedAt":"2023-01-21T09:36:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When parsing tree entries, reject mode values that don't fit into an\nunsigned int.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n tree-walk.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 74f4d710e8..5e7bc38600 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -17,6 +17,8 @@ static const char *get_mode(const char *str, unsigned int *modep)\n \twhile ((c = *str++) != ' ') {\n \t\tif (c < '0' || c > '7')\n \t\t\treturn NULL;\n+\t\tif ((mode << 3) >> 3 != mode)\n+\t\t\treturn NULL;\n \t\tmode = (mode << 3) + (c - '0');\n \t}\n \t*modep = mode;\n--\n2.39.1\n"},{"id":"470873","messageId":"Y8zquGar3rLyRdTp@coredump.intra.peff.net","threadId":"59130","inReplyTo":"d673fde7-7eb2-6306-86b6-1c1a4c988ee8@web.de","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-22T07:50:16Z","receivedAt":"2023-01-22T07:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 21, 2023 at 10:36:09AM +0100, René Scharfe wrote:\n\n> When parsing tree entries, reject mode values that don't fit into an\n> unsigned int.\n\nSeems reasonable. I don't think you can cause any interesting mischief\nhere, but it's cheap to check, and finding data problems earlier rather\nthan later is always good.\n\nShould it be s/unsigned int/uint16_t/, though?\n\n-Peff\n"},{"id":"470875","messageId":"044bdc8f-fdc9-dfd2-6cbb-941513467524@web.de","threadId":"59130","inReplyTo":"Y8zquGar3rLyRdTp@coredump.intra.peff.net","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-22T10:03:38Z","receivedAt":"2023-01-22T10:03:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.01.23 um 08:50 schrieb Jeff King:\n> On Sat, Jan 21, 2023 at 10:36:09AM +0100, René Scharfe wrote:\n>\n>> When parsing tree entries, reject mode values that don't fit into an\n>> unsigned int.\n>\n> Seems reasonable. I don't think you can cause any interesting mischief\n> here, but it's cheap to check, and finding data problems earlier rather\n> than later is always good.\n>\n> Should it be s/unsigned int/uint16_t/, though?\n\n\"mode\" is declared as unsigned int, and I was more concerned with\noverflowing that.\n\nWe could be more strict and reject everything that oversteps\nS_IFMT|ALLPERMS, but the latter is not defined everywhere.  But\npermission bits are well-known, so the magic number 07777 should be\nrecognizable enough.  Like this?\n\n--- >8 ---\nSubject: [PATCH v2] tree-walk: disallow overflowing modes\n\nWhen parsing tree entries, reject mode values with bits set outside file\ntype mask and permission bits.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n tree-walk.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 74f4d710e8..62da0e5c73 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -18,6 +18,8 @@ static const char *get_mode(const char *str, unsigned int *modep)\n \t\tif (c < '0' || c > '7')\n \t\t\treturn NULL;\n \t\tmode = (mode << 3) + (c - '0');\n+\t\tif (mode & ~(S_IFMT | 07777))\n+\t\t\treturn NULL;\n \t}\n \t*modep = mode;\n \treturn str;\n--\n2.39.1\n"},{"id":"470883","messageId":"xmqqk01esf0j.fsf@gitster.g","threadId":"59130","inReplyTo":"044bdc8f-fdc9-dfd2-6cbb-941513467524@web.de","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-22T16:36:12Z","receivedAt":"2023-01-22T16:36:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> We could be more strict and reject everything that oversteps\n> S_IFMT|ALLPERMS, but the latter is not defined everywhere.  But\n> permission bits are well-known, so the magic number 07777 should be\n> recognizable enough.  Like this?\n\nI do not quite see the reason why we want to be more strict than we\nalready are at this point in the code path.  Stricter mode check in\nreports FSCK_MSG_ZERO_PADDED_FILEMODE and FSCK_MSG_BAD_FILEMODE from\n\"fsck\", which I think is probably sufficient.\n\nAvoiding integer wraparound is a good idea, even if it were\nimpossible to induce misparsing of the tree data to lead to any\nsecurity issues, for the same reason why we check for zero padded\nfilemode, i.e. such a tree mode will allow the same tree object to\nbe given different object names.\n\nThanks.\n"},{"id":"470891","messageId":"Y82yXskkgH00vaGy@coredump.intra.peff.net","threadId":"59130","inReplyTo":"044bdc8f-fdc9-dfd2-6cbb-941513467524@web.de","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-22T22:02:06Z","receivedAt":"2023-01-22T22:02:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 22, 2023 at 11:03:38AM +0100, René Scharfe wrote:\n\n> Am 22.01.23 um 08:50 schrieb Jeff King:\n> > On Sat, Jan 21, 2023 at 10:36:09AM +0100, René Scharfe wrote:\n> >\n> >> When parsing tree entries, reject mode values that don't fit into an\n> >> unsigned int.\n> >\n> > Seems reasonable. I don't think you can cause any interesting mischief\n> > here, but it's cheap to check, and finding data problems earlier rather\n> > than later is always good.\n> >\n> > Should it be s/unsigned int/uint16_t/, though?\n> \n> \"mode\" is declared as unsigned int, and I was more concerned with\n> overflowing that.\n\nDoh, I just did \"vim -t get_mode\" and ended up in the near-identical\nversion in fast-import.c, which uses uint16_t. Maybe it would want the\nsame treatment. ;)\n\n> We could be more strict and reject everything that oversteps\n> S_IFMT|ALLPERMS, but the latter is not defined everywhere.  But\n> permission bits are well-known, so the magic number 07777 should be\n> recognizable enough.  Like this?\n\nIt feels like this level of check should be happening in the fsck\ncaller. In particular, it's nice for this parsing layer to err on the\nforgiving side, because the way you get out of these situations often\ninvolves something like \"git ls-tree | git mktree\".\n\nSo in that sense, even pushing the overflow detection into the fsck\nwould be nice, but of course it's hard to do, since we have no way to\nrepresent the overflowed value. I guess we could have a \"be more\ncareful\" flag that the fsck code would pass, but I'm not sure it's worth\nthe added complexity.\n\n-Peff\n"},{"id":"470901","messageId":"230123.86a629tzgc.gmgdl@evledraar.gmail.com","threadId":"59130","inReplyTo":"d673fde7-7eb2-6306-86b6-1c1a4c988ee8@web.de","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T08:33:57Z","receivedAt":"2023-01-23T08:41:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 21 2023, René Scharfe wrote:\n\n> When parsing tree entries, reject mode values that don't fit into an\n> unsigned int.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  tree-walk.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 74f4d710e8..5e7bc38600 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -17,6 +17,8 @@ static const char *get_mode(const char *str, unsigned int *modep)\n>  \twhile ((c = *str++) != ' ') {\n>  \t\tif (c < '0' || c > '7')\n>  \t\t\treturn NULL;\n> +\t\tif ((mode << 3) >> 3 != mode)\n> +\t\t\treturn NULL;\n>  \t\tmode = (mode << 3) + (c - '0');\n>  \t}\n>  \t*modep = mode;\n\nThere was a discussion about this on git-security last August, in a\nreport that turned out to be uninteresting for the security aspects.\n\nI'll just quote my own reply here out of context\n(<220811.86mtcbqd5x.gmgdl@evledraar.gmail.com> for those with access).\n\nFor context the other issue in the \"two issues\" below is the one I'm\nworking towards fixing in\nhttps://lore.kernel.org/git/cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com/,\nthe other is this file mode overflow.\n\nAs noted at the end below I'm conflicted between whether we should \"fix\"\nthis in some way, or just intentionally leave it alone, because while\nit's entirely accidental, this is the one part of git's object format\nI'm aware of that we could sneak in some extension in the future,\nwithout entirely breaking backwards compatibility.\n\nBEGIN QUOTE\n\nThere's really two issues being raised here, how we validate the \"mode\"\nin tree entries, and how we sometimes misreport object type X as type Y.\n\nI replied on-list just now noting that the \"mode\" thing is something I\nwas working towards fixing a while ago, but am happy to see a more\nisolated fix for:\nhttps://lore.kernel.org/git/220811.86r11nqfi2.gmgdl@evledraar.gmail.com/\n\nI have local patches for the misreporting of object types, it's not\n*that* hard to get right. Basically we just need to be more careful\nabout how we populate the \"struct object\". The most common case is when\nwe parse tags that say on their envelope that we refer to a type X, but\nit's really a type Y.\n\nIn that case we haven't .parsed=1 the object, but we note the wrong type\ndown. My local patches are basically just deferring that, so we don't\ntrust such type claims, and take our *actual* parsing of the object over\na previous reference in the object store.\n\nI don't think that leaves any difficult edge cases related to tree\nparsing, as Xavier notes we'd have modes claiming that an X is Y, but we\ncan resolve it using the same principle.\n\nBut regarding the \"mode parsing\" I think\nhttps://lore.kernel.org/git/YvQdR3sDqDMCIjIE@coredump.intra.peff.net/ is\ntaking us in the wrong direction, but wasn't comfortable saying so on\nlist, because this is \"exploitable\" in the following way:\n\n\t$ perl -wE 'say unpack \"B*\", \"hello world\"'\n\t0110100001100101011011000110110001101111001000000111011101101111011100100110110001100100\n\nWhich, combined with Jeff's series on-list you can try e.g.:\n\t\n\tdiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n\tindex ac4099ca893..d435dfd64c5 100755\n\t--- a/t/t5504-fetch-receive-strict.sh\n\t+++ b/t/t5504-fetch-receive-strict.sh\n\t@@ -357,16 +357,16 @@ test_expect_success 'badFilemode is not a strict error' '\n\t \ttree=$(\n\t \t\tcd badmode.git &&\n\t \t\tblob=$(echo blob | git hash-object -w --stdin | hex2oct) &&\n\t-\t\tprintf \"123456 foo\\0${blob}\" |\n\t+\t\tprintf \"106640110100001100101011011000110110001101111001000000111011101101111011100100110110001100100664 foo\\0${blob}\" |\n\t \t\tgit hash-object -t tree --stdin -w --literally\n\t \t) &&\n\t \n\t \trm -rf dst.git &&\n\t \tgit init --bare dst.git &&\n\t \tgit -C dst.git config transfer.fsckObjects true &&\n\t-\n\t-\tgit -C badmode.git push ../dst.git $tree:refs/tags/tree 2>err &&\n\t-\tgrep \"$tree: badFilemode\" err\n\t+\tgit -C badmode.git push ../dst.git $tree:refs/tags/tree &&\n\t+\tgit -C badmode.git fsck &&\n\t+\tgit -C dst.git fsck\n\t '\n\t \n\t test_done\n\tdiff --git a/tree-walk.c b/tree-walk.c\n\tindex 74f4d710e8f..2fb9f2e6cbe 100644\n\t--- a/tree-walk.c\n\t+++ b/tree-walk.c\n\t@@ -15,6 +15,7 @@ static const char *get_mode(const char *str, unsigned int *modep)\n\t \t\treturn NULL;\n\t \n\t \twhile ((c = *str++) != ' ') {\n\t+\t\tfprintf(stderr, \"%c\\n\", c);\n\t \t\tif (c < '0' || c > '7')\n\t \t\t\treturn NULL;\n\t \t\tmode = (mode << 3) + (c - '0');\n\nWhich gets you all tests passing, i.e. the particulars of the mode check\nare such that we'll accept a very long mode (but if you play with it\nyou'll find it's not infinite, we'll run into other buffers elsewhere\npretty soon).\n\nThis is all assuming you're on a system whose \"unsigned int\" is 64 bit.\n\nThis part of it is also something I discovered in the beginning of 2021\n(and there's some old off-list thread between myself & Elijah on the\ntopic I could dig up), but didn't report here at the time.\n\nSo, we could just \"fix\" it, and re the \"wrong direction\" above\ndowngrading the \"bad mode\" check to a mere warning is going a bit too\nfar given the above. We have some odd modes in the wild, but we don't\nhave such \"overflowing modes\".\n\nOn the other hand this edge case is also a golden opportunity we're not\nlikely to ever have again. We can't really change the git object format\nat this point without *major* headaches, but in this case we have the\nability to encode arbitrary data into tree entries (e.g file metadata)\nas long as the writer makes sure they overflow back to the valid\nfilemode :)\n\nAs hacky as that is I think it would be regrettable to forever close the\ndoor on that to fix a hypothetical security bug, hypothetical because no\nversion of git.git has an issue with it. But it is a *potential* edge\ncase in overflowing any other tree parsing code out there in the wild\n(which might be otherwise guarded by a stricter fsck check of ours).\n\nEND QUOTE\n"},{"id":"470996","messageId":"b4b48877-5b80-e96f-d09f-2fe275f42950@web.de","threadId":"59130","inReplyTo":"230123.86a629tzgc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-24T18:53:56Z","receivedAt":"2023-01-24T19:08:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.01.23 um 09:33 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Jan 21 2023, René Scharfe wrote:\n>\n>> When parsing tree entries, reject mode values that don't fit into an\n>> unsigned int.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  tree-walk.c | 2 ++\n>>  1 file changed, 2 insertions(+)\n>>\n>> diff --git a/tree-walk.c b/tree-walk.c\n>> index 74f4d710e8..5e7bc38600 100644\n>> --- a/tree-walk.c\n>> +++ b/tree-walk.c\n>> @@ -17,6 +17,8 @@ static const char *get_mode(const char *str, unsigned int *modep)\n>>  \twhile ((c = *str++) != ' ') {\n>>  \t\tif (c < '0' || c > '7')\n>>  \t\t\treturn NULL;\n>> +\t\tif ((mode << 3) >> 3 != mode)\n>> +\t\t\treturn NULL;\n>>  \t\tmode = (mode << 3) + (c - '0');\n>>  \t}\n>>  \t*modep = mode;\n>\n> There was a discussion about this on git-security last August, in a\n> report that turned out to be uninteresting for the security aspects.\n>\n> I'll just quote my own reply here out of context\n> (<220811.86mtcbqd5x.gmgdl@evledraar.gmail.com> for those with access).\n\n> On the other hand this edge case is also a golden opportunity we're not\n> likely to ever have again. We can't really change the git object format\n> at this point without *major* headaches, but in this case we have the\n> ability to encode arbitrary data into tree entries (e.g file metadata)\n> as long as the writer makes sure they overflow back to the valid\n> filemode :)\n\nPatch v1 cited above still keeps bits beyond S_IFMT (0xF000), so that's\nat 16 bits on a platform with 32-bit unsigned int for future object\ntypes or other metadata.\n\nOne bit would suffice to switch the path field into an URL and encode\nadditional metadata there.  We could do that even with the stricter\npatch v2 by using one of the bits between S_IFMT and normal permissions\n(0777) for that, e.g. the sticky bit.\n\nThat all said, the longer I think about mode overflow the less I\nunderstand why I sent these patches.  It's basically harmless.  Perhaps\nwe just need a comment stating that, to contain the urge to \"fix\" this.\nAnyway, I'd like to retract my overflowing modes patches (unless someone\nelse really, really wants one of them applied).\n\nRené\n"},{"id":"471001","messageId":"xmqqmt67fysf.fsf@gitster.g","threadId":"59130","inReplyTo":"b4b48877-5b80-e96f-d09f-2fe275f42950@web.de","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-24T20:44:16Z","receivedAt":"2023-01-24T20:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> ...  It's basically harmless.  Perhaps\n> we just need a comment stating that, to contain the urge to \"fix\" this.\n\nYeah, especially since fsck finds and warns about bad mode with\nFSCK_MSG_BAD_FILEMODE and also FSCK_MSG_ZERO_PADDED_FILEMODE in a\nseparate codepath, adding another piece of code that checks a\nslightly different condition does not sound like a good idea.\n\nThanks.\n"},{"id":"471064","messageId":"Y9JlouutFD21lvna@coredump.intra.peff.net","threadId":"59130","inReplyTo":"xmqqmt67fysf.fsf@gitster.g","subject":"Re: [PATCH] tree-walk: disallow overflowing modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-26T11:36:02Z","receivedAt":"2023-01-26T11:36:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 24, 2023 at 12:44:16PM -0800, Junio C Hamano wrote:\n\n> René Scharfe <l.s.r@web.de> writes:\n> \n> > ...  It's basically harmless.  Perhaps\n> > we just need a comment stating that, to contain the urge to \"fix\" this.\n> \n> Yeah, especially since fsck finds and warns about bad mode with\n> FSCK_MSG_BAD_FILEMODE and also FSCK_MSG_ZERO_PADDED_FILEMODE in a\n> separate codepath, adding another piece of code that checks a\n> slightly different condition does not sound like a good idea.\n\nYeah, I'm happy to drop this whole thing. I do think it would be\nreasonable for fsck to check for overflow alongside BAD_FILEMODE, etc,\nbut it's annoying to do since we are relying on the existing parser.\n\nI actually have a suspicion that it might be reasonable for fsck to just\nparse the trees itself, rather than relying on decode_tree_entry(), etc.\nBut that's a much bigger topic (and a possible maintenance burden) for\nquestionable gain, so unless we find some compelling reason (some case\nthat we really want to detect but which we want the regular decoder to\nignore), it's probably not worth exploring.\n\n-Peff\n"}]}