{"thread":{"id":"56814","subject":"[PATCH 1/2] gpg-interface: handle missing \" with \" gracefully in parse_ssh_output()","startedAt":"2021-10-30T17:05:05Z","lastAt":"2021-11-01T18:37:26Z","messageCount":8,"participants":["René Scharfe","Junio C Hamano","Fabian Stelzer"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"440128","messageId":"f6fca7c0-079c-4337-23d9-cd970c79b8ad@web.de","threadId":"56814","inReplyTo":null,"subject":"[PATCH 1/2] gpg-interface: handle missing \" with \" gracefully in parse_ssh_output()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-30T17:04:56Z","receivedAt":"2021-10-30T17:05:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If the output of ssh-keygen starts with \"Good \\\"git\\\" signature for \",\nbut is not followed by \" with \" for some reason, then parse_ssh_output()\nuses -1 as the len parameter of xmemdupz(), which in turn will end the\nprogram. Reject the signature and carry on instead in that case.\n\nSigned-off-by: René Scharfe \n---\nThis code was added after v2.33.0.\nPatch formatted with --inter-hunk-context=2 for easier review.\n\nSilly bonus question: What's the purpose of the \"+ 1\" and \"- 1\", which\nseem to cancel each other out?\n\n gpg-interface.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 800d8caa67..62d340e78a 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc)\n \tline = to_free = xmemdupz(sigc->output, strcspn(sigc->output, \"\\n\"));\n\n \tif (skip_prefix(line, \"Good \\\"git\\\" signature for \", &line)) {\n-\t\t/* Valid signature and known principal */\n-\t\tsigc->result = 'G';\n-\t\tsigc->trust_level = TRUST_FULLY;\n-\n \t\t/* Search for the last \"with\" to get the full principal */\n \t\tprincipal = line;\n \t\tdo {\n \t\t\tsearch = strstr(line, \" with \");\n \t\t\tif (search)\n \t\t\t\tline = search + 1;\n \t\t} while (search != NULL);\n+\t\tif (line == principal)\n+\t\t\tgoto cleanup;\n+\n+\t\t/* Valid signature and known principal */\n+\t\tsigc->result = 'G';\n+\t\tsigc->trust_level = TRUST_FULLY;\n \t\tsigc->signer = xmemdupz(principal, line - principal - 1);\n \t} else if (skip_prefix(line, \"Good \\\"git\\\" signature with \", &line)) {\n \t\t/* Valid signature, but key unknown */\n--\n2.33.1\n\n"},{"id":"440129","messageId":"a72ebd19-9871-f811-cd5c-72b89dad6c6f@web.de","threadId":"56814","inReplyTo":"f6fca7c0-079c-4337-23d9-cd970c79b8ad@web.de","subject":"[PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-30T17:07:38Z","receivedAt":"2021-10-30T17:07:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If the string \"key\" we found in the output of ssh-keygen happens to be\nlocated at the very end of the line, then going four characters further\nleaves us beyond the end of the string. Explicitly search for the\nspace after \"key\" to handle a missing one gracefully.\n\nSigned-off-by: René Scharfe \n---\nThis code was added after v2.33.0.\n\n gpg-interface.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 62d340e78a..3838536f0a 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -409,9 +409,9 @@ static void parse_ssh_output(struct signature_check *sigc)\n \t\tgoto cleanup;\n \t}\n\n-\tkey = strstr(line, \"key\");\n+\tkey = strstr(line, \"key \");\n \tif (key) {\n-\t\tsigc->fingerprint = xstrdup(strstr(line, \"key\") + 4);\n+\t\tsigc->fingerprint = xstrdup(strstr(line, \"key \") + 4);\n \t\tsigc->key = xstrdup(sigc->fingerprint);\n \t} else {\n \t\t/*\n--\n2.33.1\n\n"},{"id":"440211","messageId":"xmqq4k8we7ec.fsf@gitster.g","threadId":"56814","inReplyTo":"a72ebd19-9871-f811-cd5c-72b89dad6c6f@web.de","subject":"Re: [PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-01T06:07:07Z","receivedAt":"2021-11-01T06:07:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> If the string \"key\" we found in the output of ssh-keygen happens to be\n> located at the very end of the line, then going four characters further\n> leaves us beyond the end of the string. Explicitly search for the\n> space after \"key\" to handle a missing one gracefully.\n>\n> Signed-off-by: René Scharfe \n> ---\n> This code was added after v2.33.0.\n\nThanks.\n\nFabian, we are in -rc phase where we concentrate on fixing bugs in\nthe new code in 'master'. A quick ack, \"here is a better way to fix\nit\", or \"no, that won't be needed because...\" is very much\nappreciated.\n\n\n> gpg-interface.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 62d340e78a..3838536f0a 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -409,9 +409,9 @@ static void parse_ssh_output(struct signature_check *sigc)\n> \t\tgoto cleanup;\n> \t}\n>\n> -\tkey = strstr(line, \"key\");\n> +\tkey = strstr(line, \"key \");\n> \tif (key) {\n> -\t\tsigc->fingerprint = xstrdup(strstr(line, \"key\") + 4);\n> +\t\tsigc->fingerprint = xstrdup(strstr(line, \"key \") + 4);\n> \t\tsigc->key = xstrdup(sigc->fingerprint);\n> \t} else {\n> \t\t/*\n> --\n> 2.33.1\n"},{"id":"440212","messageId":"xmqqy268csrx.fsf@gitster.g","threadId":"56814","inReplyTo":"f6fca7c0-079c-4337-23d9-cd970c79b8ad@web.de","subject":"Re: [PATCH 1/2] gpg-interface: handle missing \" with \" gracefully in parse_ssh_output()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-01T06:08:18Z","receivedAt":"2021-11-01T06:08:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> If the output of ssh-keygen starts with \"Good \\\"git\\\" signature for \",\n> but is not followed by \" with \" for some reason, then parse_ssh_output()\n> uses -1 as the len parameter of xmemdupz(), which in turn will end the\n> program. Reject the signature and carry on instead in that case.\n>\n> Signed-off-by: René Scharfe \n> ---\n> This code was added after v2.33.0.\n> Patch formatted with --inter-hunk-context=2 for easier review.\n\nNice spotting.\n\n> Silly bonus question: What's the purpose of the \"+ 1\" and \"- 1\", which\n> seem to cancel each other out?\n\nFabian, we are in -rc phase where we concentrate on fixing bugs in\nthe new code in 'master'. A quick ack, \"here is a better way to fix\nit\", or \"no, that won't be needed because...\" is very much\nappreciated.\n\nThanks.\n\n>\n> gpg-interface.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 800d8caa67..62d340e78a 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc)\n> \tline = to_free = xmemdupz(sigc->output, strcspn(sigc->output, \"\\n\"));\n>\n> \tif (skip_prefix(line, \"Good \\\"git\\\" signature for \", &line)) {\n> -\t\t/* Valid signature and known principal */\n> -\t\tsigc->result = 'G';\n> -\t\tsigc->trust_level = TRUST_FULLY;\n> -\n> \t\t/* Search for the last \"with\" to get the full principal */\n> \t\tprincipal = line;\n> \t\tdo {\n> \t\t\tsearch = strstr(line, \" with \");\n> \t\t\tif (search)\n> \t\t\t\tline = search + 1;\n> \t\t} while (search != NULL);\n> +\t\tif (line == principal)\n> +\t\t\tgoto cleanup;\n> +\n> +\t\t/* Valid signature and known principal */\n> +\t\tsigc->result = 'G';\n> +\t\tsigc->trust_level = TRUST_FULLY;\n> \t\tsigc->signer = xmemdupz(principal, line - principal - 1);\n> \t} else if (skip_prefix(line, \"Good \\\"git\\\" signature with \", &line)) {\n> \t\t/* Valid signature, but key unknown */\n> --\n> 2.33.1\n"},{"id":"440216","messageId":"d758381f-064a-1d0a-afab-cb2160ea2eec@gigacodes.de","threadId":"56814","inReplyTo":"a72ebd19-9871-f811-cd5c-72b89dad6c6f@web.de","subject":"Re: [PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-01T08:33:39Z","receivedAt":"2021-11-01T08:33:47Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 30.10.21 19:07, René Scharfe wrote:\n> If the string \"key\" we found in the output of ssh-keygen happens to be\n> located at the very end of the line, then going four characters further\n> leaves us beyond the end of the string. Explicitly search for the\n> space after \"key\" to handle a missing one gracefully.\n> \n> Signed-off-by: René Scharfe \n> ---\n> This code was added after v2.33.0.\n> \n> gpg-interface.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 62d340e78a..3838536f0a 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -409,9 +409,9 @@ static void parse_ssh_output(struct signature_check *sigc)\n> \t\tgoto cleanup;\n> \t}\n> \n> -\tkey = strstr(line, \"key\");\n> +\tkey = strstr(line, \"key \");\n> \tif (key) {\n> -\t\tsigc->fingerprint = xstrdup(strstr(line, \"key\") + 4);\n> +\t\tsigc->fingerprint = xstrdup(strstr(line, \"key \") + 4);\n> \t\tsigc->key = xstrdup(sigc->fingerprint);\n> \t} else {\n> \t\t/*\n> --\n> 2.33.1\n> \n\nThanks. This is obviously correct.\n"},{"id":"440217","messageId":"dedf3971-adb1-c355-0b05-4e94ad92e1f7@gigacodes.de","threadId":"56814","inReplyTo":"f6fca7c0-079c-4337-23d9-cd970c79b8ad@web.de","subject":"Re: [PATCH 1/2] gpg-interface: handle missing \" with \" gracefully in parse_ssh_output()","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-01T08:40:18Z","receivedAt":"2021-11-01T08:40:24Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 30.10.21 19:04, René Scharfe wrote:\n> If the output of ssh-keygen starts with \"Good \\\"git\\\" signature for \",\n> but is not followed by \" with \" for some reason, then parse_ssh_output()\n> uses -1 as the len parameter of xmemdupz(), which in turn will end the\n> program. Reject the signature and carry on instead in that case.\n> \n> Signed-off-by: René Scharfe \n> ---\n> This code was added after v2.33.0.\n> Patch formatted with --inter-hunk-context=2 for easier review.\n> \n> Silly bonus question: What's the purpose of the \"+ 1\" and \"- 1\", which\n> seem to cancel each other out?\n\nThey do. But only for the xmemdupz. It is important for the strstr() to\nskip over already found strings (\" with \" could be in the principal name\nas well - multiple times even). And doing strstr(line +1, ...) can be\nproblematic for the first iteration.\n\n> \n> gpg-interface.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n> \n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 800d8caa67..62d340e78a 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc)\n> \tline = to_free = xmemdupz(sigc->output, strcspn(sigc->output, \"\\n\"));\n> \n> \tif (skip_prefix(line, \"Good \\\"git\\\" signature for \", &line)) {\n> -\t\t/* Valid signature and known principal */\n> -\t\tsigc->result = 'G';\n> -\t\tsigc->trust_level = TRUST_FULLY;\n> -\n> \t\t/* Search for the last \"with\" to get the full principal */\n> \t\tprincipal = line;\n> \t\tdo {\n> \t\t\tsearch = strstr(line, \" with \");\n> \t\t\tif (search)\n> \t\t\t\tline = search + 1;\n> \t\t} while (search != NULL);\n> +\t\tif (line == principal)\n> +\t\t\tgoto cleanup;\n> +\n> +\t\t/* Valid signature and known principal */\n> +\t\tsigc->result = 'G';\n> +\t\tsigc->trust_level = TRUST_FULLY;\n> \t\tsigc->signer = xmemdupz(principal, line - principal - 1);\n> \t} else if (skip_prefix(line, \"Good \\\"git\\\" signature with \", &line)) {\n> \t\t/* Valid signature, but key unknown */\n> --\n> 2.33.1\n> \n\nThe fix looks good otherwise.\nThanks!\n"},{"id":"440223","messageId":"xmqqbl33db3a.fsf@gitster.g","threadId":"56814","inReplyTo":"d758381f-064a-1d0a-afab-cb2160ea2eec@gigacodes.de","subject":"Re: [PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-01T17:44:57Z","receivedAt":"2021-11-01T17:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> On 30.10.21 19:07, René Scharfe wrote:\n>> If the string \"key\" we found in the output of ssh-keygen happens to be\n>> located at the very end of the line, then going four characters further\n>> leaves us beyond the end of the string. Explicitly search for the\n>> space after \"key\" to handle a missing one gracefully.\n>> \n>> Signed-off-by: René Scharfe \n>> ---\n>> This code was added after v2.33.0.\n>> \n>> \n>\n> Thanks. This is obviously correct.\n\nThanks, both. Will apply.\n"},{"id":"440225","messageId":"abc4c174-f848-a0f4-3f1d-486ee8f275b2@web.de","threadId":"56814","inReplyTo":"dedf3971-adb1-c355-0b05-4e94ad92e1f7@gigacodes.de","subject":"Re: [PATCH 1/2] gpg-interface: handle missing \" with \" gracefully in parse_ssh_output()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-11-01T18:36:45Z","receivedAt":"2021-11-01T18:37:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.11.21 um 09:40 schrieb Fabian Stelzer:\n> On 30.10.21 19:04, René Scharfe wrote:\n>> Silly bonus question: What's the purpose of the \"+ 1\" and \"- 1\", which\n>> seem to cancel each other out?\n>\n> They do. But only for the xmemdupz. It is important for the strstr() to\n> skip over already found strings (\" with \" could be in the principal name\n> as well - multiple times even). And doing strstr(line +1, ...) can be\n> problematic for the first iteration.\n\nAh, right.\n\n>> \t\tdo {\n>> \t\t\tsearch = strstr(line, \" with \");\n>> \t\t\tif (search)\n>> \t\t\t\tline = search + 1;\n>> \t\t} while (search != NULL);\n\nThis can be shortened to:\n\n\t\twhile ((search = strstr(line, \" with \")))\n\t\t\tline = search + 1;\n\nI still would have tripped over the +1 / -1, though. Calling an rfind()\nor strrstr() or find_last() function that deals with it internally\nwould have helped me avoid that, I think, but we don't have use for such\na thing anywhere else.\n\nRené\n"}]}