[PATCH 0/8] CVE fixes

[PATCH 0/8] CVE fixes

[Mans Rullgard]

These patches fix all current CVEs in Sox.  They have been posted here
before, but some were buried in other threads.  I'd appreciate a final
review before putting them in the master branch (now that I can that).

Mans Rullgard (8):
  wav: fix crash if channel count is zero (CVE-2017-11332)
  hcom: fix crash on input with corrupt dictionary (CVE-2017-11358)
  wav: fix crash writing header when channel count >64k (CVE-2017-11359)
  wav: ima_adpcm: fix buffer overflow on corrupt input (CVE-2017-15370)
  flac: fix crash on corrupt metadata (CVE-2017-15371)
  adpcm: fix stack overflow with >4 channels (CVE-2017-15372)
  aiff: fix crash on empty comment chunk (CVE-2017-15642)
  xa: validate channel count (CVE-2017-18189)

 src/adpcm.c |  8 +++++++-
 src/adpcm.h |  3 +++
 src/aiff.c  |  2 +-
 src/flac.c  |  8 +++++---
 src/hcom.c  |  5 +++++
 src/wav.c   | 18 ++++++++++++++++--
 src/xa.c    |  6 ++++++
 7 files changed, 43 insertions(+), 7 deletions(-)

-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 1/8] wav: fix crash if channel count is zero (CVE-2017-11332)

[Mans Rullgard]

---
 src/wav.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/wav.c b/src/wav.c
index 5202556c0e31..71fd52acfc13 100644
--- a/src/wav.c
+++ b/src/wav.c
@@ -712,6 +712,11 @@ static int startread(sox_format_t * ft)
     else
         lsx_report("User options overriding channels read in .wav header");
 
+    if (ft->signal.channels == 0) {
+        lsx_fail_errno(ft, SOX_EHDR, "Channel count is zero");
+        return SOX_EOF;
+    }
+
     if (ft->signal.rate == 0 || ft->signal.rate == dwSamplesPerSecond)
         ft->signal.rate = dwSamplesPerSecond;
     else
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 2/8] hcom: fix crash on input with corrupt dictionary (CVE-2017-11358)

[Mans Rullgard]

---
 src/hcom.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/hcom.c b/src/hcom.c
index e76820e9333f..ee28cba24218 100644
--- a/src/hcom.c
+++ b/src/hcom.c
@@ -150,6 +150,11 @@ static int startread(sox_format_t * ft)
                 lsx_debug("%d %d",
                        p->dictionary[i].dict_leftson,
                        p->dictionary[i].dict_rightson);
+                if ((unsigned) p->dictionary[i].dict_leftson >= dictsize ||
+                    (unsigned) p->dictionary[i].dict_rightson >= dictsize) {
+                        lsx_fail_errno(ft, SOX_EHDR, "Invalid dictionary");
+                        return SOX_EOF;
+                }
         }
         rc = lsx_skipbytes(ft, (size_t) 1); /* skip pad byte */
         if (rc)
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 3/8] wav: fix crash writing header when channel count >64k (CVE-2017-11359)

[Mans Rullgard]

---
 src/wav.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/wav.c b/src/wav.c
index 71fd52acfc13..eca1cde51ee5 100644
--- a/src/wav.c
+++ b/src/wav.c
@@ -1379,6 +1379,12 @@ static int wavwritehdr(sox_format_t * ft, int second_header)
     long blocksWritten = 0;
     sox_bool isExtensible = sox_false;    /* WAVE_FORMAT_EXTENSIBLE? */
 
+    if (ft->signal.channels > UINT16_MAX) {
+        lsx_fail_errno(ft, SOX_EOF, "Too many channels (%u)",
+                       ft->signal.channels);
+        return SOX_EOF;
+    }
+
     dwSamplesPerSecond = ft->signal.rate;
     wChannels = ft->signal.channels;
     wBitsPerSample = ft->encoding.bits_per_sample;
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 4/8] wav: ima_adpcm: fix buffer overflow on corrupt input (CVE-2017-15370)

[Mans Rullgard]

Add the same check bad block size as was done for MS adpcm in commit
f39c574b ("More checks for invalid MS ADPCM blocks").
---
 src/wav.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wav.c b/src/wav.c
index eca1cde51ee5..fad334cf56e9 100644
--- a/src/wav.c
+++ b/src/wav.c
@@ -127,7 +127,7 @@ static unsigned short  ImaAdpcmReadBlock(sox_format_t * ft)
         /* work with partial blocks.  Specs say it should be null */
         /* padded but I guess this is better than trailing quiet. */
         samplesThisBlock = lsx_ima_samples_in((size_t)0, (size_t)ft->signal.channels, bytesRead, (size_t) 0);
-        if (samplesThisBlock == 0)
+        if (samplesThisBlock == 0 || samplesThisBlock > wav->samplesPerBlock)
         {
             lsx_warn("Premature EOF on .wav input file");
             return 0;
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 5/8] flac: fix crash on corrupt metadata (CVE-2017-15371)

