Buffer overflow with long fd numbers in redirects

Buffer overflow with long fd numbers in redirects

[Mikael Magnusson]

Someone reported this on IRC the other day,
% >&333333333333333333333
zsh: number truncated after 20 digits: 333333333333333333333
*** buffer overflow detected ***: zsh terminated

At least one place where this is mishandled is in exec.c around line 3215,
        if (fil == -1) {
            char fdstr[4];

            closemnodes(mfds);
            fixfds(save);
            if (fn->fd2 != -2)
                sprintf(fdstr, "%d", fn->fd2);
            if (errno)
            zwarn("%s: %e", fn->fd2 == -2 ? "coprocess" : fdstr,
                  errno);
            execerr();
        }

Obviously anything over 999 will not fit in fdstr[]. I just checked
and it appears we do not use snprintf anywhere, is this for any
particular reason? The patch below just changes the array to [64], it
should be some time before any system uses a 256-bit type for fds. If
you guys have another preference for solving this, let me know. Note
however that just adding a check if (fn->fd2 != -2 && fn->fd2 < 1000
&& fn->fd2 > -100) is not sufficient since the zwarn attempts to use
fdstr for printing the error. (This is what I did first).

Output with the patch,
% >&333333333333333333333
zsh: number truncated after 20 digits: 333333333333333333333
zsh: 553997653: bad file descriptor

Arguably fdstr could be 21 because of that truncation but it would be
easy to miss if we lift that restriction at some point.

