From eda2cd76db725510f6f6b0c60c2f68362ce98bae Mon Sep 17 00:00:00 2001 From: Harald Kuhr Date: Thu, 19 Nov 2020 20:42:10 +0100 Subject: [PATCH] #574 Fix for possible OOME in Exif metadata. --- .../imageio/plugins/jpeg/JPEGImageReader.java | 61 +++++++++++------- .../imageio/metadata/tiff/TIFFReader.java | 52 ++++++++------- .../imageio/metadata/tiff/TIFFReaderTest.java | 41 ++++++++---- .../resources/exif/exif-bad-interop-oome.bin | Bin 0 -> 12135 bytes 4 files changed, 93 insertions(+), 61 deletions(-) create mode 100644 imageio/imageio-metadata/src/test/resources/exif/exif-bad-interop-oome.bin diff --git a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReader.java b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReader.java index 6fb038a4..fc89031f 100644 --- a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReader.java +++ b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReader.java @@ -30,6 +30,37 @@ package com.twelvemonkeys.imageio.plugins.jpeg; +import java.awt.*; +import java.awt.color.ColorSpace; +import java.awt.color.ICC_ColorSpace; +import java.awt.color.ICC_Profile; +import java.awt.image.*; +import java.io.DataInput; +import java.io.DataInputStream; +import java.io.EOFException; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.SequenceInputStream; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; + +import javax.imageio.IIOException; +import javax.imageio.ImageIO; +import javax.imageio.ImageReadParam; +import javax.imageio.ImageReader; +import javax.imageio.ImageTypeSpecifier; +import javax.imageio.event.IIOReadUpdateListener; +import javax.imageio.event.IIOReadWarningListener; +import javax.imageio.metadata.IIOMetadata; +import javax.imageio.metadata.IIOMetadataFormatImpl; +import javax.imageio.metadata.IIOMetadataNode; +import javax.imageio.spi.ImageReaderSpi; +import javax.imageio.stream.ImageInputStream; + import com.twelvemonkeys.imageio.ImageReaderBase; import com.twelvemonkeys.imageio.color.ColorSpaces; import com.twelvemonkeys.imageio.color.YCbCrConverter; @@ -49,24 +80,6 @@ import com.twelvemonkeys.imageio.util.ProgressListenerBase; import com.twelvemonkeys.lang.Validate; import com.twelvemonkeys.xml.XMLSerializer; -import javax.imageio.*; -import javax.imageio.event.IIOReadUpdateListener; -import javax.imageio.event.IIOReadWarningListener; -import javax.imageio.metadata.IIOMetadata; -import javax.imageio.metadata.IIOMetadataFormatImpl; -import javax.imageio.metadata.IIOMetadataNode; -import javax.imageio.spi.ImageReaderSpi; -import javax.imageio.stream.ImageInputStream; -import javax.imageio.stream.MemoryCacheImageInputStream; -import java.awt.*; -import java.awt.color.ColorSpace; -import java.awt.color.ICC_ColorSpace; -import java.awt.color.ICC_Profile; -import java.awt.image.*; -import java.io.*; -import java.util.List; -import java.util.*; - /** * A JPEG {@code ImageReader} implementation based on the JRE {@code JPEGImageReader}, * that adds support and properly handles cases where the JRE version throws exceptions. @@ -896,16 +909,16 @@ public final class JPEGImageReader extends ImageReaderBase { if (!exifSegments.isEmpty()) { Application exif = exifSegments.get(0); - InputStream data = exif.data(); + int offset = exif.identifier.length() + 2; // Incl. pad - if (data.read() == -1) { // Read pad + if (exif.data.length <= offset) { processWarningOccurred("Exif chunk has no data."); } else { - ImageInputStream stream = new MemoryCacheImageInputStream(data); - return (CompoundDirectory) new TIFFReader().read(stream); - - // TODO: Directory offset of thumbnail is wrong/relative to container stream, causing trouble for the TIFFReader... + // TODO: Consider returning ByteArrayImageInputStream from Segment.data() + try (ImageInputStream stream = new ByteArrayImageInputStream(exif.data, offset, exif.data.length - offset)) { + return (CompoundDirectory) new TIFFReader().read(stream); + } } } diff --git a/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReader.java b/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReader.java index 7dd497c2..6d2cf773 100644 --- a/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReader.java +++ b/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReader.java @@ -30,15 +30,8 @@ package com.twelvemonkeys.imageio.metadata.tiff; -import com.twelvemonkeys.imageio.metadata.Directory; -import com.twelvemonkeys.imageio.metadata.Entry; -import com.twelvemonkeys.imageio.metadata.MetadataReader; -import com.twelvemonkeys.lang.StringUtil; -import com.twelvemonkeys.lang.Validate; +import static com.twelvemonkeys.imageio.metadata.tiff.TIFFEntry.getValueLength; -import javax.imageio.IIOException; -import javax.imageio.ImageIO; -import javax.imageio.stream.ImageInputStream; import java.io.EOFException; import java.io.File; import java.io.IOException; @@ -46,7 +39,15 @@ import java.nio.ByteOrder; import java.nio.charset.StandardCharsets; import java.util.*; -import static com.twelvemonkeys.imageio.metadata.tiff.TIFFEntry.getValueLength; +import javax.imageio.IIOException; +import javax.imageio.ImageIO; +import javax.imageio.stream.ImageInputStream; + +import com.twelvemonkeys.imageio.metadata.Directory; +import com.twelvemonkeys.imageio.metadata.Entry; +import com.twelvemonkeys.imageio.metadata.MetadataReader; +import com.twelvemonkeys.lang.StringUtil; +import com.twelvemonkeys.lang.Validate; /** * TIFFReader @@ -336,7 +337,8 @@ public final class TIFFReader extends MetadataReader { else { long valueOffset = readOffset(pInput); // This is the *value* iff the value size is <= offsetSize - if (length > 0 && length < valueOffset + valueLength) { + // Note: This a precaution + if (count >= Integer.MAX_VALUE || length > 0 && length < valueOffset + valueLength) { value = new EOFException(String.format("TIFF value offset or size too large: %d/%d bytes (length: %d bytes)", valueOffset, valueLength, length)); } else { @@ -367,7 +369,7 @@ public final class TIFFReader extends MetadataReader { long pos = pInput.getStreamPosition(); try { pInput.seek(pOffset); - return readValue(pInput, pType, pCount); + return readValue(pInput, pType, pCount, longOffsets); } catch (EOFException e) { // TODO: Add warning listener API and report problem to client code @@ -379,10 +381,10 @@ public final class TIFFReader extends MetadataReader { } private Object readValueInLine(final ImageInputStream pInput, final short pType, final int pCount) throws IOException { - return readValue(pInput, pType, pCount); + return readValue(pInput, pType, pCount, longOffsets); } - private static Object readValue(final ImageInputStream pInput, final short pType, final int pCount) throws IOException { + private static Object readValue(final ImageInputStream pInput, final short pType, final int pCount, boolean bigTIFF) throws IOException { // TODO: Review value "widening" for the unsigned types. Right now it's inconsistent. Should we leave it to client code? // TODO: New strategy: Leave data as is, instead perform the widening in TIFFEntry.getValue. // TODO: Add getValueByte/getValueUnsignedByte/getValueShort/getValueUnsignedShort/getValueInt/etc... in API. @@ -513,24 +515,24 @@ public final class TIFFReader extends MetadataReader { case TIFF.TYPE_LONG8: case TIFF.TYPE_SLONG8: case TIFF.TYPE_IFD8: - // TODO: Assert BigTiff (version == 43) + if (bigTIFF) { + if (pCount == 1) { + long val = pInput.readLong(); + if (pType != TIFF.TYPE_SLONG8 && val < 0) { + throw new IIOException(String.format("Value > %s", Long.MAX_VALUE)); + } - if (pCount == 1) { - long val = pInput.readLong(); - if (pType != TIFF.TYPE_SLONG8 && val < 0) { - throw new IIOException(String.format("Value > %s", Long.MAX_VALUE)); + return val; } - return val; - } + long[] longs = new long[pCount]; + for (int i = 0; i < pCount; i++) { + longs[i] = pInput.readLong(); + } - long[] longs = new long[pCount]; - for (int i = 0; i < pCount; i++) { - longs[i] = pInput.readLong(); + return longs; } - return longs; - default: // Spec says skip unknown values return new Unknown(pType, pCount, pos); diff --git a/imageio/imageio-metadata/src/test/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReaderTest.java b/imageio/imageio-metadata/src/test/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReaderTest.java index 41f61335..06e38559 100644 --- a/imageio/imageio-metadata/src/test/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReaderTest.java +++ b/imageio/imageio-metadata/src/test/java/com/twelvemonkeys/imageio/metadata/tiff/TIFFReaderTest.java @@ -30,6 +30,20 @@ package com.twelvemonkeys.imageio.metadata.tiff; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.*; + +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; + +import javax.imageio.ImageIO; +import javax.imageio.stream.ImageInputStream; +import javax.imageio.stream.MemoryCacheImageInputStream; + +import org.junit.Test; + import com.twelvemonkeys.imageio.metadata.CompoundDirectory; import com.twelvemonkeys.imageio.metadata.Directory; import com.twelvemonkeys.imageio.metadata.Entry; @@ -38,18 +52,6 @@ import com.twelvemonkeys.imageio.metadata.exif.EXIF; import com.twelvemonkeys.imageio.stream.ByteArrayImageInputStream; import com.twelvemonkeys.imageio.stream.SubImageInputStream; -import org.junit.Test; - -import javax.imageio.ImageIO; -import javax.imageio.stream.ImageInputStream; -import java.io.EOFException; -import java.io.IOException; -import java.io.InputStream; - -import static org.hamcrest.CoreMatchers.instanceOf; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.*; - /** * TIFFReaderTest * @@ -348,6 +350,21 @@ public class TIFFReaderTest extends MetadataReaderAbstractTest { } } + @Test + public void testReadWithoutOOME() throws IOException { + // This EXIF segment from a JPEG contains an Interop IFD, containing a weird value that could be interpreted + // as a huge SLONG8 field (valid for BigTIFF only). + // OutOfMemoryError would only happen if length of stream is not known (ie. reading from underlying stream). + try (InputStream stream = getResource("/exif/exif-bad-interop-oome.bin").openStream()) { + CompoundDirectory directory = (CompoundDirectory) createReader().read(new MemoryCacheImageInputStream(stream)); + assertEquals(2, directory.directoryCount()); + assertEquals(11, directory.getDirectory(0).size()); + assertEquals(6, directory.getDirectory(1).size()); + assertEquals("Picasa", directory.getDirectory(0).getEntryById(TIFF.TAG_SOFTWARE).getValue()); + assertEquals("2020:11:17 16:05:37", directory.getDirectory(0).getEntryById(TIFF.TAG_DATE_TIME).getValueAsString()); + } + } + @Test(timeout = 200) public void testReadCyclicExifWithoutLoopOrOOME() throws IOException { // This EXIF segment has an interesting bug... diff --git a/imageio/imageio-metadata/src/test/resources/exif/exif-bad-interop-oome.bin b/imageio/imageio-metadata/src/test/resources/exif/exif-bad-interop-oome.bin new file mode 100644 index 0000000000000000000000000000000000000000..accc1c69da569ddf3675be3cfabc444424111dd4 GIT binary patch literal 12135 zcmeHN30O_r+h2R1bDA`r21=<@GF0SL8ifp{&?MbLXd;zn&7urZDrrK7P&CP`B2$Kp zjS?DMDpDzlM27Rd>kP#G-|yc0eE;ux{?GS$_p{det#`lgyWTbKwb$O}=4wa)Av6q$ z5hUURlng-}uoR@bAdDnvh=BAc1UW7~19&_aUjVEL^8AoqhoHsPy$g5>U`a@ShagE( z*mg+39$Tnv+y|I;fP;G#5fYq)&@5*kUms4)FtM^_EHIgAGM}+f+XI<_H1>p-VoNx0 z?cwSi=!~?OT1-PtO+!sRhNi9|Q^!zS56u8uVcxz$fehz>H6DR$0yWS$l(B(RqkCXr z9~ajFR^H0-5ZfHb#cqI^T#U=p+RE`3^HCB%qT>5n+Du9kr^T8;h|0yifQcO(83NZn zS%~c~m4XnN%<(}OFqO-f0?ePx@qq?*1-V!pun<>X9%R-g^P_ho5mEsh16TkYmQetA zl1T(CWT1`Ejv+XC2u26EK5H_W#DVYiCAqy|2;D{DVpXB$;G%mDMR>tLog$oL`ra<33ix(8Gvm7YXTMl>zW@`v?Q%(-|F}5=X zVhYj(R0I!4s|o z5^>PRCB={^N|I}{1gb9!81r!$3z!M%72t2=%0m{V0AC(bhA2X5U>~=R1K@PPc;xH{ zyc3rNP9Foji);Uw1VX!ksRc4cfHMH&QMV38zyXi}QIrfgA27IyjsZRcnCDOR5Fefb zpAItTL547IMN``#h}L~%t4@2uG~4FCo>zs0kjt}2s}dZNWtS% zfW3;c84z*IV~J=s>jSqOUPEv_Q1p6UGeGIMTaO}-AU}46*%m%wSBcV6g5w+~{)25{V>SzFtqSX@@V$GudN|+# zhnHtI4N~AQA4laRU~URmL%gw}PAErEqBYRu~ri6M8}*DxzlL=K4$ePn7*vb>#7e_W$ck zg->w6l^j~xp?nwu-)JcR8xz3a;p1}umE52CGn7BHtbfAa0y78C9nR?IEha;){e+>{ z+1F3(KV<(y1egmCzh(cwf=tAZ{E!n2c|ll#jHRMP zaB$$wkZ;ZMN6yx7_jn)xKd^rwg1xG-So%M3f6U+k=>aS(i=P`j!b8-=6gH&Z-i(!# zv^4Y?7S65#z6?8C3!cc|Oc&46)Y8>6(AVY4|Dke!)6OrkUoG&f1^(}}z%8f>eoWx? z0G40hT)jQzl?B-SMynF464IiTlCu0V{DbT#?^kdY zKMrze7lh9VFX3N7)}uP4h^$cv8A4i6C(tYfEd@)6hqR-@`NB5*;p9W~0N&IVAQ#ew zoJ#$THcxP#;Blf=0GFM`cUSVCqo<(Dq8tX@?=@P%wx`TzEp=QFTh^yJ$nmjJHqCE@ z2p{H67mNarF}fhIL-f1Ga<<$K+DV!3yuXVC!5X5kY5A*-CIC;dkFvg(QR(BT9z#qpxHX(JSOcgj382C&70FU4VgK zN;%i}ru$z+oPENd7l21SCve`Ac(Z_~L4&!4-}x!yv0C3iBPAzI95H)-6>L!^~j{0&dh@L?1dx;2F!ND2UlTST4jA8$Pl?EQ@u7RWG(k z+zBm36Y$28prFbU0W>8*0Cfb?(1038o~_M}Sc-_i0g)Nxb>az;MaB`PL#C)0>cy4( zexW~d@Ocd0$1;#H?AfBw707k7x>>#KfY3`y7hD~bSCp5OI}|vSL@YQ6WM61tlKA({ z72Bg<(XSTx)dD}Z0G{!GoOeHSGGHENzzMYidW;%iq8FfNs10@fXzFk0o}yON`M0Hi z6oiwxpQx6=XRa=!BE-f-9U%s(Cke->$Ql;Cvv6oGJ7m>C$OvtQr*akhneI(KfM=x} zy2s9~hQ_Fcb4WO`CgH;Z=tsdZ-V`(gB_VsXf4St9OP~p_Y5$lM(R8#Ntw)*YGJFO4 zGYc8ez70Rw{#g?*mn2;H^qmGrmw34oWM4vX>d(1`!FMcSxJ005=|}?3;D^K6`$*U$ zDZolP8s0f4z&XAiw1gS#KbD{%*onM@-h7T8p}TN!n2RDnj~}e#FAkgwmxr5?1*2}CmitBjgBB3SUn+j2d*b+ffP&;;3gg*~>4znN!}=*6ayI|donKir(h9}mCcH2i*}^Kx)YQ_RtfQ-^ zZ{Y0W>b7F#&-uS>v2XgI$)5MP|M2|(xgW8mf6u8H^BdubV0(dqKA<6AgT*=D zAo1rAl8brcm-UGC6p7C@H8(|^*Jf~*)re*x8ih)wQfLs-Xf!^4VF7+1v0=jmh3R4v z5_B;-U2?em2uZ1t(sa6vlFZ0aqZAbtB}XW$C@ZMQD<~>pBLt}O^YM!c2#6|3(WMmr zF|n>7cwu%x2PlLxBIyK$POz@y_hN!ZaKB^W#)LJUpz`sP6al!aEsjWnLZOgU8Wldx z1K}_9R60#UiBD2Y?ZhruT{G@6zaa; zadq=-uYTcx3)^eAwk~Mi)m9TzQ4@aln##`Liyw~M+nHXW>agGSd9~_O$;u{|4W84* zj?JEM^3ep#69O()9sW(n&!ITf*wn-xulbtZkay zcUL$>^J%{6xu=>wQsU+}-C8hH0#nxx3^UaqLDF+#?9$$RzH3v zaT%>HwQOqY?byYmW!AkAlX>o-XgRghByDT8@2e;MWc53l zlWx>f-CJLkb6!2Kd(GpXGa@3dTjrS^npy1k{z!8pu}#BL%XH2(fvnzs(Yzebq~*t6 z(EMh!SM6{(Zf@~G&t}PmZl#E=Z&eD_%uY=r!e;uJ3Y6VEy!yB8-VqX;Q{vs6>YVnQ zjf)snx1u_H>f+}*b{VSievVEh{Pl(2e(oD1PLA&JOmP;bH*Iz3u|DVc^{k5?EsLHb zK%2f@aLIhfQ)gnYF0`XeE-iRHYo21bclYaM346;dpAO_yu5#LSOgp9Ou=&^HtLJLn zvecTYW?Lb%DzxmB!{mb8iBBTdwa3ek>}jcOshM?4&vgCWFY6cIa$gW(+)^#29GymP z3R<@PLwUlOqgGP(a%ovU7@qO9oU>V5M(j+|LM z(>&}%MPpn`f79x>WZzY`zWQ{x9H*2BAw|AK1|nLIWoi{$nA|$M$BcMqZZ-SCQ3yOV@(D_a*b|Q zeR6*ldT8Y4guRA`AABjc-qYcz`beYbj{g~b10(sp@q7xFp4akk>hzlRc+W1AH*5#r;+FK1Z-Qw*#uCIJ{q&)q2 zu48)QL+jnuz7EBjWA&6$Gc8|e5l_SG3SDPiK3{ZUn{TkJ>%wA5<>5ud?n~)H0xFrg z@xJ{9U(*ghdSrUn*Gqmz;*wQ;kzJ+D1;UAWX*PS2m11#5_qC0iM(C~H)aQ6P)yc5k zs>?f4r@E+U$?hx%mAyC5#XC7MstEs=FK5S>+d);FDVk$RB%jrh59`6nO>#=$(j~Zvd+dT zB^tQ8Sg&(p-t5;|({U-IbJ&7|K1b{nwq%qp>({gFn41t>EZ#;bG81Ah@RNxt6S9(h zlhOHm3%}XGrsW33k56b0^akEgzLBs-&p`Wk)0&<`&SS1`AKiMpfa+Hwb7RiQ&ZR=n z?CN`Gl@cabWn!!1eaMUb34vC%uM~P(<{wyK=PI}R>z%0ZPB%StKo}dZh;B?ymM6G{D}3}t->9i`7|6YYMi~gcDKBD{PFiUCpb{r-pKEkvm0SD zTIki_xY_&L7jJlNb4tx5hxcZBi zv-8`4jsbBGe+wT*V(moZCiCE`+iSHYq-WTc#<#`0jjTPh)bp#YZ|)Lf?;fYGZPQ-~ zTyuRDHvM8?gxrz(ujvjfWRYSXGv@w+S<0z9D3UL=mlfMKUQFF~amj}4$S#ezNQb)( zF{Z=2E45n+4V~8Ac6YJl>&y<&%~KjjYu>+-8L57|yQbudwDSJaO20Mf^GY@l?egId z6}Mbn8>bo-sVb#Vhjb=?atb`1S$lEaVrGs2Q_b1f$X;S>by%x(lt{Y%6_tZm^xw=p z{#t2+eQ}Jj%A*j?nV;Y>zufjc3!T>)>nJ6%dE9=_1dY58%kAX6H$2lgCRd_$xz_rV zUtpzw|GJhOJ<}VD7I&>mJecdD_gnbVir05OD~_2`_5$qe`6da&*O^Q)|=fHou+{+BM)_svN~eru_7{Wsg;wwPVH=J zoTEE+Gz-NJhKw)wzj@YX+UWk?nr@vvvF{4zZ7X>p=q`7&(d^X6=k`@XIX4Yqmo+)>cFF?d&py;}U@xkp%o8!)JC~Q--u}xj=_Oj}{eT(Vo zBNK|Od^L)t7VyW;ooxHLVA_M!5uZB=1r{1C_>y@&L1JFr*;4aYf(iR7Zar=s{n>0_ zT9kaIXLeAr`JBlqUCqqQNk@&mg}1#+OV(wa5NaZWqceSF@})1n3f9(5%b%m}ID2$L zR!ORJqM&+NitjD*D$<@4pVHLe|9ay_ z$0uIR`b8CfgO&Rp+a!PKXNbMHC*N_1Ob z2K|G(?X(o(^51PPUrZRb^-0An<7+r!-Xx$sx_)DHbL&yi!U!- z)1THBT)$KKeUH87;R_z6{*5IY#E_Hw*xbcECv!XZwIxbdKUwMQSsZ*^+hViuqTxo# z2iiMjZ4x~T*JlTC=&5bCKBBezrb3ubXvg%JhNSw! zv$4L;)z4;A+9o%5G#a@T@LlaGcL)${uh4!zT2=Y-p_d!;&gMy)e%9)d(YR)`oto1} zQVWwuxSrNDzL2nSnwR7DoL4PE2Sc2QWG$;Fu^o$p_hdcVHdU)q&LLoln|X9{fK{Kx z{Mm~Hl&s`k%h&ar&Dwe@Eon`+Mza8^RF?I=-d}^6_2Jlrr|NBiT@~VHUsP{M*(fK{ zA`Xdpn$rgZ=IC^fz4c7SU?mIXk0-9~PgARww~bqv%UpO*dv4k)x{uNXeJO$~=v+8IYg%UkC&0EGi?s~Flr0442%H}}P zoFg^+Hrnv#eYTeetNfuAJTb0y)Oqmo-2b7 bj|r0V!aSA%uYp|Liq8Ro%pl+wdS3cpfdE4g literal 0 HcmV?d00001