From fd5f7bab830858e57a2baf9d4dd47e5820337b56 Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Tue, 24 Aug 2021 18:04:56 +0000 Subject: Fix MakerNote tag size overflow issues at read time. This is a cherry-pick of https://github.com/libexif/libexif/commit/435e21f05001fb03f9f186fa7cbc69454afd00d1 for CVE-2020-13112 Bug: 194342672 Test: sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.CVE_2020_13112#testPocBug_194342672 Change-Id: Ibdf388bc768213833f8fef9740b3527d46a14a2a Merged-In: Id106e79e829329145d27a93273241b58878bfac3 Signed-off-by: Jayant Chowdhary --- libexif/canon/exif-mnote-data-canon.c | 20 +++++++++++++++++--- libexif/fuji/exif-mnote-data-fuji.c | 22 ++++++++++++++++------ libexif/olympus/exif-mnote-data-olympus.c | 27 +++++++++++++++++++++------ libexif/pentax/exif-mnote-data-pentax.c | 19 +++++++++++++++---- 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/libexif/canon/exif-mnote-data-canon.c b/libexif/canon/exif-mnote-data-canon.c index acf88ab..4396c53 100644 --- a/libexif/canon/exif-mnote-data-canon.c +++ b/libexif/canon/exif-mnote-data-canon.c @@ -32,6 +32,8 @@ #define DEBUG +#define CHECKOVERFLOW(offset,datasize,structsize) (( offset >= datasize) || (structsize > datasize) || (offset > datasize - structsize )) + static void exif_mnote_data_canon_clear (ExifMnoteDataCanon *n) { @@ -209,7 +211,8 @@ exif_mnote_data_canon_load (ExifMnoteData *ne, return; } datao = 6 + n->offset; - if ((datao + 2 < datao) || (datao + 2 < 2) || (datao + 2 > buf_size)) { + + if (CHECKOVERFLOW(datao, buf_size, 2)) { exif_log (ne->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteCanon", "Short MakerNote"); return; @@ -233,7 +236,7 @@ exif_mnote_data_canon_load (ExifMnoteData *ne, tcount = 0; for (i = c, o = datao; i; --i, o += 12) { size_t s; - if ((o + 12 < o) || (o + 12 < 12) || (o + 12 > buf_size)) { + if (CHECKOVERFLOW(o,buf_size,12)) { exif_log (ne->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteCanon", "Short MakerNote"); break; @@ -248,6 +251,16 @@ exif_mnote_data_canon_load (ExifMnoteData *ne, "Loading entry 0x%x ('%s')...", n->entries[tcount].tag, mnote_canon_tag_get_name (n->entries[tcount].tag)); + /* Check if we overflow the multiplication. Use buf_size as the max size for integer overflow detection, + * we will check the buffer sizes closer later. */ + if ( exif_format_get_size (n->entries[tcount].format) && + buf_size / exif_format_get_size (n->entries[tcount].format) < n->entries[tcount].components + ) { + exif_log (ne->log, EXIF_LOG_CODE_CORRUPT_DATA, + "ExifMnoteCanon", "Tag size overflow detected (%u * %lu)", exif_format_get_size (n->entries[tcount].format), n->entries[tcount].components); + continue; + } + /* * Size? If bigger than 4 bytes, the actual data is not * in the entry but somewhere else (offset). @@ -264,7 +277,8 @@ exif_mnote_data_canon_load (ExifMnoteData *ne, } else { size_t dataofs = o + 8; if (s > 4) dataofs = exif_get_long (buf + dataofs, n->order) + 6; - if ((dataofs + s < s) || (dataofs + s < dataofs) || (dataofs + s > buf_size)) { + + if (CHECKOVERFLOW(dataofs, buf_size, s)) { exif_log (ne->log, EXIF_LOG_CODE_DEBUG, "ExifMnoteCanon", "Tag data past end of buffer (%zu > %u)", diff --git a/libexif/fuji/exif-mnote-data-fuji.c b/libexif/fuji/exif-mnote-data-fuji.c index a9949e1..11ff8c3 100644 --- a/libexif/fuji/exif-mnote-data-fuji.c +++ b/libexif/fuji/exif-mnote-data-fuji.c @@ -28,6 +28,8 @@ #include "exif-mnote-data-fuji.h" +#define CHECKOVERFLOW(offset,datasize,structsize) (( offset >= datasize) || (structsize > datasize) || (offset > datasize - structsize )) + struct _MNoteFujiDataPrivate { ExifByteOrder order; }; @@ -162,7 +164,7 @@ exif_mnote_data_fuji_load (ExifMnoteData *en, return; } datao = 6 + n->offset; - if ((datao + 12 < datao) || (datao + 12 < 12) || (datao + 12 > buf_size)) { + if (CHECKOVERFLOW(datao, buf_size, 12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataFuji", "Short MakerNote"); return; @@ -170,8 +172,7 @@ exif_mnote_data_fuji_load (ExifMnoteData *en, n->order = EXIF_BYTE_ORDER_INTEL; datao += exif_get_long (buf + datao + 8, EXIF_BYTE_ORDER_INTEL); - if ((datao + 2 < datao) || (datao + 2 < 2) || - (datao + 2 > buf_size)) { + if (CHECKOVERFLOW(datao, buf_size, 2)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataFuji", "Short MakerNote"); return; @@ -195,7 +196,7 @@ exif_mnote_data_fuji_load (ExifMnoteData *en, tcount = 0; for (i = c, o = datao; i; --i, o += 12) { size_t s; - if ((o + 12 < o) || (o + 12 < 12) || (o + 12 > buf_size)) { + if (CHECKOVERFLOW(o, buf_size, 12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataFuji", "Short MakerNote"); break; @@ -210,6 +211,16 @@ exif_mnote_data_fuji_load (ExifMnoteData *en, "Loading entry 0x%x ('%s')...", n->entries[tcount].tag, mnote_fuji_tag_get_name (n->entries[tcount].tag)); + /* Check if we overflow the multiplication. Use buf_size as the max size for integer overflow detection, + * we will check the buffer sizes closer later. */ + if ( exif_format_get_size (n->entries[tcount].format) && + buf_size / exif_format_get_size (n->entries[tcount].format) < n->entries[tcount].components + ) { + exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, + "ExifMnoteDataFuji", "Tag size overflow detected (%u * %lu)", exif_format_get_size (n->entries[tcount].format), n->entries[tcount].components); + continue; + } + /* * Size? If bigger than 4 bytes, the actual data is not * in the entry but somewhere else (offset). @@ -221,8 +232,7 @@ exif_mnote_data_fuji_load (ExifMnoteData *en, if (s > 4) /* The data in this case is merely a pointer */ dataofs = exif_get_long (buf + dataofs, n->order) + 6 + n->offset; - if ((dataofs + s < dataofs) || (dataofs + s < s) || - (dataofs + s >= buf_size)) { + if (CHECKOVERFLOW(dataofs, buf_size, s)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataFuji", "Tag data past end of " "buffer (%zu >= %u)", dataofs + s, buf_size); diff --git a/libexif/olympus/exif-mnote-data-olympus.c b/libexif/olympus/exif-mnote-data-olympus.c index f4ccbb0..e7bf984 100644 --- a/libexif/olympus/exif-mnote-data-olympus.c +++ b/libexif/olympus/exif-mnote-data-olympus.c @@ -37,6 +37,8 @@ */ /*#define EXIF_OVERCOME_SANYO_OFFSET_BUG */ +#define CHECKOVERFLOW(offset,datasize,structsize) (( offset >= datasize) || (structsize > datasize) || (offset > datasize - structsize )) + static enum OlympusVersion exif_mnote_data_olympus_identify_variant (const unsigned char *buf, unsigned int buf_size); @@ -247,7 +249,8 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, return; } o2 = 6 + n->offset; /* Start of interesting data */ - if ((o2 + 10 < o2) || (o2 + 10 < 10) || (o2 + 10 > buf_size)) { + + if (CHECKOVERFLOW(o2,buf_size,10)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataOlympus", "Short MakerNote"); return; @@ -303,6 +306,8 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, /* Olympus S760, S770 */ datao = o2; o2 += 8; + + if (CHECKOVERFLOW(o2,buf_size,4)) return; exif_log (en->log, EXIF_LOG_CODE_DEBUG, "ExifMnoteDataOlympus", "Parsing Olympus maker note v2 (0x%02x, %02x, %02x, %02x)...", buf[o2], buf[o2 + 1], buf[o2 + 2], buf[o2 + 3]); @@ -347,6 +352,7 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, case nikonV2: o2 += 6; if (o2 >= buf_size) return; + if (CHECKOVERFLOW(o2,buf_size,12)) return; exif_log (en->log, EXIF_LOG_CODE_DEBUG, "ExifMnoteDataOlympus", "Parsing Nikon maker note v2 (0x%02x, %02x, %02x, " "%02x, %02x, %02x, %02x, %02x)...", @@ -406,7 +412,8 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, } /* Sanity check the offset */ - if ((o2 + 2 < o2) || (o2 + 2 < 2) || (o2 + 2 > buf_size)) { + + if (CHECKOVERFLOW(o2,buf_size,2)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteOlympus", "Short MakerNote"); return; @@ -430,7 +437,7 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, tcount = 0; for (i = c, o = o2; i; --i, o += 12) { size_t s; - if ((o + 12 < o) || (o + 12 < 12) || (o + 12 > buf_size)) { + if (CHECKOVERFLOW(o, buf_size, 12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteOlympus", "Short MakerNote"); break; @@ -451,6 +458,15 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, n->entries[tcount].components, (int)exif_format_get_size(n->entries[tcount].format)); */ + /* Check if we overflow the multiplication. Use buf_size as the max size for integer overflow detection, + * we will check the buffer sizes closer later. */ + if (exif_format_get_size (n->entries[tcount].format) && + buf_size / exif_format_get_size (n->entries[tcount].format) < n->entries[tcount].components + ) { + exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteOlympus", "Tag size overflow detected (%u * %lu)", exif_format_get_size (n->entries[tcount].format), n->entries[tcount].components); + continue; + } + /* * Size? If bigger than 4 bytes, the actual data is not * in the entry but somewhere else (offset). @@ -469,7 +485,7 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, * tag in its MakerNote. The offset is actually the absolute * position in the file instead of the position within the IFD. */ - if (dataofs + s > buf_size && n->version == sanyoV1) { + if (dataofs > (buf_size - s) && n->version == sanyoV1) { /* fix pointer */ dataofs -= datao + 6; exif_log (en->log, EXIF_LOG_CODE_DEBUG, @@ -478,8 +494,7 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, } #endif } - if ((dataofs + s < dataofs) || (dataofs + s < s) || - (dataofs + s > buf_size)) { + if (CHECKOVERFLOW(dataofs, buf_size, s)) { exif_log (en->log, EXIF_LOG_CODE_DEBUG, "ExifMnoteOlympus", "Tag data past end of buffer (%zu > %u)", diff --git a/libexif/pentax/exif-mnote-data-pentax.c b/libexif/pentax/exif-mnote-data-pentax.c index 38fbf64..f9eb69c 100644 --- a/libexif/pentax/exif-mnote-data-pentax.c +++ b/libexif/pentax/exif-mnote-data-pentax.c @@ -28,6 +28,8 @@ #include #include +#define CHECKOVERFLOW(offset,datasize,structsize) (( offset >= datasize) || (structsize > datasize) || (offset > datasize - structsize )) + static void exif_mnote_data_pentax_clear (ExifMnoteDataPentax *n) { @@ -224,7 +226,7 @@ exif_mnote_data_pentax_load (ExifMnoteData *en, return; } datao = 6 + n->offset; - if ((datao + 8 < datao) || (datao + 8 < 8) || (datao + 8 > buf_size)) { + if (CHECKOVERFLOW(datao, buf_size, 8)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataPentax", "Short MakerNote"); return; @@ -277,7 +279,7 @@ exif_mnote_data_pentax_load (ExifMnoteData *en, tcount = 0; for (i = c, o = datao; i; --i, o += 12) { size_t s; - if ((o + 12 < o) || (o + 12 < 12) || (o + 12 > buf_size)) { + if (CHECKOVERFLOW(o,buf_size,12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataPentax", "Short MakerNote"); break; @@ -292,6 +294,16 @@ exif_mnote_data_pentax_load (ExifMnoteData *en, "Loading entry 0x%x ('%s')...", n->entries[tcount].tag, mnote_pentax_tag_get_name (n->entries[tcount].tag)); + /* Check if we overflow the multiplication. Use buf_size as the max size for integer overflow detection, + * we will check the buffer sizes closer later. */ + if ( exif_format_get_size (n->entries[tcount].format) && + buf_size / exif_format_get_size (n->entries[tcount].format) < n->entries[tcount].components + ) { + exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, + "ExifMnoteDataPentax", "Tag size overflow detected (%u * %lu)", exif_format_get_size (n->entries[tcount].format), n->entries[tcount].components); + break; + } + /* * Size? If bigger than 4 bytes, the actual data is not * in the entry but somewhere else (offset). @@ -304,8 +316,7 @@ exif_mnote_data_pentax_load (ExifMnoteData *en, if (s > 4) /* The data in this case is merely a pointer */ dataofs = exif_get_long (buf + dataofs, n->order) + 6; - if ((dataofs + s < dataofs) || (dataofs + s < s) || - (dataofs + s > buf_size)) { + if (CHECKOVERFLOW(dataofs, buf_size, s)) { exif_log (en->log, EXIF_LOG_CODE_DEBUG, "ExifMnoteDataPentax", "Tag data past end " "of buffer (%zu > %u)", dataofs + s, buf_size); -- cgit v1.2.3 From 4ceb535b530fd8d0504c9df65c99045a71e12232 Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Mon, 30 Aug 2021 22:12:01 +0000 Subject: Ensure MakeNote data pointers are initialized with NULL. This is a cherry-pick of https://github.com/libexif/libexif/commit/ec412aa4583ad71ecabb967d3c77162760169d1f Bug: 196085005 Test: sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.CVE_2020_13113#testPocBug_196085005 Change-Id: Iaed1a1161e4c026bee24337a0ef5f34d2efdb3cf Merged-In: Id106e79e829329145d27a93273241b58878bfac3 Signed-off-by: Jayant Chowdhary --- libexif/canon/exif-mnote-data-canon.c | 2 ++ libexif/fuji/exif-mnote-data-fuji.c | 2 ++ libexif/olympus/exif-mnote-data-olympus.c | 2 ++ libexif/pentax/exif-mnote-data-pentax.c | 2 ++ 4 files changed, 8 insertions(+) diff --git a/libexif/canon/exif-mnote-data-canon.c b/libexif/canon/exif-mnote-data-canon.c index 4396c53..6d97930 100644 --- a/libexif/canon/exif-mnote-data-canon.c +++ b/libexif/canon/exif-mnote-data-canon.c @@ -236,6 +236,8 @@ exif_mnote_data_canon_load (ExifMnoteData *ne, tcount = 0; for (i = c, o = datao; i; --i, o += 12) { size_t s; + + memset(&n->entries[tcount], 0, sizeof(MnoteCanonEntry)); if (CHECKOVERFLOW(o,buf_size,12)) { exif_log (ne->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteCanon", "Short MakerNote"); diff --git a/libexif/fuji/exif-mnote-data-fuji.c b/libexif/fuji/exif-mnote-data-fuji.c index 11ff8c3..3f3091b 100644 --- a/libexif/fuji/exif-mnote-data-fuji.c +++ b/libexif/fuji/exif-mnote-data-fuji.c @@ -196,6 +196,8 @@ exif_mnote_data_fuji_load (ExifMnoteData *en, tcount = 0; for (i = c, o = datao; i; --i, o += 12) { size_t s; + + memset(&n->entries[tcount], 0, sizeof(MnoteFujiEntry)); if (CHECKOVERFLOW(o, buf_size, 12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataFuji", "Short MakerNote"); diff --git a/libexif/olympus/exif-mnote-data-olympus.c b/libexif/olympus/exif-mnote-data-olympus.c index e7bf984..493463b 100644 --- a/libexif/olympus/exif-mnote-data-olympus.c +++ b/libexif/olympus/exif-mnote-data-olympus.c @@ -437,6 +437,8 @@ exif_mnote_data_olympus_load (ExifMnoteData *en, tcount = 0; for (i = c, o = o2; i; --i, o += 12) { size_t s; + + memset(&n->entries[tcount], 0, sizeof(MnoteOlympusEntry)); if (CHECKOVERFLOW(o, buf_size, 12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteOlympus", "Short MakerNote"); diff --git a/libexif/pentax/exif-mnote-data-pentax.c b/libexif/pentax/exif-mnote-data-pentax.c index f9eb69c..b4722d6 100644 --- a/libexif/pentax/exif-mnote-data-pentax.c +++ b/libexif/pentax/exif-mnote-data-pentax.c @@ -279,6 +279,8 @@ exif_mnote_data_pentax_load (ExifMnoteData *en, tcount = 0; for (i = c, o = datao; i; --i, o += 12) { size_t s; + + memset(&n->entries[tcount], 0, sizeof(MnotePentaxEntry)); if (CHECKOVERFLOW(o,buf_size,12)) { exif_log (en->log, EXIF_LOG_CODE_CORRUPT_DATA, "ExifMnoteDataPentax", "Short MakerNote"); -- cgit v1.2.3 From c9da78d8d9f302c767b366ef256e24fa32f8784f Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Fri, 12 Nov 2021 18:22:59 +0000 Subject: Zero initialize ExifMnoteData during construction with exif_mnote_data__new. This is in order to not have an uninitialized 'mem' pointer in parent ExifMnoteData after construction, when a non default ExifMem is used. Bug: 205915333 Bug: 196085005 Test: create exif_mnote_data__new with non default exif mem using malloc debug; use exif_mem pointer from previously created ExifMnoteData, client app doesn't crash. Change-Id: I35a393cdfb03755109aaa8f725b0792aef359dc6 Merged-In: Id106e79e829329145d27a93273241b58878bfac3 Signed-off-by: Jayant Chowdhary --- libexif/canon/exif-mnote-data-canon.c | 2 ++ libexif/fuji/exif-mnote-data-fuji.c | 2 ++ libexif/olympus/exif-mnote-data-olympus.c | 2 ++ libexif/pentax/exif-mnote-data-pentax.c | 2 ++ 4 files changed, 8 insertions(+) diff --git a/libexif/canon/exif-mnote-data-canon.c b/libexif/canon/exif-mnote-data-canon.c index 6d97930..3a0778c 100644 --- a/libexif/canon/exif-mnote-data-canon.c +++ b/libexif/canon/exif-mnote-data-canon.c @@ -384,6 +384,8 @@ exif_mnote_data_canon_new (ExifMem *mem, ExifDataOption o) if (!d) return NULL; + memset(d, 0, sizeof(ExifMnoteDataCanon)); + exif_mnote_data_construct (d, mem); /* Set up function pointers */ diff --git a/libexif/fuji/exif-mnote-data-fuji.c b/libexif/fuji/exif-mnote-data-fuji.c index 3f3091b..ce70bb6 100644 --- a/libexif/fuji/exif-mnote-data-fuji.c +++ b/libexif/fuji/exif-mnote-data-fuji.c @@ -342,6 +342,8 @@ exif_mnote_data_fuji_new (ExifMem *mem) d = exif_mem_alloc (mem, sizeof (ExifMnoteDataFuji)); if (!d) return NULL; + memset(d, 0, sizeof(ExifMnoteDataFuji)); + exif_mnote_data_construct (d, mem); /* Set up function pointers */ diff --git a/libexif/olympus/exif-mnote-data-olympus.c b/libexif/olympus/exif-mnote-data-olympus.c index 493463b..f11616c 100644 --- a/libexif/olympus/exif-mnote-data-olympus.c +++ b/libexif/olympus/exif-mnote-data-olympus.c @@ -657,6 +657,8 @@ exif_mnote_data_olympus_new (ExifMem *mem) d = exif_mem_alloc (mem, sizeof (ExifMnoteDataOlympus)); if (!d) return NULL; + memset(d, 0, sizeof(ExifMnoteDataOlympus)); + exif_mnote_data_construct (d, mem); /* Set up function pointers */ diff --git a/libexif/pentax/exif-mnote-data-pentax.c b/libexif/pentax/exif-mnote-data-pentax.c index b4722d6..3676563 100644 --- a/libexif/pentax/exif-mnote-data-pentax.c +++ b/libexif/pentax/exif-mnote-data-pentax.c @@ -443,6 +443,8 @@ exif_mnote_data_pentax_new (ExifMem *mem) d = exif_mem_alloc (mem, sizeof (ExifMnoteDataPentax)); if (!d) return NULL; + memset(d, 0, sizeof(ExifMnoteDataPentax)); + exif_mnote_data_construct (d, mem); /* Set up function pointers */ -- cgit v1.2.3