{"thread":{"id":"28153","subject":"[PATCH] fetch-pack: check for valid commit from server","startedAt":"2011-08-18T13:36:03Z","lastAt":"2011-08-19T20:18:13Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Shawn Pearce","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173755","messageId":"1313674563-12755-1-git-send-email-pclouds@gmail.com","threadId":"28153","inReplyTo":null,"subject":"[PATCH] fetch-pack: check for valid commit from server","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-18T13:36:03Z","receivedAt":"2011-08-18T13:36:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A malicious server can return ACK with non-existent SHA-1 or not a\ncommit. lookup_commit() in this case may return NULL. Do not let\nfetch-pack crash by accessing NULL address in this case.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n However it raises another question, what if the other end returns a\n valid commit, but not the one in \"have\" line fetch-pack sent? Are we\n OK with that?\n\n builtin/fetch-pack.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4367984..3c871c2 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -395,6 +395,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,\n \t\t\t\tcase ACK_continue: {\n \t\t\t\t\tstruct commit *commit =\n \t\t\t\t\t\tlookup_commit(result_sha1);\n+\t\t\t\t\tif (!commit)\n+\t\t\t\t\t\tdie(\"invalid commit %s\", sha1_to_hex(result_sha1));\n \t\t\t\t\tif (args.stateless_rpc\n \t\t\t\t\t && ack == ACK_common\n \t\t\t\t\t && !(commit->object.flags & COMMON)) {\n-- \n1.7.4.74.g639db\n"},{"id":"173786","messageId":"CAJo=hJvN9W00ncG53933yiqT++QCxGLUqp8KtG=BsvKD81wWyA@mail.gmail.com","threadId":"28153","inReplyTo":"1313674563-12755-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] fetch-pack: check for valid commit from server","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-08-18T21:32:47Z","receivedAt":"2011-08-18T21:32:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"2011/8/18 Nguyễn Thái Ngọc Duy :\n> A malicious server can return ACK with non-existent SHA-1 or not a\n> commit. lookup_commit() in this case may return NULL. Do not let\n> fetch-pack crash by accessing NULL address in this case.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n>  However it raises another question, what if the other end returns a\n>  valid commit, but not the one in \"have\" line fetch-pack sent? Are we\n>  OK with that?\n\nNot really. The server is not supposed to return a SHA-1 in the ACK\nline unless the client said it first in a have line. So aborting with\nan error is reasonable thing for a client to do.\n\n> +                                       if (!commit)\n> +                                               die(\"invalid commit %s\", sha1_to_hex(result_sha1));\n\nMaybe:\n\n die(\"server ACK contained unknown commit %s\", sha1_to_hex(result_sha1));\n\nis more specific to the problem.\n\n\nJust curious, did you see this on a particular server somewhere?\n\n-- \nShawn.\n"},{"id":"173829","messageId":"CACsJy8AhQ2EqdZRyBD3-E1Sx52Jz_OhSVoGUbCmsC2LKMiYZhw@mail.gmail.com","threadId":"28153","inReplyTo":"CAJo=hJvN9W00ncG53933yiqT++QCxGLUqp8KtG=BsvKD81wWyA@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: check for valid commit from server","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T04:19:16Z","receivedAt":"2011-08-19T04:19:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/8/19 Shawn Pearce :\n> Just curious, did you see this on a particular server somewhere?\n\nNo. I was looking for lookup_commit() call sites without proper NULL handling.\n-- \nDuy\n"},{"id":"173865","messageId":"1313766693-20798-1-git-send-email-pclouds@gmail.com","threadId":"28153","inReplyTo":"1313674563-12755-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] fetch-pack: check for valid commit from server","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T15:11:33Z","receivedAt":"2011-08-19T15:11:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A malicious server can return ACK with non-existent SHA-1 or not a\ncommit. lookup_commit() in this case may return NULL. Do not let\nfetch-pack crash by accessing NULL address in this case.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n 2011/8/19 Shawn Pearce :\n > 2011/8/18 Nguyễn Thái Ngọc Duy :\n >> However it raises another question, what if the other end returns a\n >> valid commit, but not the one in \"have\" line fetch-pack sent? Are we\n >> OK with that?\n >\n > Not really. The server is not supposed to return a SHA-1 in the ACK\n > line unless the client said it first in a have line. So aborting with\n > an error is reasonable thing for a client to do.\n\n I assumed I could check result_sha1 against sha1. If it did not match,\n fetch-pack would abort. But I was wrong because fetch-pack would send\n a few have lines before receiving the first ack (which carries sha1\n of some 'have' line in the middle, not the last 'have'). I'd leave it\n here if anyone wants to tackle it.\n\n builtin/fetch-pack.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4367984..561f1a3 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -395,6 +395,9 @@ static int find_common(int fd[2], unsigned char *result_sha1,\n \t\t\t\tcase ACK_continue: {\n \t\t\t\t\tstruct commit *commit =\n \t\t\t\t\t\tlookup_commit(result_sha1);\n+\t\t\t\t\tif (!commit)\n+\t\t\t\t\t\tdie(\"server ACK contained unknown commit %s\",\n+\t\t\t\t\t\t sha1_to_hex(result_sha1));\n \t\t\t\t\tif (args.stateless_rpc\n \t\t\t\t\t && ack == ACK_common\n \t\t\t\t\t && !(commit->object.flags & COMMON)) {\n-- \n1.7.4.74.g639db\n"},{"id":"173879","messageId":"7v62lt2m0q.fsf@alter.siamese.dyndns.org","threadId":"28153","inReplyTo":"1313766693-20798-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] fetch-pack: check for valid commit from server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T20:18:13Z","receivedAt":"2011-08-19T20:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, but already queued the one from yesterday in 'maint' ;-).\n"}]}