sox-devel

Re: [SoX-devel] [PATCH] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Eric W. <nor...@yh...>]

Måns Rullgård <ma...@ma...> wrote:
> Eric Wong <nor...@yh...> writes:
> > Mans Rullgard <ma...@ma...> wrote:
> >> --- a/src/adpcm.h
> >> +++ b/src/adpcm.h
> >> @@ -29,8 +29,11 @@
> >>  /* default coef sets */
> >>  extern const short lsx_ms_adpcm_i_coef[7][2];
> >>  
> >> +extern void *lsx_ms_adpcm_alloc(unsigned chans);
> >> +
> >>  /* lsx_ms_adpcm_block_expand_i() outputs interleaved samples into one output buffer */
> >>  extern const char *lsx_ms_adpcm_block_expand_i(
> >> +	void *priv,
> >>  	unsigned chans,          /* total channels             */
> >>  	int nCoef,
> >>  	const short *coef,
> >
> > Thanks, seems fine; though I'd probably export an opaque struct
> > which makes the unsigned chans arg redundant.
> 
> Do you mean to store the number of channels as well as the state buffer
> in a struct?

Exactly.

> > I'm a little concerned about the internal API changes like this
> > affecting some 3rd-party code somewhere; but I guess we limit
> > our exports nowadays (ugh, and that export regexp is nasty)
> 
> These functions aren't exported, and the supported interface is whatever
> is in sox.h, nothing else.

OK.  I guess I'm overly cautious from other projects where
people reach deep into internals and rely on it :/

Re: [SoX-devel] [PATCH] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Måns R. <ma...@ma...>]

Eric Wong <nor...@yh...> writes:

> Måns Rullgård <ma...@ma...> wrote:
>> Eric Wong <nor...@yh...> writes:
>> > Mans Rullgard <ma...@ma...> wrote:
>> >> --- a/src/adpcm.h
>> >> +++ b/src/adpcm.h
>> >> @@ -29,8 +29,11 @@
>> >>  /* default coef sets */
>> >>  extern const short lsx_ms_adpcm_i_coef[7][2];
>> >>  
>> >> +extern void *lsx_ms_adpcm_alloc(unsigned chans);
>> >> +
>> >>  /* lsx_ms_adpcm_block_expand_i() outputs interleaved samples into one output buffer */
>> >>  extern const char *lsx_ms_adpcm_block_expand_i(
>> >> +	void *priv,
>> >>  	unsigned chans,          /* total channels             */
>> >>  	int nCoef,
>> >>  	const short *coef,
>> >
>> > Thanks, seems fine; though I'd probably export an opaque struct
>> > which makes the unsigned chans arg redundant.
>> 
>> Do you mean to store the number of channels as well as the state buffer
>> in a struct?
>
> Exactly.

Sure, we could do that.  Then we could put some other stuff there too.

>> > I'm a little concerned about the internal API changes like this
>> > affecting some 3rd-party code somewhere; but I guess we limit
>> > our exports nowadays (ugh, and that export regexp is nasty)
>> 
>> These functions aren't exported, and the supported interface is
>> whatever is in sox.h, nothing else.
>
> OK.  I guess I'm overly cautious from other projects where
> people reach deep into internals and rely on it :/

My opinion is that if you rely on internal, undocumented interfaces
exposed by accident, you're on your own and should expect breakage from
time to time.

-- 
Måns Rullgård

Re: [SoX-devel] [PATCH v2] adpcm: fix stack overflow (CVE-2017-15372)

[Hans P. S. <hp...@se...>]

On 11/08/17 00:38, Eric Wong wrote:
> @@ -113,7 +113,13 @@ const char *lsx_ms_adpcm_block_expand_i(
>     const unsigned char *ip;
>     unsigned ch;
>     const char *errmsg = NULL;
> -  MsState_t state[4];  /* One decompressor state for each channel */
> +
> +  /* One decompressor state for each channel */
> +  MsState_t *state;
> +  size_t size = sizeof(*state) * chans;
> +  const size_t alloca_max = 1024;
> +

Hi,

Just make a fixed buffer on the stack instead of using alloca().

> +  state = size > alloca_max ? lsx_malloc(size) : alloca(size);
>   
>     /* Read the four-byte header for each channel */
>     ip = ibuff;
> @@ -158,6 +164,10 @@ const char *lsx_ms_adpcm_block_expand_i(
>         if (++ch2 == chans) ch2 = 0;
>       }
>     }
> +
> +  if (size > alloca_max)
> +    free(state);
> +
>     return errmsg;
>   }

Is the "chans" argument range-checked anywhere? You should put a limit 
how many channels are supported!

--HPS

Re: [SoX-devel] [PATCH v2] adpcm: fix stack overflow (CVE-2017-15372)

[Måns R. <ma...@ma...>]

Hans Petter Selasky <hp...@se...> writes:

> On 11/08/17 00:38, Eric Wong wrote:
>> @@ -113,7 +113,13 @@ const char *lsx_ms_adpcm_block_expand_i(
>>     const unsigned char *ip;
>>     unsigned ch;
>>     const char *errmsg = NULL;
>> -  MsState_t state[4];  /* One decompressor state for each channel */
>> +
>> +  /* One decompressor state for each channel */
>> +  MsState_t *state;
>> +  size_t size = sizeof(*state) * chans;
>> +  const size_t alloca_max = 1024;
>> +
>
> Hi,
>
> Just make a fixed buffer on the stack instead of using alloca().
>
>> +  state = size > alloca_max ? lsx_malloc(size) : alloca(size);
>>       /* Read the four-byte header for each channel */
>>     ip = ibuff;
>> @@ -158,6 +164,10 @@ const char *lsx_ms_adpcm_block_expand_i(
>>         if (++ch2 == chans) ch2 = 0;
>>       }
>>     }
>> +
>> +  if (size > alloca_max)
>> +    free(state);
>> +
>>     return errmsg;
>>   }
>
> Is the "chans" argument range-checked anywhere? You should put a limit
> how many channels are supported!

The WAV container is limited to 64k channels.  We could of course
enforce a lower limit.

-- 
Måns Rullgård

Re: [SoX-devel] [PATCH v2] adpcm: fix stack overflow (CVE-2017-15372)

[Hans P. S. <hp...@se...>]

On 11/08/17 12:07, Måns Rullgård wrote:
> The WAV container is limited to 64k channels.  We could of course
> enforce a lower limit.

Hi,

You should not allow that many channels. Make sure the value is >= 1, to 
avoid division by zero and <= 512 to avoid overflow. During my time as a 
sound technican, it is very rare that the number of channels go beyond 
64. It has practical implications, that the data rate goes into the roof 
and USB audio among others is not possible.

--HPS

Re: [SoX-devel] [PATCH v2] adpcm: fix stack overflow (CVE-2017-15372)

[Måns R. <ma...@ma...>]

Hans Petter Selasky <hp...@se...> writes:

> On 11/08/17 12:07, Måns Rullgård wrote:
>> The WAV container is limited to 64k channels.  We could of course
>> enforce a lower limit.
>
> Hi,
>
> You should not allow that many channels. Make sure the value is >= 1, to
> avoid division by zero

We already do that.

> and <= 512 to avoid overflow.

That's a pretty arbitrary limit.

> During my time as a sound technican, it is very rare that the number
> of channels go beyond 64. It has practical implications, that the data
> rate goes into the roof and USB audio among others is not possible.

During my time working on AV software, there's no end to the crazy
things I've seen people do.  Someone might have a good reason to store a
silly number of channels.  Not all audio files are intended to be sent
to a playback device.

-- 
Måns Rullgård