From d74b1fd54fdbc45966d12ea907dece11e072fb2b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 1 Dec 2022 15:45:44 +0100 Subject: attr: fix silently splitting up lines longer than 2048 bytes When reading attributes from a file we use fgets(3P) with a buffer size of 2048 bytes. This means that as soon as a line exceeds the buffer size we split it up into multiple parts and parse each of them as a separate pattern line. This is of course not what the user intended, and even worse the behaviour is inconsistent with how we read attributes from the index. Fix this bug by converting the code to use `strbuf_getline()` instead. This will indeed read in the whole line, which may theoretically lead to an out-of-memory situation when the gitattributes file is huge. We're about to reject any gitattributes files larger than 100MB in the next commit though, which makes this less of a concern. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t0003-attributes.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 't') diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index b660593c20..416386ce2f 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -339,4 +339,25 @@ test_expect_success 'query binary macro directly' ' test_cmp expect actual ' +test_expect_success 'large attributes line ignores trailing content in tree' ' + test_when_finished "rm .gitattributes" && + # older versions of Git broke lines at 2048 bytes; the 2045 bytes + # of 0-padding here is accounting for the three bytes of "a 1", which + # would knock "trailing" to the "next" line, where it would be + # erroneously parsed. + printf "a %02045dtrailing attribute\n" 1 >.gitattributes && + git check-attr --all trailing >actual 2>err && + test_must_be_empty err && + test_must_be_empty actual +' + +test_expect_success 'large attributes line ignores trailing content in index' ' + test_when_finished "git update-index --remove .gitattributes" && + blob=$(printf "a %02045dtrailing attribute\n" 1 | git hash-object -w --stdin) && + git update-index --add --cacheinfo 100644,$blob,.gitattributes && + git check-attr --cached --all trailing >actual 2>err && + test_must_be_empty err && + test_must_be_empty actual +' + test_done -- cgit v1.2.1 From dfa6b32b5e599d97448337ed4fc18dd50c90758f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 1 Dec 2022 15:45:48 +0100 Subject: attr: ignore attribute lines exceeding 2048 bytes There are two different code paths to read gitattributes: once via a file, and once via the index. These two paths used to behave differently because when reading attributes from a file, we used fgets(3P) with a buffer size of 2kB. Consequentially, we silently truncate line lengths when lines are longer than that and will then parse the remainder of the line as a new pattern. It goes without saying that this is entirely unexpected, but it's even worse that the behaviour depends on how the gitattributes are parsed. While this is simply wrong, the silent truncation saves us with the recently discovered vulnerabilities that can cause out-of-bound writes or reads with unreasonably long lines due to integer overflows. As the common path is to read gitattributes via the worktree file instead of via the index, we can assume that any gitattributes file that had lines longer than that is already broken anyway. So instead of lifting the limit here, we can double down on it to fix the vulnerabilities. Introduce an explicit line length limit of 2kB that is shared across all paths that read attributes and ignore any line that hits this limit while printing a warning. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t0003-attributes.sh | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) (limited to 't') diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 416386ce2f..7d68e6a56e 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -339,6 +339,15 @@ test_expect_success 'query binary macro directly' ' test_cmp expect actual ' +test_expect_success 'large attributes line ignored in tree' ' + test_when_finished "rm .gitattributes" && + printf "path %02043d" 1 >.gitattributes && + git check-attr --all path >actual 2>err && + echo "warning: ignoring overly long attributes line 1" >expect && + test_cmp expect err && + test_must_be_empty actual +' + test_expect_success 'large attributes line ignores trailing content in tree' ' test_when_finished "rm .gitattributes" && # older versions of Git broke lines at 2048 bytes; the 2045 bytes @@ -347,7 +356,18 @@ test_expect_success 'large attributes line ignores trailing content in tree' ' # erroneously parsed. printf "a %02045dtrailing attribute\n" 1 >.gitattributes && git check-attr --all trailing >actual 2>err && - test_must_be_empty err && + echo "warning: ignoring overly long attributes line 1" >expect && + test_cmp expect err && + test_must_be_empty actual +' + +test_expect_success 'large attributes line ignored in index' ' + test_when_finished "git update-index --remove .gitattributes" && + blob=$(printf "path %02043d" 1 | git hash-object -w --stdin) && + git update-index --add --cacheinfo 100644,$blob,.gitattributes && + git check-attr --cached --all path >actual 2>err && + echo "warning: ignoring overly long attributes line 1" >expect && + test_cmp expect err && test_must_be_empty actual ' @@ -356,7 +376,8 @@ test_expect_success 'large attributes line ignores trailing content in index' ' blob=$(printf "a %02045dtrailing attribute\n" 1 | git hash-object -w --stdin) && git update-index --add --cacheinfo 100644,$blob,.gitattributes && git check-attr --cached --all trailing >actual 2>err && - test_must_be_empty err && + echo "warning: ignoring overly long attributes line 1" >expect && + test_cmp expect err && test_must_be_empty actual ' -- cgit v1.2.1 From 3c50032ff5289cc45659f21949c8d09e52164579 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 1 Dec 2022 15:45:53 +0100 Subject: attr: ignore overly large gitattributes files Similar as with the preceding commit, start ignoring gitattributes files that are overly large to protect us against out-of-bounds reads and writes caused by integer overflows. Unfortunately, we cannot just define "overly large" in terms of any preexisting limits in the codebase. Instead, we choose a very conservative limit of 100MB. This is plenty of room for specifying gitattributes, and incidentally it is also the limit for blob sizes for GitHub. While we don't want GitHub to dictate limits here, it is still sensible to use this fact for an informed decision given that it is hosting a huge set of repositories. Furthermore, over at GitLab we scanned a subset of repositories for their root-level attribute files. We found that 80% of them have a gitattributes file smaller than 100kB, 99.99% have one smaller than 1MB, and only a single repository had one that was almost 3MB in size. So enforcing a limit of 100MB seems to give us ample of headroom. With this limit in place we can be reasonably sure that there is no easy way to exploit the gitattributes file via integer overflows anymore. Furthermore, it protects us against resource exhaustion caused by allocating the in-memory data structures required to represent the parsed attributes. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t0003-attributes.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 't') diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 7d68e6a56e..9d9aa2855d 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -361,6 +361,14 @@ test_expect_success 'large attributes line ignores trailing content in tree' ' test_must_be_empty actual ' +test_expect_success EXPENSIVE 'large attributes file ignored in tree' ' + test_when_finished "rm .gitattributes" && + dd if=/dev/zero of=.gitattributes bs=101M count=1 2>/dev/null && + git check-attr --all path >/dev/null 2>err && + echo "warning: ignoring overly large gitattributes file ${SQ}.gitattributes${SQ}" >expect && + test_cmp expect err +' + test_expect_success 'large attributes line ignored in index' ' test_when_finished "git update-index --remove .gitattributes" && blob=$(printf "path %02043d" 1 | git hash-object -w --stdin) && @@ -381,4 +389,13 @@ test_expect_success 'large attributes line ignores trailing content in index' ' test_must_be_empty actual ' +test_expect_success EXPENSIVE 'large attributes file ignored in index' ' + test_when_finished "git update-index --remove .gitattributes" && + blob=$(dd if=/dev/zero bs=101M count=1 2>/dev/null | git hash-object -w --stdin) && + git update-index --add --cacheinfo 100644,$blob,.gitattributes && + git check-attr --cached --all path >/dev/null 2>err && + echo "warning: ignoring overly large gitattributes blob ${SQ}.gitattributes${SQ}" >expect && + test_cmp expect err +' + test_done -- cgit v1.2.1