{"thread":{"id":"49004","subject":"[PATCH] color: protect against out-of-bounds array access/assignment","startedAt":"2018-08-02T09:32:42Z","lastAt":"2018-08-03T06:43:32Z","messageCount":8,"participants":["Eric Sunshine","Johannes Schindelin","Junio C Hamano","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"354225","messageId":"20180802093208.27420-1-sunshine@sunshineco.com","threadId":"49004","inReplyTo":null,"subject":"[PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T09:32:08Z","receivedAt":"2018-08-02T09:32:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"want_color_fd() is designed to work only with standard input, output,\nand error file descriptors, and stores information about each descriptor\nin an array. However, it doesn't verify that the passed-in descriptor\nlives within that set, which, with a buggy caller, could lead to\naccess/assignment outside the array bounds.\n\nSigned-off-by: Eric Sunshine \n---\n\nJust something I noticed while studying this code in relation to a patch\nreview.\n\n color.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/color.c b/color.c\nindex b1c24c69de..b0be9ce505 100644\n--- a/color.c\n+++ b/color.c\n@@ -343,6 +343,9 @@ int want_color_fd(int fd, int var)\n \n \tstatic int want_auto[3] = { -1, -1, -1 };\n \n+\tif (fd < 0 || fd >= ARRAY_SIZE(want_auto))\n+\t BUG(\"file descriptor out of range: %d\", fd);\n+\n \tif (var < 0)\n \t\tvar = git_use_color_default;\n \n-- \n2.18.0.599.g4ce2a8faa4.dirty\n\n"},{"id":"354242","messageId":"nycvar.QRO.7.76.6.1808021438460.71@tvgsbejvaqbjf.bet","threadId":"49004","inReplyTo":"20180802093208.27420-1-sunshine@sunshineco.com","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-02T12:38:56Z","receivedAt":"2018-08-02T12:39:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Thu, 2 Aug 2018, Eric Sunshine wrote:\n\n> want_color_fd() is designed to work only with standard input, output,\n> and error file descriptors, and stores information about each descriptor\n> in an array. However, it doesn't verify that the passed-in descriptor\n> lives within that set, which, with a buggy caller, could lead to\n> access/assignment outside the array bounds.\n\nACK!\n\nThanks,\nDscho\n\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> \n> Just something I noticed while studying this code in relation to a patch\n> review.\n> \n> color.c | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/color.c b/color.c\n> index b1c24c69de..b0be9ce505 100644\n> --- a/color.c\n> +++ b/color.c\n> @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var)\n> \n> \tstatic int want_auto[3] = { -1, -1, -1 };\n> \n> +\tif (fd < 0 || fd >= ARRAY_SIZE(want_auto))\n> +\t BUG(\"file descriptor out of range: %d\", fd);\n> +\n> \tif (var < 0)\n> \t\tvar = git_use_color_default;\n> \n> -- \n> 2.18.0.599.g4ce2a8faa4.dirty\n> \n> \n"},{"id":"354279","messageId":"xmqqftzw3c21.fsf@gitster-ct.c.googlers.com","threadId":"49004","inReplyTo":"nycvar.QRO.7.76.6.1808021438460.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T17:36:54Z","receivedAt":"2018-08-02T17:36:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Eric,\n>\n> On Thu, 2 Aug 2018, Eric Sunshine wrote:\n>\n>> want_color_fd() is designed to work only with standard input, output,\n>> and error file descriptors, and stores information about each descriptor\n>> in an array. However, it doesn't verify that the passed-in descriptor\n>> lives within that set, which, with a buggy caller, could lead to\n>> access/assignment outside the array bounds.\n>\n> ACK!\n>\n> Thanks,\n> Dscho\n\nDid you write a buggy caller that would have been caught or helped\nwith this change? You did not write the callee that is made more\ndefensive with this patch, so I am being curious as to where that\nAck is coming from (I wouldn't have felt curious if this were\na reviewed-by instead).\n\nIn any case, this looks like a good defensive measure.\n\n\n"},{"id":"354283","messageId":"CAPig+cQyYRvF5QkJRUi6W2LiOo_poxWyJw9dn+FOimb1ryX8Mg@mail.gmail.com","threadId":"49004","inReplyTo":"xmqqftzw3c21.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T17:45:08Z","receivedAt":"2018-08-02T17:45:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> > ACK!\n>\n> Did you write a buggy caller that would have been caught or helped\n> with this change? You did not write the callee that is made more\n> defensive with this patch, so I am being curious as to where that\n> Ack is coming from (I wouldn't have felt curious if this were\n> a reviewed-by instead).\n\nThe code being made more defensive with this patch was authored by Dscho[1].\n\n[1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)\n"},{"id":"354309","messageId":"xmqq8t5o1shl.fsf@gitster-ct.c.googlers.com","threadId":"49004","inReplyTo":"CAPig+cQyYRvF5QkJRUi6W2LiOo_poxWyJw9dn+FOimb1ryX8Mg@mail.gmail.com","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T19:24:54Z","receivedAt":"2018-08-02T19:24:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano wrote:\n>> Johannes Schindelin writes:\n>> > ACK!\n>>\n>> Did you write a buggy caller that would have been caught or helped\n>> with this change? You did not write the callee that is made more\n>> defensive with this patch, so I am being curious as to where that\n>> Ack is coming from (I wouldn't have felt curious if this were\n>> a reviewed-by instead).\n>\n> The code being made more defensive with this patch was authored by Dscho[1].\n>\n> [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)\n\nAh, OK. The original by Peff done long time ago didn't check three\nfds separately, but just did a single check_auto_color() implicitly\nonly for the standard output.\n\nCome to think of it, would want_color_fd(0, var) ever make sense?\n\nIn any case, thanks for unconfusing me.\n"},{"id":"354310","messageId":"20180802193038.GB28962@sigill.intra.peff.net","threadId":"49004","inReplyTo":"xmqq8t5o1shl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T19:30:38Z","receivedAt":"2018-08-02T19:30:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n> \n> > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano wrote:\n> >> Johannes Schindelin writes:\n> >> > ACK!\n> >>\n> >> Did you write a buggy caller that would have been caught or helped\n> >> with this change? You did not write the callee that is made more\n> >> defensive with this patch, so I am being curious as to where that\n> >> Ack is coming from (I wouldn't have felt curious if this were\n> >> a reviewed-by instead).\n> >\n> > The code being made more defensive with this patch was authored by Dscho[1].\n> >\n> > [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)\n> \n> Ah, OK. The original by Peff done long time ago didn't check three\n> fds separately, but just did a single check_auto_color() implicitly\n> only for the standard output.\n\nRight. Technically Eric's check could go into the \"if (...AUTO)\"\nconditional, since that was what was touched in 295d949cfa. But I doubt\nit matters for performance (and if it did, we should probably be\ncoalescing all of these conditionals into a single cached int flag).\n\n> Come to think of it, would want_color_fd(0, var) ever make sense?\n\nNo, it's just there because of 0-indexing. The BUG() check could\nactually be \"if (fd < 1 || ...)\" to cover that (it \"works\", but it is\nnonsensical).\n\nOr we could even use \"fd - 1\" as the index. But it is probably not worth\nthe effort.\n\n-Peff\n"},{"id":"354352","messageId":"20180803060749.GA237521@aiede.svl.corp.google.com","threadId":"49004","inReplyTo":"20180802093208.27420-1-sunshine@sunshineco.com","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-03T06:07:49Z","receivedAt":"2018-08-03T06:08:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nEric Sunshine wrote:\n\n> want_color_fd() is designed to work only with standard input, output,\n> and error file descriptors, and stores information about each descriptor\n> in an array. However, it doesn't verify that the passed-in descriptor\n> lives within that set, which, with a buggy caller, could lead to\n> access/assignment outside the array bounds.\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> color.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/color.c b/color.c\n> index b1c24c69de..b0be9ce505 100644\n> --- a/color.c\n> +++ b/color.c\n> @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var)\n> \n> \tstatic int want_auto[3] = { -1, -1, -1 };\n> \n> +\tif (fd < 0 || fd >= ARRAY_SIZE(want_auto))\n> +\t BUG(\"file descriptor out of range: %d\", fd);\n\nThe indentation looks wrong here.\n\nCombining that with the other suggestions from this thread, I end up\nwith this v2:\n\n-- >8 --\nFrom: Eric Sunshine \nSubject: color: protect against out-of-bounds reads and writes\n\nwant_color_fd() is designed to work only with standard output and\nerror file descriptors and stores information about each descriptor in\nan array. However, it doesn't verify that the passed-in descriptor\nlives within that set, which, with a buggy caller, could lead to\naccess or assignment outside the array bounds.\n\nSigned-off-by: Eric Sunshine \nAcked-by: Johannes Schindelin \nSigned-off-by: Jonathan Nieder \n---\n color.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/color.c b/color.c\nindex b1c24c69de..ebb222ec33 100644\n--- a/color.c\n+++ b/color.c\n@@ -343,6 +343,9 @@ int want_color_fd(int fd, int var)\n \n \tstatic int want_auto[3] = { -1, -1, -1 };\n \n+\tif (fd < 1 || fd >= ARRAY_SIZE(want_auto))\n+\t\tBUG(\"file descriptor out of range: %d\", fd);\n+\n \tif (var < 0)\n \t\tvar = git_use_color_default;\n \n-- \n2.18.0.597.ga71716f1ad\n\n"},{"id":"354356","messageId":"CAPig+cTbXt30ic+6T32zRRfBRAZo6LtgS+XGQ_Tw7UtFkbHYew@mail.gmail.com","threadId":"49004","inReplyTo":"20180803060749.GA237521@aiede.svl.corp.google.com","subject":"Re: [PATCH] color: protect against out-of-bounds array access/assignment","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T06:43:17Z","receivedAt":"2018-08-03T06:43:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 2:26 AM Jonathan Nieder wrote:\n> Eric Sunshine wrote:\n> > + if (fd < 0 || fd >= ARRAY_SIZE(want_auto))\n> > + BUG(\"file descriptor out of range: %d\", fd);\n>\n> The indentation looks wrong here.\n\nYep, that's weird. I can't figure out how that got indented with four\nspaces rather than a TAB. I just re-typed the entire change in my\neditor as I believe I typed it earlier, and the editor correctly\nauto-indented it with a TAB. Odd. Thanks for catching it.\n\n> Combining that with the other suggestions from this thread, I end up\n> with this v2:\n\nThis looks good to me. Thanks. And, if needed:\n\nSigned-off-by: Eric Sunshine \n\n(patch left unsnipped)\n\n> -- >8 --\n> From: Eric Sunshine \n> Subject: color: protect against out-of-bounds reads and writes\n>\n> want_color_fd() is designed to work only with standard output and\n> error file descriptors and stores information about each descriptor in\n> an array. However, it doesn't verify that the passed-in descriptor\n> lives within that set, which, with a buggy caller, could lead to\n> access or assignment outside the array bounds.\n>\n> Signed-off-by: Eric Sunshine \n> Acked-by: Johannes Schindelin \n> Signed-off-by: Jonathan Nieder \n> ---\n> color.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/color.c b/color.c\n> index b1c24c69de..ebb222ec33 100644\n> --- a/color.c\n> +++ b/color.c\n> @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var)\n>\n> static int want_auto[3] = { -1, -1, -1 };\n>\n> + if (fd < 1 || fd >= ARRAY_SIZE(want_auto))\n> + BUG(\"file descriptor out of range: %d\", fd);\n> +\n> if (var < 0)\n> var = git_use_color_default;\n>\n> --\n> 2.18.0.597.ga71716f1ad\n"}]}