[Mans Rullgard]

---
 src/flac.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/flac.c b/src/flac.c
index 0d7829ec830d..07f45c1be8e4 100644
--- a/src/flac.c
+++ b/src/flac.c
@@ -119,9 +119,10 @@ static void decoder_metadata_callback(FLAC__StreamDecoder const * const flac, FL
     p->total_samples = metadata->data.stream_info.total_samples;
   }
   else if (metadata->type == FLAC__METADATA_TYPE_VORBIS_COMMENT) {
+    const FLAC__StreamMetadata_VorbisComment *vc = &metadata->data.vorbis_comment;
     size_t i;
 
-    if (metadata->data.vorbis_comment.num_comments == 0)
+    if (vc->num_comments == 0)
       return;
 
     if (ft->oob.comments != NULL) {
@@ -129,8 +130,9 @@ static void decoder_metadata_callback(FLAC__StreamDecoder const * const flac, FL
       return;
     }
 
-    for (i = 0; i < metadata->data.vorbis_comment.num_comments; ++i)
-      sox_append_comment(&ft->oob.comments, (char const *) metadata->data.vorbis_comment.comments[i].entry);
+    for (i = 0; i < vc->num_comments; ++i)
+      if (vc->comments[i].entry)
+        sox_append_comment(&ft->oob.comments, (char const *) vc->comments[i].entry);
   }
 }
 
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 6/8] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Mans Rullgard]

---
 src/adpcm.c | 8 +++++++-
 src/adpcm.h | 3 +++
 src/wav.c   | 5 ++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/adpcm.c b/src/adpcm.c
index 2e13867e94b0..f64b7d5c2787 100644
--- a/src/adpcm.c
+++ b/src/adpcm.c
@@ -71,6 +71,11 @@ const short lsx_ms_adpcm_i_coef[7][2] = {
                         { 392,-232}
 };
 
