{"thread":{"id":"60850","subject":"[PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","startedAt":"2024-02-05T17:21:49Z","lastAt":"2024-02-08T21:44:51Z","messageCount":13,"participants":["Chandra Pratap via GitGitGadget","René Scharfe","Kyle Lippincott","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487940","messageId":"pull.1652.git.1707153705840.gitgitgadget@gmail.com","threadId":"60850","inReplyTo":null,"subject":"[PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-05T17:21:45Z","receivedAt":"2024-02-05T17:21:49Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nSigned-off-by: Chandra Pratap \n---\n commit.c: ensure strchrnul() doesn't scan beyond range\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1652\n\n commit.c | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex ef679a0b939..a65b8e92e94 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1743,15 +1743,9 @@ const char *find_header_mem(const char *msg, size_t len,\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\t/*\n-\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n-\t * given by len. However, current callers are safe because they compute\n-\t * len by scanning a NUL-terminated block of memory starting at msg.\n-\t * Nonetheless, it would be better to ensure the function does not look\n-\t * at msg beyond the len provided by the caller.\n-\t */\n \twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n+\t\tassert(eol - line <= len);\n \n \t\tif (line == eol)\n \t\t\treturn NULL;\n\nbase-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n-- \ngitgitgadget\n"},{"id":"487948","messageId":"ce83bd09-dbd2-4c9e-8197-6e4800935523@web.de","threadId":"60850","inReplyTo":"pull.1652.git.1707153705840.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-05T19:57:46Z","receivedAt":"2024-02-05T19:57:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.02.24 um 18:21 schrieb Chandra Pratap via GitGitGadget:\n> From: Chandra Pratap \n>\n> Signed-off-by: Chandra Pratap \n> ---\n> commit.c: ensure strchrnul() doesn't scan beyond range\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1652\n>\n> commit.c | 8 +-------\n> 1 file changed, 1 insertion(+), 7 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index ef679a0b939..a65b8e92e94 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1743,15 +1743,9 @@ const char *find_header_mem(const char *msg, size_t len,\n> \tint key_len = strlen(key);\n> \tconst char *line = msg;\n>\n> -\t/*\n> -\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> -\t * given by len. However, current callers are safe because they compute\n> -\t * len by scanning a NUL-terminated block of memory starting at msg.\n> -\t * Nonetheless, it would be better to ensure the function does not look\n> -\t * at msg beyond the len provided by the caller.\n> -\t */\n> \twhile (line && line < msg + len) {\n> \t\tconst char *eol = strchrnul(line, '\\n');\n> +\t\tassert(eol - line <= len);\n\nSomething like this might work in Verse, but C is more simple-minded.\nYou can't undo an out-of-bounds access after the fact, and assert()\nwould be compiled out if the code is built with NDEBUG anyway.\n\nIf you want to make the code work with buffers that lack a terminating\nNUL then you need to replace the strchrnul() call with something that\nrespects buffer lengths. You could e.g. call memchr(). Don't forget\nto check for NUL to preserve the original behavior. Or you could roll\nyour own custom replacement, perhaps like this:\n\nchar *strnchrnul(const char *s, int c, size_t len)\n{\n\twhile (len-- && *s && *s != c)\n\t\ts++;\n\treturn (char *)s;\n}\n\nA test with the new unit-test framework would be nice. It should be\npossible to show that the current code runs over the passed len,\nwithout causing undefined behavior. E.g. find_header_mem(\"foo bar\",\n2, \"foo\", &len) is safe, but returns \"bar\" instead of NULL.\n\n>\n> \t\tif (line == eol)\n> \t\t\treturn NULL;\n>\n> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n\n"},{"id":"487976","messageId":"CAO_smVg8EepRvpDwpEiq8pVJvX08GSTX=WUpzsOVQwm46JbTcA@mail.gmail.com","threadId":"60850","inReplyTo":"pull.1652.git.1707153705840.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-02-06T01:41:49Z","receivedAt":"2024-02-06T01:42:06Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Feb 5, 2024 at 9:23 AM Chandra Pratap via GitGitGadget\n wrote:\n>\n> From: Chandra Pratap \n>\n> Signed-off-by: Chandra Pratap \n> ---\n> commit.c: ensure strchrnul() doesn't scan beyond range\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1652\n>\n> commit.c | 8 +-------\n> 1 file changed, 1 insertion(+), 7 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index ef679a0b939..a65b8e92e94 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1743,15 +1743,9 @@ const char *find_header_mem(const char *msg, size_t len,\n> int key_len = strlen(key);\n> const char *line = msg;\n>\n> - /*\n> - * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> - * given by len. However, current callers are safe because they compute\n> - * len by scanning a NUL-terminated block of memory starting at msg.\n> - * Nonetheless, it would be better to ensure the function does not look\n> - * at msg beyond the len provided by the caller.\n> - */\n> while (line && line < msg + len) {\n> const char *eol = strchrnul(line, '\\n');\n> + assert(eol - line <= len);\n\nI don't think this is sufficient to address the NEEDSWORK. `assert` is\nonly active in debug builds, and strchrnul would have already\npotentially exceeded the bounds of its memory by the time this check\nis happening. We'd need a safe version of strchrnul that took the\nmaximum length and never exceeded it.\n\n>\n> if (line == eol)\n> return NULL;\n>\n> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n> --\n> gitgitgadget\n>\n"},{"id":"488085","messageId":"xmqqwmrhh1q2.fsf@gitster.g","threadId":"60850","inReplyTo":"ce83bd09-dbd2-4c9e-8197-6e4800935523@web.de","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T18:44:21Z","receivedAt":"2024-02-06T18:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> \twhile (line && line < msg + len) {\n>> \t\tconst char *eol = strchrnul(line, '\\n');\n>> +\t\tassert(eol - line <= len);\n>\n> Something like this might work in Verse, but C is more simple-minded.\n> You can't undo an out-of-bounds access after the fact, and assert()\n> would be compiled out if the code is built with NDEBUG anyway.\n\nGood comments. Thanks.\n"},{"id":"488135","messageId":"pull.1652.v2.git.1707314246530.gitgitgadget@gmail.com","threadId":"60850","inReplyTo":"pull.1652.git.1707153705840.gitgitgadget@gmail.com","subject":"[PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T13:57:26Z","receivedAt":"2024-02-07T13:57:30Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nSigned-off-by: Chandra Pratap \n---\n commit.c: ensure find_header_mem() doesn't scan beyond given range\n \n Thanks for the feedback, Kyle and René! I have update the patch to\n actually solve the problem at hand but I am not very sure about the\n resulting dropping of const-ness of 'eol' from this and how big of a\n problem it might create (if any). I wonder if a custom strchrnul() is\n the best solution to this after all.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1652\n\nRange-diff vs v1:\n\n 1: 1c62f6ee353 ! 1: dcb2de3faea commit.c: ensure strchrnul() doesn't scan beyond range\n @@ Metadata\n Author: Chandra Pratap \n \n ## Commit message ##\n - commit.c: ensure strchrnul() doesn't scan beyond range\n + commit.c: ensure find_header_mem() doesn't scan beyond given range\n \n Signed-off-by: Chandra Pratap \n \n @@ commit.c: const char *find_header_mem(const char *msg, size_t len,\n -\t * at msg beyond the len provided by the caller.\n -\t */\n \twhile (line && line < msg + len) {\n - \t\tconst char *eol = strchrnul(line, '\\n');\n -+\t\tassert(eol - line <= len);\n +-\t\tconst char *eol = strchrnul(line, '\\n');\n ++\t\tchar *eol = (char *) line;\n ++\t\tfor (size_t i = 0; i < len && *eol && *eol != '\\n'; i++) {\n ++\t\t\teol++;\n ++\t\t}\n \n \t\tif (line == eol)\n \t\t\treturn NULL;\n\n\n commit.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex ef679a0b939..9a460b2fd6f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1743,15 +1743,11 @@ const char *find_header_mem(const char *msg, size_t len,\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\t/*\n-\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n-\t * given by len. However, current callers are safe because they compute\n-\t * len by scanning a NUL-terminated block of memory starting at msg.\n-\t * Nonetheless, it would be better to ensure the function does not look\n-\t * at msg beyond the len provided by the caller.\n-\t */\n \twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n+\t\tchar *eol = (char *) line;\n+\t\tfor (size_t i = 0; i < len && *eol && *eol != '\\n'; i++) {\n+\t\t\teol++;\n+\t\t}\n \n \t\tif (line == eol)\n \t\t\treturn NULL;\n\nbase-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n-- \ngitgitgadget\n"},{"id":"488162","messageId":"e7b269ea-6a10-4f3c-ae97-a58eb7ccc6ef@web.de","threadId":"60850","inReplyTo":"pull.1652.v2.git.1707314246530.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-07T17:09:54Z","receivedAt":"2024-02-07T17:09:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.02.24 um 14:57 schrieb Chandra Pratap via GitGitGadget:\n> From: Chandra Pratap \n>\n> Signed-off-by: Chandra Pratap \n> ---\n> commit.c: ensure find_header_mem() doesn't scan beyond given range\n>\n> Thanks for the feedback, Kyle and René! I have update the patch to\n> actually solve the problem at hand but I am not very sure about the\n> resulting dropping of const-ness of 'eol' from this and how big of a\n> problem it might create (if any). I wonder if a custom strchrnul() is\n> the best solution to this after all.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1652\n>\n> Range-diff vs v1:\n>\n> 1: 1c62f6ee353 ! 1: dcb2de3faea commit.c: ensure strchrnul() doesn't scan beyond range\n> @@ Metadata\n> Author: Chandra Pratap \n>\n> ## Commit message ##\n> - commit.c: ensure strchrnul() doesn't scan beyond range\n> + commit.c: ensure find_header_mem() doesn't scan beyond given range\n>\n> Signed-off-by: Chandra Pratap \n>\n> @@ commit.c: const char *find_header_mem(const char *msg, size_t len,\n> -\t * at msg beyond the len provided by the caller.\n> -\t */\n> \twhile (line && line < msg + len) {\n> - \t\tconst char *eol = strchrnul(line, '\\n');\n> -+\t\tassert(eol - line <= len);\n> +-\t\tconst char *eol = strchrnul(line, '\\n');\n> ++\t\tchar *eol = (char *) line;\n> ++\t\tfor (size_t i = 0; i < len && *eol && *eol != '\\n'; i++) {\n> ++\t\t\teol++;\n> ++\t\t}\n>\n> \t\tif (line == eol)\n> \t\t\treturn NULL;\n>\n>\n> commit.c | 12 ++++--------\n> 1 file changed, 4 insertions(+), 8 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index ef679a0b939..9a460b2fd6f 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1743,15 +1743,11 @@ const char *find_header_mem(const char *msg, size_t len,\n> \tint key_len = strlen(key);\n> \tconst char *line = msg;\n>\n> -\t/*\n> -\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> -\t * given by len. However, current callers are safe because they compute\n> -\t * len by scanning a NUL-terminated block of memory starting at msg.\n> -\t * Nonetheless, it would be better to ensure the function does not look\n> -\t * at msg beyond the len provided by the caller.\n> -\t */\n> \twhile (line && line < msg + len) {\n> -\t\tconst char *eol = strchrnul(line, '\\n');\n> +\t\tchar *eol = (char *) line;\n> +\t\tfor (size_t i = 0; i < len && *eol && *eol != '\\n'; i++) {\n> +\t\t\teol++;\n> +\t\t}\n\nThis uses the pointer eol only for reading, so you can keep it const.\n\nThe loop starts counting from 0 to len for each line, which cannot be\nright. find_header_mem(\"headers\\nfoo bar\", 9, \"foo\", &len) would still\nreturn \"bar\" instead of NULL.\n\nYou could initialize i to the offset of line within msg instead (i.e.\ni = line - msg). Or check eol < msg + len instead of i < len -- then\nyou don't even need to introduce that separate counter.\n\nStyle nit: We tend to omit curly braces if they contain only a single\nstatement.\n\n> \t\tif (line == eol)\n> \t\t\treturn NULL;\n>\n> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n"},{"id":"488164","messageId":"xmqqr0ho9oi9.fsf@gitster.g","threadId":"60850","inReplyTo":"e7b269ea-6a10-4f3c-ae97-a58eb7ccc6ef@web.de","subject":"Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T17:23:58Z","receivedAt":"2024-02-07T17:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> -\t/*\n>> -\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n>> -\t * given by len. However, current callers are safe because they compute\n>> -\t * len by scanning a NUL-terminated block of memory starting at msg.\n>> -\t * Nonetheless, it would be better to ensure the function does not look\n>> -\t * at msg beyond the len provided by the caller.\n>> -\t */\n>> \twhile (line && line < msg + len) {\n>> -\t\tconst char *eol = strchrnul(line, '\\n');\n>> +\t\tchar *eol = (char *) line;\n>> +\t\tfor (size_t i = 0; i < len && *eol && *eol != '\\n'; i++) {\n>> +\t\t\teol++;\n>> +\t\t}\n>\n> This uses the pointer eol only for reading, so you can keep it const.\n>\n> The loop starts counting from 0 to len for each line, which cannot be\n> right. find_header_mem(\"headers\\nfoo bar\", 9, \"foo\", &len) would still\n> return \"bar\" instead of NULL.\n>\n> You could initialize i to the offset of line within msg instead (i.e.\n> i = line - msg). Or check eol < msg + len instead of i < len -- then\n> you don't even need to introduce that separate counter.\n>\n> Style nit: We tend to omit curly braces if they contain only a single\n> statement.\n\nAll true. As we already use an extra variable 'i' for counting, we\ncan do without eol and reference line[i] instead, which would make\nthe whole thing something like\n\n\twhile (line && line < msg + len) {\n\t\tsize_t i;\n\t\tfor (i = 0;\n i < len && line[i] && line[i] != '\\n';\n\t\t i++)\n\t\t\t;\n\t\tif (key_len < i &&\n\t\t !strncmp(line, key, ken_len) &&\n\t\t linhe[key_len] == ' ') {\n\t\t\t*out_len = i - key_len - 1;\n\t\t\treturn line + key_len + 1;\n\t\t}\n line = line[i] ? line + i + 1 : NULL;\n\t}\n\nwhich is not too bad, simply because the original already needed to\nknow the length of the current line and due to lack of this \"i\" you\nintroduced, it used \"eol-line\" instead. Now you have \"i\", the code\nmay get even simpler by getting rid of \"eol\".\n\n"},{"id":"488201","messageId":"20240208010040.GB1059751@coredump.intra.peff.net","threadId":"60850","inReplyTo":"ce83bd09-dbd2-4c9e-8197-6e4800935523@web.de","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-08T01:00:40Z","receivedAt":"2024-02-08T01:00:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 05, 2024 at 08:57:46PM +0100, René Scharfe wrote:\n\n> If you want to make the code work with buffers that lack a terminating\n> NUL then you need to replace the strchrnul() call with something that\n> respects buffer lengths. You could e.g. call memchr(). Don't forget\n> to check for NUL to preserve the original behavior. Or you could roll\n> your own custom replacement, perhaps like this:\n\nI'm not sure it is worth retaining the check for NUL. The original\nfunction added by me in fe6eb7f2c5 (commit: provide a function to find a\nheader in a buffer, 2014-08-27) just took a NUL-terminated string, so\nwe certainly were not expecting embedded NULs.\n\nIn cfc5cf428b (receive-pack.c: consolidate find header logic,\n2022-01-06) we switched to taking the \"len\" parameter, but the new\ncaller just passes strlen(msg) anyway.\n\nI guess you could argue that before that commit, receive-pack.c's\nfind_header() which took a length was buggy to use strchrnul(). It gets\nfed with a push-cert buffer. I guess it's possible for there to be an\nembedded NUL there, but in practice there shouldn't be. If we are\nthinking of malformed or malicious input, it's not clear which behavior\n(finding or not finding a header past a NUL) is more harmful. So all\nthings being equal, I would try to reduce the number of special cases\nhere by not worrying about NULs.\n\n(Though if somebody really wants to dig, it's possible there's a clever\ndual-parser attack here where \"\\nfoo\\0bar baz\" finds the header \"bar\nbaz\" in one parser but not in another).\n\n-Peff\n"},{"id":"488248","messageId":"8313d9d6-f6bd-4fae-be9c-e7a8129768eb@web.de","threadId":"60850","inReplyTo":"20240208010040.GB1059751@coredump.intra.peff.net","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-08T18:31:51Z","receivedAt":"2024-02-08T18:32:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.02.24 um 02:00 schrieb Jeff King:\n> On Mon, Feb 05, 2024 at 08:57:46PM +0100, René Scharfe wrote:\n>\n>> If you want to make the code work with buffers that lack a terminating\n>> NUL then you need to replace the strchrnul() call with something that\n>> respects buffer lengths. You could e.g. call memchr(). Don't forget\n>> to check for NUL to preserve the original behavior. Or you could roll\n>> your own custom replacement, perhaps like this:\n>\n> I'm not sure it is worth retaining the check for NUL. The original\n> function added by me in fe6eb7f2c5 (commit: provide a function to find a\n> header in a buffer, 2014-08-27) just took a NUL-terminated string, so\n> we certainly were not expecting embedded NULs.\n>\n> In cfc5cf428b (receive-pack.c: consolidate find header logic,\n> 2022-01-06) we switched to taking the \"len\" parameter, but the new\n> caller just passes strlen(msg) anyway.\n>\n> I guess you could argue that before that commit, receive-pack.c's\n> find_header() which took a length was buggy to use strchrnul(). It gets\n> fed with a push-cert buffer. I guess it's possible for there to be an\n> embedded NUL there, but in practice there shouldn't be. If we are\n> thinking of malformed or malicious input, it's not clear which behavior\n> (finding or not finding a header past a NUL) is more harmful. So all\n> things being equal, I would try to reduce the number of special cases\n> here by not worrying about NULs.\n>\n> (Though if somebody really wants to dig, it's possible there's a clever\n> dual-parser attack here where \"\\nfoo\\0bar baz\" finds the header \"bar\n> baz\" in one parser but not in another).\n\nGood point. A _mem function shouldn't worry about NULs. Its callers\nare responsible for that -- if necessary.\n\nNo idea what an attacker could do with nonce and push-option headers\nwith varying visibility. Version detection? Something worse?\n\nBut anyway: If NULs are of no concern and we currently end parsing when\nwe see one in all cases, why do we need a _mem function at all? The\noriginal version of the function, find_commit_header(), should suffice.\ncheck_nonce() could be run against the NUL-terminated sigcheck.payload\nand check_cert_push_options() parses an entire strbuf, so there is no\nrisk of out-of-bounds access.\n\nRené\n"},{"id":"488250","messageId":"xmqqil2yn3ey.fsf@gitster.g","threadId":"60850","inReplyTo":"8313d9d6-f6bd-4fae-be9c-e7a8129768eb@web.de","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-08T19:48:05Z","receivedAt":"2024-02-08T19:48:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> But anyway: If NULs are of no concern and we currently end parsing when\n> we see one in all cases, why do we need a _mem function at all? The\n> original version of the function, find_commit_header(), should suffice.\n> check_nonce() could be run against the NUL-terminated sigcheck.payload\n> and check_cert_push_options() parses an entire strbuf, so there is no\n> risk of out-of-bounds access.\n\nIf I recall correctly, the caller that does not pass strlen() as the\npayload length gives a length that is shorter than the buffer, i.e.\n\"stop the parsing here, do not get confused into thinking the\ngarbage after this point contains useful payload\" was the reason why\nwe have a separate \"len\".\n"},{"id":"488251","messageId":"CAO_smVhsKHu0QrvpFbofd7y-Exhnk7=JUzffECNZQx=MWzmnsw@mail.gmail.com","threadId":"60850","inReplyTo":"xmqqil2yn3ey.fsf@gitster.g","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-02-08T19:52:35Z","receivedAt":"2024-02-08T19:52:52Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Thu, Feb 8, 2024 at 11:48 AM Junio C Hamano wrote:\n>\n> René Scharfe writes:\n>\n> > But anyway: If NULs are of no concern and we currently end parsing when\n> > we see one in all cases, why do we need a _mem function at all? The\n> > original version of the function, find_commit_header(), should suffice.\n> > check_nonce() could be run against the NUL-terminated sigcheck.payload\n> > and check_cert_push_options() parses an entire strbuf, so there is no\n> > risk of out-of-bounds access.\n>\n> If I recall correctly, the caller that does not pass strlen() as the\n> payload length gives a length that is shorter than the buffer, i.e.\n> \"stop the parsing here, do not get confused into thinking the\n> garbage after this point contains useful payload\" was the reason why\n> we have a separate \"len\".\n>\n\nI just rediscovered that. I think this probably should be something\nthat caller (check_nonce) implements, then. Having a _mem function\nimplies to me (though I'm very new to this codebase) that it supports\nembedded NULs, but that's not what's happening here.\n"},{"id":"488257","messageId":"20240208214137.GB1090198@coredump.intra.peff.net","threadId":"60850","inReplyTo":"xmqqil2yn3ey.fsf@gitster.g","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-08T21:41:37Z","receivedAt":"2024-02-08T21:41:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 08, 2024 at 11:48:05AM -0800, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> > But anyway: If NULs are of no concern and we currently end parsing when\n> > we see one in all cases, why do we need a _mem function at all? The\n> > original version of the function, find_commit_header(), should suffice.\n> > check_nonce() could be run against the NUL-terminated sigcheck.payload\n> > and check_cert_push_options() parses an entire strbuf, so there is no\n> > risk of out-of-bounds access.\n> \n> If I recall correctly, the caller that does not pass strlen() as the\n> payload length gives a length that is shorter than the buffer, i.e.\n> \"stop the parsing here, do not get confused into thinking the\n> garbage after this point contains useful payload\" was the reason why\n> we have a separate \"len\".\n\nYes, check_nonce() passes in a length limited by the start of the actual\nsignature, as determined by parse_signed_buffer(). Though that generally\ncomes after a blank line, which would also stop find_header() from\nparsing further.\n\nBut more interestingly: even though we pass a buf/len pair to\nparse_signed_buffer(), it then calls get_format_by_sig() which takes\nonly a NUL-terminated string. So:\n\n 1. It is not possible for the buf/len pair we pass to check_nonce() to\n contain a NUL. And thus there is no caller of find_header_mem()\n that can contain an embedded NUL. So switching from strchrnul() to\n just memchr() should be OK there.\n\n 2. That raises the question of whether parse_signed_buffer() has a\n similar walk-too-far problem. ;) The answer is no, because we feed\n it from a strbuf. But it's not a great pattern overall.\n\n-Peff\n"},{"id":"488258","messageId":"xmqqcyt6my0f.fsf@gitster.g","threadId":"60850","inReplyTo":"20240208214137.GB1090198@coredump.intra.peff.net","subject":"Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-08T21:44:48Z","receivedAt":"2024-02-08T21:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 1. It is not possible for the buf/len pair we pass to check_nonce() to\n> contain a NUL. And thus there is no caller of find_header_mem()\n> that can contain an embedded NUL. So switching from strchrnul() to\n> just memchr() should be OK there.\n\nCorrect.\n\n> 2. That raises the question of whether parse_signed_buffer() has a\n> similar walk-too-far problem. ;) The answer is no, because we feed\n> it from a strbuf. But it's not a great pattern overall.\n\nTrue, too.\n\nThanks.\n"}]}