[PATCH] color: protect against out-of-bounds array access/assignment

[PATCH] color: protect against out-of-bounds array access/assignment

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---

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

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Johannes Schindelin]

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 <sunshine@sunshineco.com>
> ---
> 
> 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
> 
> 

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Eric Sunshine]

On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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)

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Jeff King]

On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Jonathan Nieder]

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 <sunshine@sunshineco.com>
> ---
>  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 <sunshine@sunshineco.com>
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 <sunshine@sunshineco.com>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Eric Sunshine]

On Fri, Aug 3, 2018 at 2:26 AM Jonathan Nieder <jrnieder@gmail.com> 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 <sunshine@sunshineco.com>

(patch left unsnipped)

> -- >8 --
> From: Eric Sunshine <sunshine@sunshineco.com>
> 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 <sunshine@sunshineco.com>
> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  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