On Thu, Nov 29, 2012 at 09:46:41AM -0500, Jay Berkenbilt wrote:
> Moritz Muehlenhoff <[email protected]> wrote:
>
> >
> > Hi Jay,
> > another security issue was discovered by Red Hat's Huzaifa S. Sidhpurwala:
> > The Red Hat bug contains the necessary details:
> > https://bugzilla.redhat.com/show_bug.cgi?id=867235
>
> Looking at the bugzilla issue, it's not completely clear to me whether
> this was fixed in 4.0.2 or 4.0.3, and the patch will be pretty different
> for the 3.x versions and the 4.x versions. I'll see what I can do about
> finding time very soon to address this. I'm a little concerned about
> Tom Lane's comment about a behavioral change:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=867235#c6
>
> I'll look at it a little before blindly taking the diff.
I'm attaching the Ubuntu patch for 12.04 (based on 3.9.5-2)
Cheers,
Moritz
Author: Frank Warmerdam <[email protected]>
Description: * libtiff/tif_dir.c, tif_print.c : Remove FIELD_CUSTOM handling
for PAGENUMBER, HALFTONEHINTS, and YCBCRSUBSAMPLING. Implement DOTRANGE
differently. This is to avoid using special TIFFGetField/TIFFSetField rules
for these fields in non-image directories (like EXIF).
Back-ported patch from upstream CVS by Huzaifa S. Sidhpurwala of Red Hat
Security Response Team.
https://bugzilla.redhat.com/show_bug.cgi?id=867235
https://bugzilla.redhat.com/attachment.cgi?id=640578
Index: tiff-3.9.5/libtiff/tif_dir.c
===================================================================
--- tiff-3.9.5.orig/libtiff/tif_dir.c 2010-07-08 09:17:59.000000000 -0700
+++ tiff-3.9.5/libtiff/tif_dir.c 2012-11-30 17:36:11.000000000 -0800
@@ -493,94 +493,90 @@
status = 0;
goto end;
}
+ if (fip->field_tag == TIFFTAG_DOTRANGE
+ && strcmp(fip->field_name,"DotRange") == 0) {
+ /* TODO: This is an evil exception and should not have been
+ handled this way ... likely best if we move it into
+ the directory structure with an explicit field in
+ libtiff 4.1 and assign it a FIELD_ value */
+ uint16 v[2];
+ v[0] = (uint16)va_arg(ap, int);
+ v[1] = (uint16)va_arg(ap, int);
+ _TIFFmemcpy(tv->value, &v, 4);
+ }
+
+ else if (fip->field_passcount
+ || fip->field_writecount == TIFF_VARIABLE
+ || fip->field_writecount == TIFF_VARIABLE2
+ || fip->field_writecount == TIFF_SPP
+ || tv->count > 1) {
- if ((fip->field_passcount
- || fip->field_writecount == TIFF_VARIABLE
- || fip->field_writecount == TIFF_VARIABLE2
- || fip->field_writecount == TIFF_SPP
- || tv->count > 1)
- && fip->field_tag != TIFFTAG_PAGENUMBER
- && fip->field_tag != TIFFTAG_HALFTONEHINTS
- && fip->field_tag != TIFFTAG_YCBCRSUBSAMPLING
- && fip->field_tag != TIFFTAG_DOTRANGE) {
_TIFFmemcpy(tv->value, va_arg(ap, void *),
tv->count * tv_size);
} else {
- /*
- * XXX: The following loop required to handle
- * TIFFTAG_PAGENUMBER, TIFFTAG_HALFTONEHINTS,
- * TIFFTAG_YCBCRSUBSAMPLING and TIFFTAG_DOTRANGE tags.
- * These tags are actually arrays and should be passed as
- * array pointers to TIFFSetField() function, but actually
- * passed as a list of separate values. This behaviour
- * must be changed in the future!
- */
- int i;
+ assert( tv->count == 1 );
char *val = (char *)tv->value;
-
- for (i = 0; i < tv->count; i++, val += tv_size) {
- switch (fip->field_type) {
- case TIFF_BYTE:
- case TIFF_UNDEFINED:
- {
- uint8 v = (uint8)va_arg(ap, int);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_SBYTE:
- {
- int8 v = (int8)va_arg(ap, int);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_SHORT:
- {
- uint16 v = (uint16)va_arg(ap, int);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_SSHORT:
- {
- int16 v = (int16)va_arg(ap, int);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_LONG:
- case TIFF_IFD:
- {
- uint32 v = va_arg(ap, uint32);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_SLONG:
- {
- int32 v = va_arg(ap, int32);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_RATIONAL:
- case TIFF_SRATIONAL:
- case TIFF_FLOAT:
- {
- float v = (float)va_arg(ap, double);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- case TIFF_DOUBLE:
- {
- double v = va_arg(ap, double);
- _TIFFmemcpy(val, &v, tv_size);
- }
- break;
- default:
- _TIFFmemset(val, 0, tv_size);
- status = 0;
- break;
+ switch (fip->field_type) {
+ case TIFF_BYTE:
+ case TIFF_UNDEFINED:
+ {
+ uint8 v = (uint8)va_arg(ap, int);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_SBYTE:
+ {
+ int8 v = (int8)va_arg(ap, int);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_SHORT:
+ {
+ uint16 v = (uint16)va_arg(ap, int);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_SSHORT:
+ {
+ int16 v = (int16)va_arg(ap, int);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_LONG:
+ case TIFF_IFD:
+ {
+ uint32 v = va_arg(ap, uint32);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_SLONG:
+ {
+ int32 v = va_arg(ap, int32);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_RATIONAL:
+ case TIFF_SRATIONAL:
+ case TIFF_FLOAT:
+ {
+ float v = (float)va_arg(ap, double);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ case TIFF_DOUBLE:
+ {
+ double v = va_arg(ap, double);
+ _TIFFmemcpy(val, &v, tv_size);
+ }
+ break;
+ default:
+ _TIFFmemset(val, 0, tv_size);
+ status = 0;
+ break;
}
}
}
}
- }
}
if (status) {
TIFFSetFieldBit(tif, _TIFFFieldWithTag(tif, tag)->field_bit);
@@ -868,75 +864,76 @@
*va_arg(ap, uint16*) = (uint16)tv->count;
*va_arg(ap, void **) = tv->value;
ret_val = 1;
+ } else if (fip->field_tag == TIFFTAG_DOTRANGE
+ && strcmp(fip->field_name,"DotRange") == 0) {
+ /* TODO: This is an evil exception and should not have been
+ handled this way ... likely best if we move it into
+ the directory structure with an explicit field in
+ libtiff 4.1 and assign it a FIELD_ value */
+ *va_arg(ap, uint16*) = ((uint16 *)tv->value)[0];
+ *va_arg(ap, uint16*) = ((uint16 *)tv->value)[1];
+ ret_val = 1;
} else {
- if ((fip->field_type == TIFF_ASCII
+ if (fip->field_type == TIFF_ASCII
|| fip->field_readcount == TIFF_VARIABLE
|| fip->field_readcount == TIFF_VARIABLE2
|| fip->field_readcount == TIFF_SPP
- || tv->count > 1)
- && fip->field_tag != TIFFTAG_PAGENUMBER
- && fip->field_tag != TIFFTAG_HALFTONEHINTS
- && fip->field_tag != TIFFTAG_YCBCRSUBSAMPLING
- && fip->field_tag != TIFFTAG_DOTRANGE) {
+ || tv->count > 1) {
*va_arg(ap, void **) = tv->value;
ret_val = 1;
} else {
- int j;
char *val = (char *)tv->value;
-
- for (j = 0; j < tv->count;
- j++, val += _TIFFDataSize(tv->info->field_type)) {
- switch (fip->field_type) {
- case TIFF_BYTE:
- case TIFF_UNDEFINED:
- *va_arg(ap, uint8*) =
- *(uint8 *)val;
- ret_val = 1;
- break;
- case TIFF_SBYTE:
- *va_arg(ap, int8*) =
- *(int8 *)val;
- ret_val = 1;
- break;
- case TIFF_SHORT:
- *va_arg(ap, uint16*) =
- *(uint16 *)val;
- ret_val = 1;
- break;
- case TIFF_SSHORT:
- *va_arg(ap, int16*) =
- *(int16 *)val;
- ret_val = 1;
- break;
- case TIFF_LONG:
- case TIFF_IFD:
- *va_arg(ap, uint32*) =
- *(uint32 *)val;
- ret_val = 1;
- break;
- case TIFF_SLONG:
- *va_arg(ap, int32*) =
- *(int32 *)val;
- ret_val = 1;
- break;
- case TIFF_RATIONAL:
- case TIFF_SRATIONAL:
- case TIFF_FLOAT:
- *va_arg(ap, float*) =
- *(float *)val;
- ret_val = 1;
- break;
- case TIFF_DOUBLE:
- *va_arg(ap, double*) =
- *(double *)val;
- ret_val = 1;
- break;
- default:
- ret_val = 0;
- break;
- }
- }
- }
+ assert( tv->count == 1 );
+ switch (fip->field_type) {
+ case TIFF_BYTE:
+ case TIFF_UNDEFINED:
+ *va_arg(ap, uint8*) =
+ *(uint8 *)val;
+ ret_val = 1;
+ break;
+ case TIFF_SBYTE:
+ *va_arg(ap, int8*) =
+ *(int8 *)val;
+ ret_val = 1;
+ break;
+ case TIFF_SHORT:
+ *va_arg(ap, uint16*) =
+ *(uint16 *)val;
+ ret_val = 1;
+ break;
+ case TIFF_SSHORT:
+ *va_arg(ap, int16*) =
+ *(int16 *)val;
+ ret_val = 1;
+ break;
+ case TIFF_LONG:
+ case TIFF_IFD:
+ *va_arg(ap, uint32*) =
+ *(uint32 *)val;
+ ret_val = 1;
+ break;
+ case TIFF_SLONG:
+ *va_arg(ap, int32*) =
+ *(int32 *)val;
+ ret_val = 1;
+ break;
+ case TIFF_RATIONAL:
+ case TIFF_SRATIONAL:
+ case TIFF_FLOAT:
+ *va_arg(ap, float*) =
+ *(float *)val;
+ ret_val = 1;
+ break;
+ case TIFF_DOUBLE:
+ *va_arg(ap, double*) =
+ *(double *)val;
+ ret_val = 1;
+ break;
+ default:
+ ret_val = 0;
+ break;
+ }
+ }
}
break;
}
