From 13b48016b3ef1e822c393c2871b0a561ce19ecb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:00:57 +0300 Subject: [PATCH 1/7] wavparse: Check for short reads when parsing headers in pull mode And also return the actual flow return to the caller instead of always returning GST_FLOW_ERROR. Thanks to Antonio Morales for finding and reporting the issue. Fixes GHSL-2024-258, GHSL-2024-260 Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/3886 Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/3888 Part-of: --- gst/wavparse/gstwavparse.c | 63 ++++++++++++++----- 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index d074f273c501..97d5591fae8f 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -1097,6 +1097,24 @@ parse_ds64 (GstWavParse * wav, GstBuffer * buf) return TRUE; } +static GstFlowReturn +gst_wavparse_pull_range_exact (GstWavParse * wav, guint64 offset, guint size, + GstBuffer ** buffer) +{ + GstFlowReturn res; + + res = gst_pad_pull_range (wav->sinkpad, offset, size, buffer); + if (res != GST_FLOW_OK) + return res; + + if (gst_buffer_get_size (*buffer) < size) { + gst_clear_buffer (buffer); + return GST_FLOW_EOS; + } + + return res; +} + static GstFlowReturn gst_wavparse_stream_headers (GstWavParse * wav) { @@ -1292,9 +1310,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) buf = NULL; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset, 8, + gst_wavparse_pull_range_exact (wav, wav->offset, 8, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; gst_buffer_map (buf, &map, GST_MAP_READ); tag = GST_READ_UINT32_LE (map.data); size = GST_READ_UINT32_LE (map.data + 4); @@ -1397,9 +1415,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) gst_buffer_unref (buf); buf = NULL; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset + 8, + gst_wavparse_pull_range_exact (wav, wav->offset + 8, data_size, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; gst_buffer_extract (buf, 0, &wav->fact, 4); wav->fact = GUINT32_FROM_LE (wav->fact); gst_buffer_unref (buf); @@ -1444,9 +1462,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) gst_buffer_unref (buf); buf = NULL; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset + 8, - size, &buf)) != GST_FLOW_OK) - goto header_read_error; + gst_wavparse_pull_range_exact (wav, wav->offset + 8, size, + &buf)) != GST_FLOW_OK) + goto header_pull_error; gst_buffer_map (buf, &map, GST_MAP_READ); acid = (const gst_riff_acid *) map.data; tempo = acid->tempo; @@ -1484,9 +1502,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) gst_buffer_unref (buf); buf = NULL; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset, 12, + gst_wavparse_pull_range_exact (wav, wav->offset, 12, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; gst_buffer_extract (buf, 8, <ag, 4); ltag = GUINT32_FROM_LE (ltag); } @@ -1513,9 +1531,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) buf = NULL; if (data_size > 0) { if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset, + gst_wavparse_pull_range_exact (wav, wav->offset, data_size, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; } } if (data_size > 0) { @@ -1553,9 +1571,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) buf = NULL; wav->offset += 12; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset, + gst_wavparse_pull_range_exact (wav, wav->offset, data_size, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; gst_buffer_map (buf, &map, GST_MAP_READ); gst_wavparse_adtl_chunk (wav, (const guint8 *) map.data, data_size); @@ -1599,9 +1617,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) gst_buffer_unref (buf); buf = NULL; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset, + gst_wavparse_pull_range_exact (wav, wav->offset, data_size, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; gst_buffer_map (buf, &map, GST_MAP_READ); if (!gst_wavparse_cue_chunk (wav, (const guint8 *) map.data, data_size)) { @@ -1643,9 +1661,9 @@ gst_wavparse_stream_headers (GstWavParse * wav) gst_buffer_unref (buf); buf = NULL; if ((res = - gst_pad_pull_range (wav->sinkpad, wav->offset, + gst_wavparse_pull_range_exact (wav, wav->offset, data_size, &buf)) != GST_FLOW_OK) - goto header_read_error; + goto header_pull_error; gst_buffer_map (buf, &map, GST_MAP_READ); if (!gst_wavparse_smpl_chunk (wav, (const guint8 *) map.data, data_size)) { @@ -1797,6 +1815,17 @@ header_read_error: ("Couldn't read in header %d (%s)", res, gst_flow_get_name (res))); goto fail; } +header_pull_error: + { + if (res == GST_FLOW_EOS) { + GST_WARNING_OBJECT (wav, "Couldn't pull header %d (%s)", res, + gst_flow_get_name (res)); + } else { + GST_ELEMENT_ERROR (wav, STREAM, DEMUX, (NULL), + ("Couldn't pull header %d (%s)", res, gst_flow_get_name (res))); + } + goto exit; + } } /* -- GitLab From 4c198f4891cfabde868944d55ff98925e7beb757 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:09:43 +0300 Subject: [PATCH 2/7] wavparse: Make sure enough data for the tag list tag is available before parsing Thanks to Antonio Morales for finding and reporting the issue. Fixes GHSL-2024-258 Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/3886 Part-of: --- gst/wavparse/gstwavparse.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index 97d5591fae8f..21cb48c07eb3 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -1489,6 +1489,10 @@ gst_wavparse_stream_headers (GstWavParse * wav) case GST_RIFF_TAG_LIST:{ guint32 ltag; + /* Need at least the ltag */ + if (size < 4) + goto exit; + if (wav->streaming) { const guint8 *data = NULL; -- GitLab From 296e17b4ea81e5c228bb853f6037b654fdca7d47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:15:27 +0300 Subject: [PATCH 3/7] wavparse: Fix parsing of acid chunk Simply casting the bytes to a struct can lead to crashes because of unaligned reads, and is also missing the endianness swapping that is necessary on big endian architectures. Part-of: --- gst/wavparse/gstwavparse.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index 21cb48c07eb3..6a0c44638ea2 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -1434,8 +1434,7 @@ gst_wavparse_stream_headers (GstWavParse * wav) break; } case GST_RIFF_TAG_acid:{ - const gst_riff_acid *acid = NULL; - const guint data_size = sizeof (gst_riff_acid); + const guint data_size = 24; gfloat tempo; GST_INFO_OBJECT (wav, "Have acid chunk"); @@ -1449,13 +1448,13 @@ gst_wavparse_stream_headers (GstWavParse * wav) break; } if (wav->streaming) { + const guint8 *data; if (!gst_wavparse_peek_chunk (wav, &tag, &size)) { goto exit; } gst_adapter_flush (wav->adapter, 8); - acid = (const gst_riff_acid *) gst_adapter_map (wav->adapter, - data_size); - tempo = acid->tempo; + data = gst_adapter_map (wav->adapter, data_size); + tempo = GST_READ_FLOAT_LE (data + 20); gst_adapter_unmap (wav->adapter); } else { GstMapInfo map; @@ -1466,8 +1465,7 @@ gst_wavparse_stream_headers (GstWavParse * wav) &buf)) != GST_FLOW_OK) goto header_pull_error; gst_buffer_map (buf, &map, GST_MAP_READ); - acid = (const gst_riff_acid *) map.data; - tempo = acid->tempo; + tempo = GST_READ_FLOAT_LE (map.data + 20); gst_buffer_unmap (buf, &map); } /* send data as tags */ -- GitLab From c72025cabdfcb2fe30d24eda7bb9d1d01a1b6555 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:21:44 +0300 Subject: [PATCH 4/7] wavparse: Check that at least 4 bytes are available before parsing cue chunks Part-of: --- gst/wavparse/gstwavparse.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index 6a0c44638ea2..5655ee3825ca 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -790,6 +790,11 @@ gst_wavparse_cue_chunk (GstWavParse * wav, const guint8 * data, guint32 size) return TRUE; } + if (size < 4) { + GST_WARNING_OBJECT (wav, "broken file %d", size); + return FALSE; + } + ncues = GST_READ_UINT32_LE (data); if (size < 4 + ncues * 24) { -- GitLab From 93d79c22a82604adc5512557c1238f72f41188c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:22:02 +0300 Subject: [PATCH 5/7] wavparse: Check that at least 32 bytes are available before parsing smpl chunks Thanks to Antonio Morales for finding and reporting the issue. Fixes GHSL-2024-259 Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/3887 Part-of: --- gst/wavparse/gstwavparse.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index 5655ee3825ca..8a04805ed427 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -894,6 +894,9 @@ gst_wavparse_smpl_chunk (GstWavParse * wav, const guint8 * data, guint32 size) { guint32 note_number; + if (size < 32) + return FALSE; + /* manufacturer_id = GST_READ_UINT32_LE (data); product_id = GST_READ_UINT32_LE (data + 4); -- GitLab From 526d0eef0d850c8f2fa1bf0aef15a836797f1a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:27:27 +0300 Subject: [PATCH 6/7] wavparse: Fix clipping of size to the file size The size does not include the 8 bytes tag and length, so an additional 8 bytes must be removed here. 8 bytes are always available at this point because otherwise the parsing of the tag and length right above would've failed. Thanks to Antonio Morales for finding and reporting the issue. Fixes GHSL-2024-260 Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/3888 Part-of: --- gst/wavparse/gstwavparse.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index 8a04805ed427..998cbb276dbf 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -1338,10 +1338,11 @@ gst_wavparse_stream_headers (GstWavParse * wav) } /* Clip to upstream size if known */ - if (upstream_size > 0 && size + wav->offset > upstream_size) { + if (upstream_size > 0 && size + 8 + wav->offset > upstream_size) { GST_WARNING_OBJECT (wav, "Clipping chunk size to file size"); g_assert (upstream_size >= wav->offset); - size = upstream_size - wav->offset; + g_assert (upstream_size - wav->offset >= 8); + size = upstream_size - wav->offset - 8; } /* wav is a st00pid format, we don't know for sure where data starts. -- GitLab From 4f381d15014471b026020d0990a5f5a9f420a22b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 4 Oct 2024 13:51:00 +0300 Subject: [PATCH 7/7] wavparse: Check size before reading ds64 chunk Thanks to Antonio Morales for finding and reporting the issue. Fixes GHSL-2024-261 Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/3889 Part-of: --- gst/wavparse/gstwavparse.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index 998cbb276dbf..958868de6d9e 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -1088,6 +1088,11 @@ parse_ds64 (GstWavParse * wav, GstBuffer * buf) guint32 sampleCountLow, sampleCountHigh; gst_buffer_map (buf, &map, GST_MAP_READ); + if (map.size < 6 * 4) { + GST_WARNING_OBJECT (wav, "Too small ds64 chunk (%" G_GSIZE_FORMAT ")", + map.size); + return FALSE; + } dataSizeLow = GST_READ_UINT32_LE (map.data + 2 * 4); dataSizeHigh = GST_READ_UINT32_LE (map.data + 3 * 4); sampleCountLow = GST_READ_UINT32_LE (map.data + 4 * 4); -- GitLab