+extern void *lsx_ms_adpcm_alloc(unsigned chans)
+{
+        return lsx_malloc(chans * sizeof(MsState_t));
+}
+
 static inline sox_sample_t AdpcmDecode(sox_sample_t c, MsState_t *state,
                                sox_sample_t sample1, sox_sample_t sample2)
 {
@@ -102,6 +107,7 @@ static inline sox_sample_t AdpcmDecode(sox_sample_t c, MsState_t *state,
 
 /* lsx_ms_adpcm_block_expand_i() outputs interleaved samples into one output buffer */
 const char *lsx_ms_adpcm_block_expand_i(
+        void *priv,
         unsigned chans,          /* total channels             */
         int nCoef,
         const short *coef,
@@ -113,7 +119,7 @@ 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 */
+  MsState_t *state = priv;  /* One decompressor state for each channel */
 
   /* Read the four-byte header for each channel */
   ip = ibuff;
diff --git a/src/adpcm.h b/src/adpcm.h
index af4d6f08117d..db5cc6152196 100644
--- 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,
diff --git a/src/wav.c b/src/wav.c
index fad334cf56e9..066be6d7732d 100644
--- a/src/wav.c
+++ b/src/wav.c
@@ -82,6 +82,7 @@ typedef struct {
     /* following used by *ADPCM wav files */
     unsigned short nCoefs;          /* ADPCM: number of coef sets */
     short         *lsx_ms_adpcm_i_coefs;          /* ADPCM: coef sets           */
+    void          *ms_adpcm_data;   /* Private data of adpcm decoder */
     unsigned char *packet;          /* Temporary buffer for packets */
     short         *samples;         /* interleaved samples buffer */
     short         *samplePtr;       /* Pointer to current sample  */
@@ -175,7 +176,7 @@ static unsigned short  AdpcmReadBlock(sox_format_t * ft)
         }
     }
 
-    errmsg = lsx_ms_adpcm_block_expand_i(ft->signal.channels, wav->nCoefs, wav->lsx_ms_adpcm_i_coefs, wav->packet, wav->samples, samplesThisBlock);
+    errmsg = lsx_ms_adpcm_block_expand_i(wav->ms_adpcm_data, ft->signal.channels, wav->nCoefs, wav->lsx_ms_adpcm_i_coefs, wav->packet, wav->samples, samplesThisBlock);
 
     if (errmsg)
         lsx_warn("%s", errmsg);
@@ -791,6 +792,7 @@ static int startread(sox_format_t * ft)
 
         /* nCoefs, lsx_ms_adpcm_i_coefs used by adpcm.c */
         wav->lsx_ms_adpcm_i_coefs = lsx_malloc(wav->nCoefs * 2 * sizeof(short));
+        wav->ms_adpcm_data = lsx_ms_adpcm_alloc(wChannels);
         {
             int i, errct=0;
             for (i=0; len>=2 && i < 2*wav->nCoefs; i++) {
@@ -1216,6 +1218,7 @@ static int stopread(sox_format_t * ft)
     free(wav->packet);
     free(wav->samples);
     free(wav->lsx_ms_adpcm_i_coefs);
+    free(wav->ms_adpcm_data);
     free(wav->comment);
     wav->comment = NULL;
 
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 7/8] aiff: fix crash on empty comment chunk (CVE-2017-15642)

[Mans Rullgard]

This fixes a use after free and double free if an empty comment
chunk follows a non-empty one.
---
 src/aiff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/aiff.c b/src/aiff.c
index 240d2e1fdf63..11ddb542ca85 100644
--- a/src/aiff.c
+++ b/src/aiff.c
@@ -62,7 +62,6 @@ int lsx_aiffstartread(sox_format_t * ft)
   size_t ssndsize = 0;
   char *annotation;
   char *author;
-  char *comment = NULL;
   char *copyright;
   char *nametext;
 
@@ -270,6 +269,7 @@ int lsx_aiffstartread(sox_format_t * ft)
       free(annotation);
     }
     else if (strncmp(buf, "COMT", (size_t)4) == 0) {
+      char *comment = NULL;
       rc = commentChunk(&comment, "Comment:", ft);
       if (rc) {
         /* Fail already called in function */
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

[PATCH 8/8] xa: validate channel count (CVE-2017-18189)

[Mans Rullgard]

A corrupt header specifying zero channels would send read_channels()
into an infinite loop.  Prevent this by sanity checking the channel
count in open_read().  Also add an upper bound to prevent overflow
in multiplication.
---
 src/xa.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/xa.c b/src/xa.c
index 81a767720d93..9fc086eca2b2 100644
--- a/src/xa.c
+++ b/src/xa.c
@@ -143,6 +143,12 @@ static int startread(sox_format_t * ft)
         lsx_report("User options overriding rate read in .xa header");
     }
 
+    if (ft->signal.channels == 0 || ft->signal.channels > UINT16_MAX) {
+        lsx_fail_errno(ft, SOX_EFMT, "invalid channel count %d",
+                       ft->signal.channels);
+        return SOX_EOF;
+    }
+
     /* Check for supported formats */
     if (ft->encoding.bits_per_sample != 16) {
         lsx_fail_errno(ft, SOX_EFMT, "%d-bit sample resolution not supported.",
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

Re: [PATCH 6/8] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Eric Wong]

Mans Rullgard <mans@mansr.com> wrote:
> +extern void *lsx_ms_adpcm_alloc(unsigned chans)
> +{
> +        return lsx_malloc(chans * sizeof(MsState_t));

Initially I thought this might overflow, but it appears channels
is capped to UINT16_MAX by the previous patch.


On a side note, lsx_valloc could probably be updated to do
overflow checking and we could use it here to make future
auditing/review easier.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

Re: [PATCH 6/8] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Måns Rullgård]

Eric Wong <normalperson@yhbt.net> writes:

> Mans Rullgard <mans@mansr.com> wrote:
>> +extern void *lsx_ms_adpcm_alloc(unsigned chans)
>> +{
>> +        return lsx_malloc(chans * sizeof(MsState_t));
>
> Initially I thought this might overflow, but it appears channels
> is capped to UINT16_MAX by the previous patch.
>
> On a side note, lsx_valloc could probably be updated to do
> overflow checking and we could use it here to make future
> auditing/review easier.

We should probably also put a global cap on number of channels.

-- 
Måns Rullgård

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

Re: [PATCH 6/8] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Sonny Ray]

[-- Attachment #1.1: Type: text/plain, Size: 1277 bytes --]

I'm using sox for a scalable multitrack recording use case. If we're going
to put a global cap, can it be something absurd to human eyes, like 256
channels or something? Or will that not resolve the bug?

On Sat, Apr 28, 2018 at 6:55 AM Måns Rullgård <mans@mansr.com> wrote:

> Eric Wong <normalperson@yhbt.net> writes:
>
> > Mans Rullgard <mans@mansr.com> wrote:
> >> +extern void *lsx_ms_adpcm_alloc(unsigned chans)
> >> +{
> >> +        return lsx_malloc(chans * sizeof(MsState_t));
> >
> > Initially I thought this might overflow, but it appears channels
> > is capped to UINT16_MAX by the previous patch.
> >
> > On a side note, lsx_valloc could probably be updated to do
> > overflow checking and we could use it here to make future
> > auditing/review easier.
>
> We should probably also put a global cap on number of channels.
>
> --
> Måns Rullgård
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> SoX-devel mailing list
> SoX-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/sox-devel
>

[-- Attachment #1.2: Type: text/html, Size: 1982 bytes --]

[-- Attachment #2: Type: text/plain, Size: 202 bytes --]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

Re: [PATCH 6/8] adpcm: fix stack overflow with >4 channels (CVE-2017-15372)

[Måns Rullgård]

Sonny Ray <sonnyray@gmail.com> writes:

> I'm using sox for a scalable multitrack recording use case. If we're going
> to put a global cap, can it be something absurd to human eyes, like 256
> channels or something? Or will that not resolve the bug?

I was thinking something really absurd like 64k.

-- 
Måns Rullgård

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel