# [PATCH] color: protect against out-of-bounds array access/assignment 8 messages from 2018-08-02 to 2018-08-03. Participants: Eric Sunshine, Johannes Schindelin, Junio C Hamano, Jeff King, Jonathan Nieder. Thread: https://gitlist.dev/t/49004 ## Eric Sunshine, 2018-08-02 09:32 Subject: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: <20180802093208.27420-1-sunshine@sunshineco.com> URL: https://gitlist.dev/e/20180802093208.27420-1-sunshine%40sunshineco.com ``` want_color_fd() is designed to work only with standard input, output, and error file descriptors, and stores information about each descriptor in an array. However, it doesn't verify that the passed-in descriptor lives within that set, which, with a buggy caller, could lead to access/assignment outside the array bounds. Signed-off-by: Eric Sunshine --- Just something I noticed while studying this code in relation to a patch review. color.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/color.c b/color.c index b1c24c69de..b0be9ce505 100644 --- a/color.c +++ b/color.c @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var) static int want_auto[3] = { -1, -1, -1 }; + if (fd < 0 || fd >= ARRAY_SIZE(want_auto)) + BUG("file descriptor out of range: %d", fd); + if (var < 0) var = git_use_color_default; -- 2.18.0.599.g4ce2a8faa4.dirty ``` ## Johannes Schindelin, 2018-08-02 12:38 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1808021438460.71%40tvgsbejvaqbjf.bet In-Reply-To: <20180802093208.27420-1-sunshine@sunshineco.com> ``` Hi Eric, On Thu, 2 Aug 2018, Eric Sunshine wrote: > want_color_fd() is designed to work only with standard input, output, > and error file descriptors, and stores information about each descriptor > in an array. However, it doesn't verify that the passed-in descriptor > lives within that set, which, with a buggy caller, could lead to > access/assignment outside the array bounds. ACK! Thanks, Dscho > > Signed-off-by: Eric Sunshine > --- > > Just something I noticed while studying this code in relation to a patch > review. > > color.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/color.c b/color.c > index b1c24c69de..b0be9ce505 100644 > --- a/color.c > +++ b/color.c > @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var) > > static int want_auto[3] = { -1, -1, -1 }; > > + if (fd < 0 || fd >= ARRAY_SIZE(want_auto)) > + BUG("file descriptor out of range: %d", fd); > + > if (var < 0) > var = git_use_color_default; > > -- > 2.18.0.599.g4ce2a8faa4.dirty > > ``` ## Junio C Hamano, 2018-08-02 17:36 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: URL: https://gitlist.dev/e/xmqqftzw3c21.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Johannes Schindelin writes: > Hi Eric, > > On Thu, 2 Aug 2018, Eric Sunshine wrote: > >> want_color_fd() is designed to work only with standard input, output, >> and error file descriptors, and stores information about each descriptor >> in an array. However, it doesn't verify that the passed-in descriptor >> lives within that set, which, with a buggy caller, could lead to >> access/assignment outside the array bounds. > > ACK! > > Thanks, > Dscho Did you write a buggy caller that would have been caught or helped with this change? You did not write the callee that is made more defensive with this patch, so I am being curious as to where that Ack is coming from (I wouldn't have felt curious if this were a reviewed-by instead). In any case, this looks like a good defensive measure. ``` ## Eric Sunshine, 2018-08-02 17:45 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQyYRvF5QkJRUi6W2LiOo_poxWyJw9dn%2BFOimb1ryX8Mg%40mail.gmail.com In-Reply-To: ``` On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano wrote: > Johannes Schindelin writes: > > ACK! > > Did you write a buggy caller that would have been caught or helped > with this change? You did not write the callee that is made more > defensive with this patch, so I am being curious as to where that > Ack is coming from (I wouldn't have felt curious if this were > a reviewed-by instead). The code being made more defensive with this patch was authored by Dscho[1]. [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21) ``` ## Junio C Hamano, 2018-08-02 19:24 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: URL: https://gitlist.dev/e/xmqq8t5o1shl.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Eric Sunshine writes: > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano wrote: >> Johannes Schindelin writes: >> > ACK! >> >> Did you write a buggy caller that would have been caught or helped >> with this change? You did not write the callee that is made more >> defensive with this patch, so I am being curious as to where that >> Ack is coming from (I wouldn't have felt curious if this were >> a reviewed-by instead). > > The code being made more defensive with this patch was authored by Dscho[1]. > > [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21) Ah, OK. The original by Peff done long time ago didn't check three fds separately, but just did a single check_auto_color() implicitly only for the standard output. Come to think of it, would want_color_fd(0, var) ever make sense? In any case, thanks for unconfusing me. ``` ## Jeff King, 2018-08-02 19:30 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: <20180802193038.GB28962@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180802193038.GB28962%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote: > Eric Sunshine writes: > > > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano wrote: > >> Johannes Schindelin writes: > >> > ACK! > >> > >> Did you write a buggy caller that would have been caught or helped > >> with this change? You did not write the callee that is made more > >> defensive with this patch, so I am being curious as to where that > >> Ack is coming from (I wouldn't have felt curious if this were > >> a reviewed-by instead). > > > > The code being made more defensive with this patch was authored by Dscho[1]. > > > > [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21) > > Ah, OK. The original by Peff done long time ago didn't check three > fds separately, but just did a single check_auto_color() implicitly > only for the standard output. Right. Technically Eric's check could go into the "if (...AUTO)" conditional, since that was what was touched in 295d949cfa. But I doubt it matters for performance (and if it did, we should probably be coalescing all of these conditionals into a single cached int flag). > Come to think of it, would want_color_fd(0, var) ever make sense? No, it's just there because of 0-indexing. The BUG() check could actually be "if (fd < 1 || ...)" to cover that (it "works", but it is nonsensical). Or we could even use "fd - 1" as the index. But it is probably not worth the effort. -Peff ``` ## Jonathan Nieder, 2018-08-03 06:07 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: <20180803060749.GA237521@aiede.svl.corp.google.com> URL: https://gitlist.dev/e/20180803060749.GA237521%40aiede.svl.corp.google.com In-Reply-To: <20180802093208.27420-1-sunshine@sunshineco.com> ``` Hi, Eric Sunshine wrote: > want_color_fd() is designed to work only with standard input, output, > and error file descriptors, and stores information about each descriptor > in an array. However, it doesn't verify that the passed-in descriptor > lives within that set, which, with a buggy caller, could lead to > access/assignment outside the array bounds. > > Signed-off-by: Eric Sunshine > --- > color.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/color.c b/color.c > index b1c24c69de..b0be9ce505 100644 > --- a/color.c > +++ b/color.c > @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var) > > static int want_auto[3] = { -1, -1, -1 }; > > + if (fd < 0 || fd >= ARRAY_SIZE(want_auto)) > + BUG("file descriptor out of range: %d", fd); The indentation looks wrong here. Combining that with the other suggestions from this thread, I end up with this v2: -- >8 -- From: Eric Sunshine Subject: color: protect against out-of-bounds reads and writes want_color_fd() is designed to work only with standard output and error file descriptors and stores information about each descriptor in an array. However, it doesn't verify that the passed-in descriptor lives within that set, which, with a buggy caller, could lead to access or assignment outside the array bounds. Signed-off-by: Eric Sunshine Acked-by: Johannes Schindelin Signed-off-by: Jonathan Nieder --- color.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/color.c b/color.c index b1c24c69de..ebb222ec33 100644 --- a/color.c +++ b/color.c @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var) static int want_auto[3] = { -1, -1, -1 }; + if (fd < 1 || fd >= ARRAY_SIZE(want_auto)) + BUG("file descriptor out of range: %d", fd); + if (var < 0) var = git_use_color_default; -- 2.18.0.597.ga71716f1ad ``` ## Eric Sunshine, 2018-08-03 06:43 Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTbXt30ic%2B6T32zRRfBRAZo6LtgS%2BXGQ_Tw7UtFkbHYew%40mail.gmail.com In-Reply-To: <20180803060749.GA237521@aiede.svl.corp.google.com> ``` On Fri, Aug 3, 2018 at 2:26 AM Jonathan Nieder wrote: > Eric Sunshine wrote: > > + if (fd < 0 || fd >= ARRAY_SIZE(want_auto)) > > + BUG("file descriptor out of range: %d", fd); > > The indentation looks wrong here. Yep, that's weird. I can't figure out how that got indented with four spaces rather than a TAB. I just re-typed the entire change in my editor as I believe I typed it earlier, and the editor correctly auto-indented it with a TAB. Odd. Thanks for catching it. > Combining that with the other suggestions from this thread, I end up > with this v2: This looks good to me. Thanks. And, if needed: Signed-off-by: Eric Sunshine (patch left unsnipped) > -- >8 -- > From: Eric Sunshine > Subject: color: protect against out-of-bounds reads and writes > > want_color_fd() is designed to work only with standard output and > error file descriptors and stores information about each descriptor in > an array. However, it doesn't verify that the passed-in descriptor > lives within that set, which, with a buggy caller, could lead to > access or assignment outside the array bounds. > > Signed-off-by: Eric Sunshine > Acked-by: Johannes Schindelin > Signed-off-by: Jonathan Nieder > --- > color.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/color.c b/color.c > index b1c24c69de..ebb222ec33 100644 > --- a/color.c > +++ b/color.c > @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var) > > static int want_auto[3] = { -1, -1, -1 }; > > + if (fd < 1 || fd >= ARRAY_SIZE(want_auto)) > + BUG("file descriptor out of range: %d", fd); > + > if (var < 0) > var = git_use_color_default; > > -- > 2.18.0.597.ga71716f1ad ```