From 80d781f24346e2ba76e9eedfc943f6013abb2771 Mon Sep 17 00:00:00 2001 From: Su_Laus Date: Tue, 30 Aug 2022 16:56:48 +0200 Subject: [PATCH] (CVE-2022-3599) Revised handling of TIFFTAG_INKNAMES and related TIFFTAG_NUMBEROFINKS value In order to solve the buffer overflow issues related to TIFFTAG_INKNAMES and related TIFFTAG_NUMBEROFINKS value, a revised handling of those tags within LibTiff is proposed: Behaviour for writing: `NumberOfInks` MUST fit to the number of inks in the `InkNames` string. `NumberOfInks` is automatically set when `InkNames` is set. If `NumberOfInks` is different to the number of inks within `InkNames` string, that will be corrected and a warning is issued. If `NumberOfInks` is not equal to samplesperpixel only a warning will be issued. Behaviour for reading: When reading `InkNames` from a TIFF file, the `NumberOfInks` will be set automatically to the number of inks in `InkNames` string. If `NumberOfInks` is different to the number of inks within `InkNames` string, that will be corrected and a warning is issued. If `NumberOfInks` is not equal to samplesperpixel only a warning will be issued. This allows the safe use of the NumberOfInks value to read out the InkNames without buffer overflow This MR will close the following issues: #149, #150, #152, #168 (to be checked), #250, #269, #398 and #456. It also fixes the old bug at http://bugzilla.maptools.org/show_bug.cgi?id=2599, for which the limitation of `NumberOfInks = SPP` was introduced, which is in my opinion not necessary and does not solve the general issue. (cherry picked from commit f00484b9519df933723deb38fff943dc291a793d) --- libtiff/tif_dir.c | 119 ++++++++++++++++++++++++----------------- libtiff/tif_dir.h | 2 + libtiff/tif_dirinfo.c | 2 +- libtiff/tif_dirwrite.c | 5 ++ libtiff/tif_print.c | 4 ++ 5 files changed, 82 insertions(+), 50 deletions(-) diff --git a/libtiff/tif_dir.c b/libtiff/tif_dir.c index e90f14a0..a4295dc9 100644 --- a/libtiff/tif_dir.c +++ b/libtiff/tif_dir.c @@ -136,32 +136,30 @@ setExtraSamples(TIFF* tif, va_list ap, uint32_t* v) } /* - * Confirm we have "samplesperpixel" ink names separated by \0. Returns + * Count ink names separated by \0. Returns * zero if the ink names are not as expected. */ -static uint32_t -checkInkNamesString(TIFF* tif, uint32_t slen, const char* s) +static uint16_t +countInkNamesString(TIFF *tif, uint32_t slen, const char *s) { - TIFFDirectory* td = &tif->tif_dir; - uint16_t i = td->td_samplesperpixel; + uint16_t i = 0; + const char *ep = s + slen; + const char *cp = s; if (slen > 0) { - const char* ep = s+slen; - const char* cp = s; - for (; i > 0; i--) { + do { for (; cp < ep && *cp != '\0'; cp++) {} if (cp >= ep) goto bad; cp++; /* skip \0 */ - } - return ((uint32_t)(cp - s)); + i++; + } while (cp < ep); + return (i); } bad: TIFFErrorExt(tif->tif_clientdata, "TIFFSetField", - "%s: Invalid InkNames value; expecting %"PRIu16" names, found %"PRIu16, - tif->tif_name, - td->td_samplesperpixel, - (uint16_t)(td->td_samplesperpixel-i)); + "%s: Invalid InkNames value; no NUL at given buffer end location %"PRIu32", after %"PRIu16" ink", + tif->tif_name, slen, i); return (0); } @@ -475,13 +473,61 @@ _TIFFVSetField(TIFF* tif, uint32_t tag, va_list ap) _TIFFsetFloatArray(&td->td_refblackwhite, va_arg(ap, float*), 6); break; case TIFFTAG_INKNAMES: - v = (uint16_t) va_arg(ap, uint16_vap); - s = va_arg(ap, char*); - v = checkInkNamesString(tif, v, s); - status = v > 0; - if( v > 0 ) { - _TIFFsetNString(&td->td_inknames, s, v); - td->td_inknameslen = v; + { + v = (uint16_t) va_arg(ap, uint16_vap); + s = va_arg(ap, char*); + uint16_t ninksinstring; + ninksinstring = countInkNamesString(tif, v, s); + status = ninksinstring > 0; + if(ninksinstring > 0 ) { + _TIFFsetNString(&td->td_inknames, s, v); + td->td_inknameslen = v; + /* Set NumberOfInks to the value ninksinstring */ + if (TIFFFieldSet(tif, FIELD_NUMBEROFINKS)) + { + if (td->td_numberofinks != ninksinstring) { + TIFFErrorExt(tif->tif_clientdata, module, + "Warning %s; Tag %s:\n Value %"PRIu16" of NumberOfInks is different from the number of inks %"PRIu16".\n -> NumberOfInks value adapted to %"PRIu16"", + tif->tif_name, fip->field_name, td->td_numberofinks, ninksinstring, ninksinstring); + td->td_numberofinks = ninksinstring; + } + } else { + td->td_numberofinks = ninksinstring; + TIFFSetFieldBit(tif, FIELD_NUMBEROFINKS); + } + if (TIFFFieldSet(tif, FIELD_SAMPLESPERPIXEL)) + { + if (td->td_numberofinks != td->td_samplesperpixel) { + TIFFErrorExt(tif->tif_clientdata, module, + "Warning %s; Tag %s:\n Value %"PRIu16" of NumberOfInks is different from the SamplesPerPixel value %"PRIu16"", + tif->tif_name, fip->field_name, td->td_numberofinks, td->td_samplesperpixel); + } + } + } + } + break; + case TIFFTAG_NUMBEROFINKS: + v = (uint16_t)va_arg(ap, uint16_vap); + /* If InkNames already set also NumberOfInks is set accordingly and should be equal */ + if (TIFFFieldSet(tif, FIELD_INKNAMES)) + { + if (v != td->td_numberofinks) { + TIFFErrorExt(tif->tif_clientdata, module, + "Error %s; Tag %s:\n It is not possible to set the value %"PRIu32" for NumberOfInks\n which is different from the number of inks in the InkNames tag (%"PRIu16")", + tif->tif_name, fip->field_name, v, td->td_numberofinks); + /* Do not set / overwrite number of inks already set by InkNames case accordingly. */ + status = 0; + } + } else { + td->td_numberofinks = (uint16_t)v; + if (TIFFFieldSet(tif, FIELD_SAMPLESPERPIXEL)) + { + if (td->td_numberofinks != td->td_samplesperpixel) { + TIFFErrorExt(tif->tif_clientdata, module, + "Warning %s; Tag %s:\n Value %"PRIu32" of NumberOfInks is different from the SamplesPerPixel value %"PRIu16"", + tif->tif_name, fip->field_name, v, td->td_samplesperpixel); + } + } } break; case TIFFTAG_PERSAMPLE: @@ -915,34 +961,6 @@ _TIFFVGetField(TIFF* tif, uint32_t tag, va_list ap) if (fip->field_bit == FIELD_CUSTOM) { standard_tag = 0; } - - if( standard_tag == TIFFTAG_NUMBEROFINKS ) - { - int i; - for (i = 0; i < td->td_customValueCount; i++) { - uint16_t val; - TIFFTagValue *tv = td->td_customValues + i; - if (tv->info->field_tag != standard_tag) - continue; - if( tv->value == NULL ) - return 0; - val = *(uint16_t *)tv->value; - /* Truncate to SamplesPerPixel, since the */ - /* setting code for INKNAMES assume that there are SamplesPerPixel */ - /* inknames. */ - /* Fixes http://bugzilla.maptools.org/show_bug.cgi?id=2599 */ - if( val > td->td_samplesperpixel ) - { - TIFFWarningExt(tif->tif_clientdata,"_TIFFVGetField", - "Truncating NumberOfInks from %u to %"PRIu16, - val, td->td_samplesperpixel); - val = td->td_samplesperpixel; - } - *va_arg(ap, uint16_t*) = val; - return 1; - } - return 0; - } switch (standard_tag) { case TIFFTAG_SUBFILETYPE: @@ -1124,6 +1142,9 @@ _TIFFVGetField(TIFF* tif, uint32_t tag, va_list ap) case TIFFTAG_INKNAMES: *va_arg(ap, const char**) = td->td_inknames; break; + case TIFFTAG_NUMBEROFINKS: + *va_arg(ap, uint16_t *) = td->td_numberofinks; + break; default: { int i; diff --git a/libtiff/tif_dir.h b/libtiff/tif_dir.h index 09065648..0c251c9e 100644 --- a/libtiff/tif_dir.h +++ b/libtiff/tif_dir.h @@ -117,6 +117,7 @@ typedef struct { /* CMYK parameters */ int td_inknameslen; char* td_inknames; + uint16_t td_numberofinks; /* number of inks in InkNames string */ int td_customValueCount; TIFFTagValue *td_customValues; @@ -174,6 +175,7 @@ typedef struct { #define FIELD_TRANSFERFUNCTION 44 #define FIELD_INKNAMES 46 #define FIELD_SUBIFD 49 +#define FIELD_NUMBEROFINKS 50 /* FIELD_CUSTOM (see tiffio.h) 65 */ /* end of support for well-known tags; codec-private tags follow */ #define FIELD_CODEC 66 /* base of codec-private tags */ diff --git a/libtiff/tif_dirinfo.c b/libtiff/tif_dirinfo.c index c30f569b..a7e78aae 100644 --- a/libtiff/tif_dirinfo.c +++ b/libtiff/tif_dirinfo.c @@ -114,7 +114,7 @@ tiffFields[] = { { TIFFTAG_SUBIFD, -1, -1, TIFF_IFD8, 0, TIFF_SETGET_C16_IFD8, TIFF_SETGET_UNDEFINED, FIELD_SUBIFD, 1, 1, "SubIFD", (TIFFFieldArray*) &tiffFieldArray }, { TIFFTAG_INKSET, 1, 1, TIFF_SHORT, 0, TIFF_SETGET_UINT16, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 0, 0, "InkSet", NULL }, { TIFFTAG_INKNAMES, -1, -1, TIFF_ASCII, 0, TIFF_SETGET_C16_ASCII, TIFF_SETGET_UNDEFINED, FIELD_INKNAMES, 1, 1, "InkNames", NULL }, - { TIFFTAG_NUMBEROFINKS, 1, 1, TIFF_SHORT, 0, TIFF_SETGET_UINT16, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 1, 0, "NumberOfInks", NULL }, + { TIFFTAG_NUMBEROFINKS, 1, 1, TIFF_SHORT, 0, TIFF_SETGET_UINT16, TIFF_SETGET_UNDEFINED, FIELD_NUMBEROFINKS, 1, 0, "NumberOfInks", NULL }, { TIFFTAG_DOTRANGE, 2, 2, TIFF_SHORT, 0, TIFF_SETGET_UINT16_PAIR, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 0, 0, "DotRange", NULL }, { TIFFTAG_TARGETPRINTER, -1, -1, TIFF_ASCII, 0, TIFF_SETGET_ASCII, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 1, 0, "TargetPrinter", NULL }, { TIFFTAG_EXTRASAMPLES, -1, -1, TIFF_SHORT, 0, TIFF_SETGET_C16_UINT16, TIFF_SETGET_UNDEFINED, FIELD_EXTRASAMPLES, 0, 1, "ExtraSamples", NULL }, diff --git a/libtiff/tif_dirwrite.c b/libtiff/tif_dirwrite.c index 2fef6d82..1a00edbf 100644 --- a/libtiff/tif_dirwrite.c +++ b/libtiff/tif_dirwrite.c @@ -708,6 +708,11 @@ TIFFWriteDirectorySec(TIFF* tif, int isimage, int imagedone, uint64_t* pdiroff) if (!TIFFWriteDirectoryTagAscii(tif,&ndir,dir,TIFFTAG_INKNAMES,tif->tif_dir.td_inknameslen,tif->tif_dir.td_inknames)) goto bad; } + if (TIFFFieldSet(tif, FIELD_NUMBEROFINKS)) + { + if (!TIFFWriteDirectoryTagShort(tif, &ndir, dir, TIFFTAG_NUMBEROFINKS, tif->tif_dir.td_numberofinks)) + goto bad; + } if (TIFFFieldSet(tif,FIELD_SUBIFD)) { if (!TIFFWriteDirectoryTagSubifd(tif,&ndir,dir)) diff --git a/libtiff/tif_print.c b/libtiff/tif_print.c index 80a9d90f..1ed90e28 100644 --- a/libtiff/tif_print.c +++ b/libtiff/tif_print.c @@ -401,6 +401,10 @@ TIFFPrintDirectory(TIFF* tif, FILE* fd, long flags) } fputs("\n", fd); } + if (TIFFFieldSet(tif, FIELD_NUMBEROFINKS)) { + fprintf(fd, " NumberOfInks: %d\n", + td->td_numberofinks); + } if (TIFFFieldSet(tif,FIELD_THRESHHOLDING)) { fprintf(fd, " Thresholding: "); switch (td->td_threshholding) {