On Feb 13 13:50:43, h...@stare.cz wrote: > On Feb 07 15:09:39, t...@theobuehler.org wrote: > > On Tue, Feb 07, 2023 at 02:48:25PM +0100, Jan Stary wrote: > > > On Feb 06 22:56:02, t...@theobuehler.org wrote: > > > > There is an ongoing discussion on audio/sox on oss-security: > > > > > > > > https://marc.info/?l=oss-security&m=167546008232629&w=2 > > > > > > > > Steffen Nurpmeso ported the patches to apply against the commit > > > > we also use in our ports, that's what's included in the diff below. > > > > > > > > The patches look sensible to me although I haven't reviewed them > > > > thoroughly. > > > > > > > > It's probably a good idea to keep an eye on this discussion both for > > > > reviews of the patches and for possible developments of a new upstream > > > > repo containing them. > > > > > > I just asked upstream - let's wait a on whether the upstream maintainer > > > decides to include these in the upstream git (SF) that we build from; > > > I would prefer that to maintaining the patches (thank you Steffen!) > > > > For reference: > > > > https://marc.info/?l=sox-devel&m=167577672104072&w=2 > > > > The patches were sent to oss-security *because* upstream failed to react: > > > > I am working on fixing known vulnerabilities in sox and since upstream > > seems mostly dead (no commits in more than a year, no replies to bug > > reports) > > > > Given this, waiting until upstream decides to unhibernate makes little > > sense to me. > > After talkig to both Helmut and upstream, > I am convinced there is no point waiting > till upstream acts on this (or anything, really).
Can we have those patches in the port please? Jan > I have created merge requests upstream > for each of the patches individually. > Let's see ahat happens. > > Going through the individual patches: > it seems SOX_EOF is the universal indicator of a returned error, > even though the err msgs use SOX_EHDR or SOX_EFMT. > But it seems the rest of the code doesn't really > distinguish the kind of returned error ... > > Comments inline. > > Jan > > > > diff --git a/src/aiff.c b/src/aiff.c > > index 3a152c588c..6de94f3276 100644 > > --- a/src/aiff.c > > +++ b/src/aiff.c > > @@ -619,6 +619,11 @@ int lsx_aiffstartwrite(sox_format_t * ft) > > At 48 kHz, 16 bits stereo, this gives ~3 hours of audio. > > Sorry, the AIFF format does not provide for an indefinite > > number of samples. */ > > + if (ft->signal.channels >= (0x7f000000 / > > (ft->encoding.bits_per_sample >> 3))) > > + { > > + lsx_fail_errno(ft, SOX_EOF, "too many channels for AIFF > > header"); > > + return SOX_EOF; > > + } > > return(aiffwriteheader(ft, (uint64_t) 0x7f000000 / > > ((ft->encoding.bits_per_sample>>3)*ft->signal.channels))); > > } > > This prevents sox from _writing_ an aiff file > with an absurd number of channels; the bug > https://sourceforge.net/p/sox/bugs/360/ > seems related: should we also refuse to _read_ > audio files with 1073807359 channels and the like? > > > > > diff --git a/src/formats.c b/src/formats.c > > index 3fcf4382b6..5eda5e3612 100644 > > --- a/src/formats.c > > +++ b/src/formats.c > > @@ -627,6 +627,7 @@ error: > > free(ft->priv); > > free(ft->filename); > > free(ft->filetype); > > + sox_delete_comments(&ft->oob.comments); > > free(ft); > > return NULL; > > } > > > This is in a failed open_read(); > the same imho applies to a failed open_write(); > updated patch attached. > > However, once SoX fails an open_{read,write}, > it exits soon afterwards anyway. > > > > diff --git a/src/formats_i.c b/src/formats_i.c > > index 7048040d1c..5f5ef979d4 100644 > > --- a/src/formats_i.c > > +++ b/src/formats_i.c > > @@ -19,6 +19,7 @@ > > */ > > > > #include "sox_i.h" > > +#include <limits.h> > > #include <string.h> > > #include <sys/stat.h> > > #include <stdarg.h> > > @@ -60,13 +61,24 @@ int lsx_check_read_params(sox_format_t * ft, unsigned > > channels, > > if (ft->seekable) > > ft->data_start = lsx_tell(ft); > > > > - if (channels && ft->signal.channels && ft->signal.channels != channels) > > + if (channels && ft->signal.channels && ft->signal.channels != channels) { > > lsx_warn("`%s': overriding number of channels", ft->filename); > > - else ft->signal.channels = channels; > > + } else if (channels > SHRT_MAX) { > > + lsx_fail_errno(ft, EINVAL, "implausibly large number of channels"); > > + return SOX_EOF; > > + } else { > > + ft->signal.channels = channels; > > + } > > Ah, this is what correctly rejects the files at > https://sourceforge.net/p/sox/bugs/360/ > > > - if (rate && ft->signal.rate && ft->signal.rate != rate) > > + if (rate && ft->signal.rate && ft->signal.rate != rate) { > > lsx_warn("`%s': overriding sample rate", ft->filename); > > - else ft->signal.rate = rate; > > + /* Since NaN comparisons yield false, the negation rejects them. */ > > + } else if (!(rate > 0)) { > > + lsx_fail_errno(ft, EINVAL, "invalid rate value"); > > + return SOX_EOF; > > + } else { > > + ft->signal.rate = rate; > > + } > > > > if (encoding && ft->encoding.encoding && ft->encoding.encoding != > > encoding) > > lsx_warn("`%s': overriding encoding type", ft->filename); > > diff --git a/src/hcom.c b/src/hcom.c > > index 594c870606..94ed3dbdb0 100644 > > --- a/src/hcom.c > > +++ b/src/hcom.c > > @@ -141,6 +141,11 @@ static int startread(sox_format_t * ft) > > return (SOX_EOF); > > } > > lsx_readw(ft, &dictsize); > > + if (dictsize == 0 || dictsize > 511) > > + { > > + lsx_fail_errno(ft, SOX_EHDR, "Implausible dictionary size > > in HCOM header"); > > + return SOX_EOF; > > Should this be SOX_EHDR instead? > > > + } > > > > /* Translate to sox parameters */ > > ft->encoding.encoding = SOX_ENCODING_HCOM; > > @@ -161,13 +166,18 @@ static int startread(sox_format_t * ft) > > p->dictionary[i].dict_rightson); > > if (!dictvalid(i, dictsize, p->dictionary[i].dict_leftson, > > p->dictionary[i].dict_rightson)) { > > + free(p->dictionary); > > + p->dictionary = NULL; > > lsx_fail_errno(ft, SOX_EHDR, "Invalid dictionary"); > > return SOX_EOF; > > } > > } > > rc = lsx_skipbytes(ft, (size_t) 1); /* skip pad byte */ > > - if (rc) > > + if (rc) { > > + free(p->dictionary); > > + p->dictionary = NULL; > > return rc; > > + } > > > > /* Initialized the decompression engine */ > > p->checksum = checksum; > > @@ -249,6 +259,9 @@ static int stopread(sox_format_t * ft) > > { > > register priv_t *p = (priv_t *) ft->priv; > > > > + free(p->dictionary); > > + p->dictionary = NULL; > > + > > if (p->huffcount != 0) > > { > > lsx_fail_errno(ft,SOX_EFMT,"not all HCOM data read"); > > @@ -259,8 +272,7 @@ static int stopread(sox_format_t * ft) > > lsx_fail_errno(ft,SOX_EFMT,"checksum error in HCOM data"); > > return (SOX_EOF); > > } > > - free(p->dictionary); > > - p->dictionary = NULL; > > + > > return (SOX_SUCCESS); > > } > > > > diff --git a/src/sphere.c b/src/sphere.c > > index a3fd1c64c2..9544d16000 100644 > > --- a/src/sphere.c > > +++ b/src/sphere.c > > @@ -63,7 +63,8 @@ static int start_read(sox_format_t * ft) > > return (SOX_EOF); > > } > > > > - header_size -= (strlen(buf) + 1); > > + bytes_read = strlen(buf); > > + header_size -= bytes_read >= header_size ? header_size : bytes_read + 1; > > > > while (strncmp(buf, "end_head", (size_t)8) != 0) { > > if (strncmp(buf, "sample_n_bytes", (size_t)14) == 0) > > @@ -105,7 +106,8 @@ static int start_read(sox_format_t * ft) > > return (SOX_EOF); > > } > > > > - header_size -= (strlen(buf) + 1); > > + bytes_read = strlen(buf); > > + header_size -= bytes_read >= header_size ? header_size : bytes_read + > > 1; > > } > > > > if (!bytes_per_sample) > > diff --git a/src/voc.c b/src/voc.c > > index a75639e94e..0ca07f9450 100644 > > --- a/src/voc.c > > +++ b/src/voc.c > > @@ -625,6 +625,10 @@ static int getblock(sox_format_t * ft) > > v->rate = new_rate_32; > > ft->signal.rate = new_rate_32; > > lsx_readb(ft, &uc); > > + if (uc <= 1) { > > + lsx_fail_errno(ft, SOX_EFMT, "2 bits per word required"); > > + return (SOX_EOF); > > SOX_EFMT? > > > + } > > v->size = uc; > > lsx_readb(ft, &uc); > > if (v->channels != -1 && uc != v->channels) { > > diff --git a/src/wav.c b/src/wav.c > > index 3f6beb4517..39e0c487e8 100644 > > --- a/src/wav.c > > +++ b/src/wav.c > > @@ -654,6 +654,12 @@ static int wav_read_fmt(sox_format_t *ft, uint32_t len) > > if (err) > > return SOX_EOF; > > > > + if (wav->bitsPerSample == 0) > > This avoids the division by zero, > but should it also check for <= sensible? > > > + { > > + lsx_fail_errno(ft, SOX_EHDR, "WAV file bits per sample is zero"); > > + return SOX_EOF; > > Again, shouldn't this be SOX_EHDR? > > > + } > > + > > /* non-PCM formats except alaw and mulaw formats have extended fmt > > chunk. > > * Check for those cases. > > */ > > > > + if (p->huffcount != 0) > > + { > > + lsx_fail_errno(ft,SOX_EFMT,"not all HCOM data read"); > > +@@ -259,8 +272,7 @@ static int stopread(sox_format_t * ft) > > + lsx_fail_errno(ft,SOX_EFMT,"checksum error in HCOM data"); > > + return (SOX_EOF); > > Maybe SOX_EOF is the universal indicator of an error to the caller > (which in itself is a bit confusing), but this is SOX_EFMT, right? > > > Index: patches/patch-src_sphere_c > > =================================================================== > > RCS file: patches/patch-src_sphere_c > > diff -N patches/patch-src_sphere_c > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ patches/patch-src_sphere_c 6 Feb 2023 21:38:58 -0000 > > @@ -0,0 +1,25 @@ > > +https://marc.info/?l=oss-security&m=167571683504082&w=2 > > + > > +Index: src/sphere.c > > +--- src/sphere.c.orig > > ++++ src/sphere.c > > +@@ -63,7 +63,8 @@ static int start_read(sox_format_t * ft) > > + return (SOX_EOF); > > + } > > + > > +- header_size -= (strlen(buf) + 1); > > ++ bytes_read = strlen(buf); > > ++ header_size -= bytes_read >= header_size ? header_size : bytes_read + 1; > > + > > + while (strncmp(buf, "end_head", (size_t)8) != 0) { > > + if (strncmp(buf, "sample_n_bytes", (size_t)14) == 0) > > +@@ -105,7 +106,8 @@ static int start_read(sox_format_t * ft) > > + return (SOX_EOF); > > + } > > + > > +- header_size -= (strlen(buf) + 1); > > ++ bytes_read = strlen(buf); > > ++ header_size -= bytes_read >= header_size ? header_size : bytes_read + > > 1; > > + } > > + > > + if (!bytes_per_sample) > > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ patches/patch-src_voc_c 6 Feb 2023 21:38:58 -0000 > > @@ -0,0 +1,16 @@ > > +https://marc.info/?l=oss-security&m=167571683504082&w=2 > > + > > +Index: src/voc.c > > +--- src/voc.c.orig > > ++++ src/voc.c > > +@@ -625,6 +625,10 @@ static int getblock(sox_format_t * ft) > > + v->rate = new_rate_32; > > + ft->signal.rate = new_rate_32; > > + lsx_readb(ft, &uc); > > ++ if (uc <= 1) { > > ++ lsx_fail_errno(ft, SOX_EFMT, "2 bits per word required"); > > ++ return (SOX_EOF); > > Again, SOX_EFMT? > > > ++ } > > + v->size = uc; > > + lsx_readb(ft, &uc); > > + if (v->channels != -1 && uc != v->channels) { > > Index: patches/patch-src_wav_c > > =================================================================== > > RCS file: patches/patch-src_wav_c > > diff -N patches/patch-src_wav_c > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ patches/patch-src_wav_c 6 Feb 2023 21:38:58 -0000 > > @@ -0,0 +1,18 @@ > > +https://marc.info/?l=oss-security&m=167571683504082&w=2 > > + > > +Index: src/wav.c > > +--- src/wav.c.orig > > ++++ src/wav.c > > +@@ -654,6 +654,12 @@ static int wav_read_fmt(sox_format_t *ft, uint32_t len > > + if (err) > > + return SOX_EOF; > > + > > ++ if (wav->bitsPerSample == 0) > > ++ { > > ++ lsx_fail_errno(ft, SOX_EHDR, "WAV file bits per sample is zero"); > > ++ return SOX_EOF; > > Also here: SOX_EHDR? > > Jan > > https://marc.info/?l=oss-security&m=167571683504082&w=2 > > Index: src/formats.c > --- src/formats.c.orig > +++ src/formats.c > @@ -360,7 +360,7 @@ static int sox_checkformat(sox_format_t * ft) > return SOX_SUCCESS; > } > > -static sox_bool is_url(char const * text) /* detects only wget-supported > URLs */ > +static sox_bool is_url(char const * text) > { > return !( > strncasecmp(text, "http:" , (size_t)5) && > @@ -442,7 +442,7 @@ static FILE * xfopen(char const * identifier, char con > else if (is_url(identifier)) { > FILE * f = NULL; > #ifdef HAVE_POPEN > - char const * const command_format = "wget --no-check-certificate -q -O- > \"%s\""; > + char const * const command_format = "ftp -a -V -o - \"%s\""; > char * command = lsx_malloc(strlen(command_format) + strlen(identifier)); > sprintf(command, command_format, identifier); > f = popen(command, POPEN_MODE); > @@ -627,6 +627,7 @@ error: > free(ft->priv); > free(ft->filename); > free(ft->filetype); > + sox_delete_comments(&ft->oob.comments); > free(ft); > return NULL; > } > @@ -988,6 +989,7 @@ error: > free(ft->priv); > free(ft->filename); > free(ft->filetype); > + sox_delete_comments(&ft->oob.comments); > free(ft); > return NULL; > }