On Feb 13 13:50:43, [email protected] wrote:
> On Feb 07 15:09:39, [email protected] wrote:
> > On Tue, Feb 07, 2023 at 02:48:25PM +0100, Jan Stary wrote:
> > > On Feb 06 22:56:02, [email protected] 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;
> }