diff --git i/Src/exec.c w/Src/exec.c
index 499606f..906b6ca 100644
--- i/Src/exec.c
+++ w/Src/exec.c
@@ -3210,7 +3210,7 @@ execcmd()
                    fil = movefd(dup(fd));
                }
                if (fil == -1) {
-                   char fdstr[4];
+                   char fdstr[64];

                    closemnodes(mfds);
                    fixfds(save);


-- 
Mikael Magnusson

Re: Buffer overflow with long fd numbers in redirects

[Peter Stephenson]

On Mon, 06 Oct 2014 16:00:44 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> Obviously anything over 999 will not fit in fdstr[]. I just checked
> and it appears we do not use snprintf anywhere, is this for any
> particular reason?

I think the shell's been around longer than snprintf has been
widespread.  It will need checking in configure and variant code; the
latter makes the shell less safe overall.

> The patch below just changes the array to [64], it
> should be some time before any system uses a 256-bit type for fds. If
> you guys have another preference for solving this, let me know

Shouldn't DIGBUFSIZE work?

pws

Re: Buffer overflow with long fd numbers in redirects

[Mikael Magnusson]

On 6 October 2014 16:09, Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Mon, 06 Oct 2014 16:00:44 +0200
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> Obviously anything over 999 will not fit in fdstr[]. I just checked
>> and it appears we do not use snprintf anywhere, is this for any
>> particular reason?
>
> I think the shell's been around longer than snprintf has been
> widespread.  It will need checking in configure and variant code; the
> latter makes the shell less safe overall.
>
>> The patch below just changes the array to [64], it
>> should be some time before any system uses a 256-bit type for fds. If
>> you guys have another preference for solving this, let me know
>
> Shouldn't DIGBUFSIZE work?
>
> pws

Ah, I was unaware of such a thing. I'll commit it with that instead
then, thanks!

-- 
Mikael Magnusson

Re: Buffer overflow with long fd numbers in redirects

[Peter Stephenson]

On Mon, 06 Oct 2014 16:58:11 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> > Shouldn't DIGBUFSIZE work?
>
> Ah, I was unaware of such a thing. I'll commit it with that instead
> then, thanks!

Thanks --- by the way, the deadline is likely to be some time tomorrow
evening UK time.  I'll then make the entirely unwarranted but necessary
assumption that we're not just about to find another major issue.

pws

Re: Buffer overflow with long fd numbers in redirects

[Axel Beckert]

Hi,

On Mon, Oct 06, 2014 at 04:00:44PM +0200, Mikael Magnusson wrote:
> Someone reported this on IRC the other day,
> % >&333333333333333333333
> zsh: number truncated after 20 digits: 333333333333333333333
> *** buffer overflow detected ***: zsh terminated
> 
> At least one place where this is mishandled is in exec.c around line 3215,

I can reproduce this in 5.0.6.

But I can't reproduce this in 4.3.17 as in Debian Wheezy. There it
looks exactly like this:

> Output with the patch,
> % >&333333333333333333333
> zsh: number truncated after 20 digits: 333333333333333333333
> zsh: 553997653: bad file descriptor

!518 Z7 ?0 L2 abe@snidget:~ (pts/40 zsh 4.3.17 wheezy) 16:22:44 
~ → echo $ZSH_VERSION
4.3.17
!518 Z7 ?0 L2 abe@snidget:~ (pts/40 zsh 4.3.17 wheezy) 16:22:44 
~ → >&333333333333333333333
zsh: number truncated after 20 digits: 333333333333333333333
zsh: 553997653: bad file descriptor
!519 Z8 ?1 L2 abe@snidget:~ (pts/40 zsh 4.3.17 wheezy) 16:22:50 
~ → 

So this issue probably crept in somewhen between 4.3.17 and 5.0.6.

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: Buffer overflow with long fd numbers in redirects

[Mikael Magnusson]

On 6 October 2014 16:24, Axel Beckert <abe@deuxchevaux.org> wrote:
> Hi,
>
> On Mon, Oct 06, 2014 at 04:00:44PM +0200, Mikael Magnusson wrote:
>> Someone reported this on IRC the other day,
>> % >&333333333333333333333
>> zsh: number truncated after 20 digits: 333333333333333333333
>> *** buffer overflow detected ***: zsh terminated
>>
>> At least one place where this is mishandled is in exec.c around line 3215,
>
> I can reproduce this in 5.0.6.
>
> But I can't reproduce this in 4.3.17 as in Debian Wheezy. There it
> looks exactly like this:
>
>> Output with the patch,
>> % >&333333333333333333333
>> zsh: number truncated after 20 digits: 333333333333333333333
>> zsh: 553997653: bad file descriptor
>
> !518 Z7 ?0 L2 abe@snidget:~ (pts/40 zsh 4.3.17 wheezy) 16:22:44
> ~ → echo $ZSH_VERSION
> 4.3.17
> !518 Z7 ?0 L2 abe@snidget:~ (pts/40 zsh 4.3.17 wheezy) 16:22:44
> ~ → >&333333333333333333333
> zsh: number truncated after 20 digits: 333333333333333333333
> zsh: 553997653: bad file descriptor
> !519 Z8 ?1 L2 abe@snidget:~ (pts/40 zsh 4.3.17 wheezy) 16:22:50
> ~ →

You'll only see this error if zsh was compiled with buffer overflow
checking enabled (or against a glibc that has it enabled, not 100%
sure on the implementation details), probably it wasn't for the older
package. Overflowing the buffer doesn't write on any unallocated
memory so it won't segfault (fdstr is the last variable on the stack).

-- 
Mikael Magnusson

Re: Buffer overflow with long fd numbers in redirects

[Bart Schaefer]

On Oct 6,  4:24pm, Axel Beckert wrote:
}
} On Mon, Oct 06, 2014 at 04:00:44PM +0200, Mikael Magnusson wrote:
} > Someone reported this on IRC the other day,
} > % >&333333333333333333333
} > zsh: number truncated after 20 digits: 333333333333333333333
} > *** buffer overflow detected ***: zsh terminated
} > 
} > At least one place where this is mishandled is in exec.c around line 3215,
} 
} I can reproduce this in 5.0.6.
} 
} But I can't reproduce this in 4.3.17 as in Debian Wheezy.

I think that may be a difference in compilation rather than in code, i.e.
whether the binary was compiled with FORTIFY_SOURCE defined.

The char fdstr[4] has been there since before 1999.

DIGBUFSIZ should be used there, as PWS suggested.