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 <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/attr.c b/attr.c
index 41657479fffbe2a7edd45614093ddc19d4878034..38ecd2fff30d33a86ec11f16f454a0a504a15fa2 100644 (file)
--- a/attr.c
+++ b/attr.c
@@ -344,6 +344,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
                return NULL;
        name = cp;
 
+       if (strlen(line) >= ATTR_MAX_LINE_LENGTH) {
+               warning(_("ignoring overly long attributes line %d"), lineno);
+               return NULL;
+       }
+
        if (*cp == '"' && !unquote_c_style(&pattern, name, &states)) {
                name = pattern.buf;
                namelen = pattern.len;

diff --git a/attr.h b/attr.h
index 404548f028a8b8fa4e8836ef69867475810a0e15..df9a75da550eb7f0fcd627dfee2e7e5ed098ae70 100644 (file)
--- a/attr.h
+++ b/attr.h
@@ -107,6 +107,12 @@
  * - Free the `attr_check` struct by calling `attr_check_free()`.
  */
 
+/**
+ * The maximum line length for a gitattributes file. If the line exceeds this
+ * length we will ignore it.
+ */
+#define ATTR_MAX_LINE_LENGTH 2048
+
 struct index_state;
 
 /**

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 416386ce2f8924dc6cff9bbeba8b6063fac8251b..7d68e6a56e964dda5f59b4da790b414a97611a0d 100755 (executable)
--- 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
 '