On May 30 14:48:30, m...@mansr.com wrote:
> > https://nvd.nist.gov/vuln/detail/CVE-2019-8354
> >
> > fix possible buffer size overflow in lsx_make_lpf() (CVE-2019-8354)
> > The multiplication in the size argument malloc() might overflow,
> > resulting in a small buffer being allocated. Use calloc() instead.
> >
> > but the segmentation fault (core dumped) persists both immediately after
> > this commit and in all subsequent versions:
> > https://sourceforge.net/p/sox/bugs/319/
>
> This crash has nothing to do with the multiplication overflow described
> in the CVE. Still it's a bug, so I've fixed it.
Right; the 2019 fix
- double * h = malloc(num_taps * sizeof(*h)), sum = 0;
+ double * h = calloc(num_taps, sizeof(*h)), sum = 0;
replaces malloc(num * size) with calloc(num, size)
and is clearly correct, except for not checking
the return value of course.
Here is a couple of notes.
Firstly, there are more cases of the same.
Here is a selection from a naive grep of just
lsx_malloc(), i.e. none of the other *alloc() functions:
adpcm.c: return lsx_malloc(chans * sizeof(MsState_t));
alsa.c: p->buf = lsx_malloc(p->buf_len * formats[p->format].bytes);
chorus.c: chorus->lookup_tab[i] = lsx_malloc(sizeof (int) *
chorus->length[i]);
chorus.c: chorus->chorusbuf = lsx_malloc(sizeof (float) *
chorus->maxsamples);
coreaudio.c: ac->buf = lsx_malloc(ac->bufsize * sizeof(float));
delay.c: p->buffer = lsx_malloc(p->buffer_size * sizeof(*p->buffer));
echo.c: echo->delay_buf = lsx_malloc(sizeof (double) * echo->maxsamples);
echos.c: echos->delay_buf = lsx_malloc(sizeof (double) *
echos->sumsamples);
effects.c: char * * argv2 = lsx_malloc((argc + 1) * sizeof(*argv2));
effects.c: chain->il_buf = lsx_malloc(sox_globals.bufsiz *
sizeof(sox_sample_t));
effects_i_dsp.c: double p, qn, sig, un, * u = lsx_malloc((n - 1) * sizeof(*u));
effects_i_dsp.c: double * work = lsx_malloc(n * sizeof(*work));
effects_i_dsp.c: pi_wraps = lsx_malloc((((size_t)work_len + 2) / 2) *
sizeof(*pi_wraps));
effects_i_dsp.c: double * H = lsx_malloc((N / 2 + 1) * sizeof(*H));
example2.c: assert((buf = malloc(sizeof(sox_sample_t) * block_size)));
flac.c: p->leftover_buf = lsx_malloc(to_stash * sizeof(sox_sample_t));
flac.c: p->decoded_samples = lsx_malloc(p->number_of_samples *
sizeof(FLAC__int32));
formats.c: char * c = lsx_malloc((len + 1) * sizeof(*c));
formats.c: char * command = lsx_malloc(strlen(command_format) +
strlen(identifier));
formats.c: char * text = lsx_malloc(text_length + 1);
formats_i.c: uint8_t *data = lsx_malloc(size * len); \
formats_i.c: uint8_t *data = lsx_malloc(size * len); \
gsm.c: p->frames = lsx_malloc(p->channels*FRAMESIZE);
gsm.c: p->samples = lsx_malloc(BLOCKSIZE * (p->channels+1) *
sizeof(gsm_signal));
hcom.c: p->dictionary = lsx_malloc(511 * sizeof(dictent));
id3.c: char * comment = lsx_malloc(strlen(id3tagmap[i][1]) + 1 +
strlen((char *)utf8) + 1);
ladspa.c: l_st->inputs = lsx_malloc(l_st->desc->PortCount * sizeof(unsigned
long));
ladspa.c: l_st->outputs = lsx_malloc(l_st->desc->PortCount * sizeof(unsigned
long));
ladspa.c: l_st->handles = lsx_malloc(effp->in_signal.channels *
mcompand.c: l->attackRate = lsx_malloc(sizeof(double) * rates);
mcompand.c: l->decayRate = lsx_malloc(sizeof(double) * rates);
mcompand.c: l->volume = lsx_malloc(sizeof(double) * rates);
mcompand.c: ibuf_copy = lsx_malloc(*isamp * sizeof(sox_sample_t));
noisered.c: double * work = malloc(2 * NumSamples * sizeof(*work));
phaser.c: p->mod_buf = lsx_malloc(p->mod_buf_len * sizeof(*p->mod_buf));
rate.c: sample_t * result = malloc(length * (interp_order + 1) *
sizeof(*result));
raw.c: ctype *data = lsx_malloc(sizeof(ctype) * len); \
raw.c: ctype *data = lsx_malloc(sizeof(ctype) * len); \
silence.c: silence->window = lsx_malloc(silence->window_size *
sizeof(double));
silence.c: silence->start_holdoff =
lsx_malloc(sizeof(sox_sample_t)*silence->start_duration);
silence.c: silence->stop_holdoff =
lsx_malloc(sizeof(sox_sample_t)*silence->stop_duration);
sox.c: z->ibuf = lsx_malloc(input_count * sizeof(*z->ibuf));
sox.c: z->ibuf[i] = lsx_malloc(sox_globals.bufsiz * sizeof(sox_sample_t));
sox.c: z->ilen = lsx_malloc(input_count * sizeof(*z->ilen));
sox.c: format_list = lsx_malloc(formats * sizeof(*format_list));
spectrogram.c: double *q = lsx_malloc(2 * (n / 2 + 1) * n * sizeof(*q));
spectrogram.c: png_byte *pixels = lsx_malloc(cols * rows *
sizeof(*pixels));
spectrogram.c: png_bytepp png_rows = lsx_malloc(rows * sizeof(*png_rows));
speexdsp.c: p->buffer = lsx_malloc(p->buffer_end * sizeof(p->buffer[0]));
stat.c: stat->re_in = lsx_malloc(sizeof(float) * stat->fft_size);
stat.c: stat->re_out = lsx_malloc(sizeof(float) * (stat->fft_size / 2 + 1));
stretch.c: p->ibuf = lsx_malloc(p->segment * sizeof(sox_sample_t));
stretch.c: p->obuf = lsx_malloc(p->segment * sizeof(double));
stretch.c: p->fade_coefs = lsx_malloc(p->overlap * sizeof(double));
tempo.c: t->overlap_buf = lsx_malloc(t->overlap * t->channels *
sizeof(*t->overlap_buf));
wav.c: wav->samples = lsx_malloc(sbsize*sizeof(short));
waveaudio.c: priv->data = lsx_malloc((priv->buf_len * num_buffers) <<
priv->sample_shift);
wavpack.c: int32_t * obuf = lsx_malloc(len * sizeof(*obuf));
Not all of these are necessarily bugs that break something,
but the precision of 511 in lsx_malloc(511 * sizeof(dictent))
surely break my heart.
An obvious homework now is to go through all of them
and see if they have the same problem that Mans fixed (above).
Secondly, we have lsx_malloc() and lsx_malloc() etc,
so why not use those instead of plain malloc() et al?
Just kidding.
void *lsx_malloc(size_t size)
{
return lsx_checkptr(malloc(size + !size));
}
void *lsx_calloc(size_t n, size_t size)
{
return lsx_checkptr(calloc(n + !n, size + !size));
}
A comment precedes that:
* For malloc, `If the size of the space requested is zero, the behavior is
* implementation defined: either a null pointer is returned, or the
* behavior is as if the size were some nonzero value, except that the
* returned pointer shall not be used to access an object'
The wording comes straight from C17,
and indeed, behaviour of malloc(0) varies:
On OpenBSD (a sane system):
If nmemb or size is equal to 0, a unique pointer
to an access protected, zero sized object is returned.
Access via this pointer will generate a SIGSEGV exception.
On macOS (slightly silly), the manpage doesn't say,
but macOS has a malloc_size() function, so let's see:
#include <malloc/malloc.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
int
main(void)
{
void * p;
size_t s;
if ((p = malloc(1))) {
s = malloc_size(p);
printf("%zu bytes at %p\n", s, p);
memset(p, 'x', s);
write(STDOUT_FILENO, p, s);
}
return 0;
}
$ cc -Wall -pedantic -o zero zero.c
hans@mb:C$ ./zero
16 bytes at 0x6000015dc030
xxxxxxxxxxxxxxxx
The result is the same with malloc(1).
So what does lsx_malloc() do? Check for the corner case?
Return an explicit NULL? Refuse and fail? No:
if the caller requsted zero bytes,
pretend they requested exactly one byte.
For sure, that does away with the indeterminacy.
But it's completely wrong: if the caller asks
for zero bytes, they will be given 16 bytes
and allowed to write there (on macOS at least).
That _hides_the_error_ instead of exposing it.
So here's a version (rewriting xmalloc.c completely,
so see the whole file below instead of a diff):
If the caller requests zero bytes, log the case and fail.
Let's see the fallout; where in sox do we ask for zero bytes?
If size > 0 and malloc fails, fail right away, obviously
- displaying the system error, not one made up.
Otherwise, return the malloc() result.
$ sox -n file.wav trim 0 00:00:01
sox FAIL xmalloc: zero size callocation (1 * 0)
sox FAIL xmalloc: zero size callocation (1 * 0)
sox FAIL xmalloc: zero size mallocation
Apparently, saving a silent signal involves
a request to allocate zero bytes somewhere.
And it doesn't matter if the pointer is NULL.
The code contains three possibilities,
sorted from the mildest:
- return what the caller asked for
- return NULL
- fail
Please try them all and report here.
Jan
#include <stdlib.h>
#include <err.h>
#include "sox_i.h"
/* `If the size of the space requested is zero,
* the behavior is implementation defined' (says C17);
* fail immediately if the caller asked for that. */
void *
lsx_malloc(size_t size)
{
void * ptr;
if (size == 0) {
lsx_fail("zero size mallocation");
return malloc(0);
return NULL;
exit(2);
}
if ((ptr = malloc(size)) == NULL)
err(2, NULL);
return ptr;
}
void *
lsx_calloc(size_t num, size_t size)
{
void * ptr;
if (num == 0 || size == 0) {
lsx_fail("zero size callocation (%zu * %zu)", num, size);
return calloc(num, size);
return NULL;
exit(2);
}
if ((ptr = calloc(num, size)) == NULL)
err(2, NULL);
return ptr;
}
void *
lsx_realloc(void *ptr, size_t newsize)
{
if (newsize == 0) {
lsx_fail("zero size reallocation");
return realloc(ptr, newsize);
return NULL;
exit(2);
}
if ((ptr = realloc(ptr, newsize)) == NULL)
err(2, NULL);
return ptr;
}
void *
lsx_realloc_array(void *p, size_t num, size_t size)
{
if (num > ((size_t)-1) / size) {
lsx_fail("reallocation size overflow");
exit(2);
}
return lsx_realloc(p, num * size);
}
char *
lsx_strdup(const char *s)
{
char * ptr;
if ((ptr = strdup(s)) == NULL)
err(2, NULL);
return ptr;
}
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel
