From 7ab72f016181b0d63f0a89bf6dbe7f8de3d31143 Mon Sep 17 00:00:00 2001 From: Harald Kuhr Date: Tue, 22 May 2018 21:10:57 +0200 Subject: [PATCH] #329 JPEGImageReader endless loop fix --- .../imageio/plugins/jpeg/Application.java | 2 +- .../imageio/plugins/jpeg/JPEGImageReader.java | 19 ++- .../jpeg/JPEGSegmentImageInputStream.java | 31 +++-- .../plugins/jpeg/JPEGImageReaderTest.java | 130 +++++++++++++++++- .../jpeg/JPEGSegmentImageInputStreamTest.java | 28 ++++ .../513f29d0-02a8-11e7-9756-6035edb96e79.jpg | Bin 0 -> 8223 bytes .../51432b02-02a8-11e7-9203-b42b1c43c0c3.jpg | Bin 0 -> 1641 bytes .../5145e95a-02a8-11e7-8372-4787a7307ab8.jpg | Bin 0 -> 7 bytes .../514b20dc-02a8-11e7-92c6-d4fed7b4ebb1.jpg | Bin 0 -> 2743 bytes .../514c48ea-02a8-11e7-8789-bb75321f404f.jpg | Bin 0 -> 232 bytes .../514e4122-02a8-11e7-8c03-0830d60cd585.jpg | Bin 0 -> 1641 bytes .../515e763c-02a8-11e7-85fb-573b41201f05.jpg | Bin 0 -> 2939 bytes .../516c2778-02a8-11e7-9d50-e916ce831200.jpg | Bin 0 -> 8223 bytes .../metadata/jpeg/JPEGSegmentUtil.java | 71 ++++++++-- 14 files changed, 251 insertions(+), 30 deletions(-) create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/513f29d0-02a8-11e7-9756-6035edb96e79.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/51432b02-02a8-11e7-9203-b42b1c43c0c3.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/5145e95a-02a8-11e7-8372-4787a7307ab8.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/514b20dc-02a8-11e7-92c6-d4fed7b4ebb1.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/514c48ea-02a8-11e7-8789-bb75321f404f.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/514e4122-02a8-11e7-8c03-0830d60cd585.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/515e763c-02a8-11e7-85fb-573b41201f05.jpg create mode 100644 imageio/imageio-jpeg/src/test/resources/broken-jpeg/516c2778-02a8-11e7-9d50-e916ce831200.jpg diff --git a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/Application.java b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/Application.java index 80e07237..586c91d8 100644 --- a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/Application.java +++ b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/Application.java @@ -90,7 +90,7 @@ class Application extends Segment { default: // Generic APPn segment - byte[] bytes = new byte[length - 2]; + byte[] bytes = new byte[Math.max(0, length - 2)]; data.readFully(bytes); return new Application(marker, identifier, bytes); } 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 52e420f1..eadd04ca 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 @@ -233,7 +233,14 @@ public final class JPEGImageReader extends ImageReaderBase { @Override public Iterator getImageTypes(int imageIndex) throws IOException { - Iterator types = delegate.getImageTypes(imageIndex); + Iterator types; + try { + types = delegate.getImageTypes(imageIndex); + } + catch (IndexOutOfBoundsException | NegativeArraySizeException ignore) { + types = null; + } + JPEGColorSpace csType = getSourceCSType(getJFIF(), getAdobeDCT(), getSOF()); if (types == null || !types.hasNext() || csType == JPEGColorSpace.CMYK || csType == JPEGColorSpace.YCCK) { @@ -302,7 +309,7 @@ public final class JPEGImageReader extends ImageReaderBase { return rawType; } } - catch (IIOException | NullPointerException ignore) { + catch (IIOException | NullPointerException | ArrayIndexOutOfBoundsException | NegativeArraySizeException ignore) { // Fall through } @@ -933,7 +940,13 @@ public final class JPEGImageReader extends ImageReaderBase { return new JPEGLosslessDecoderWrapper(this).readRaster(segments, imageInput); } - return delegate.readRaster(imageIndex, param); + try { + return delegate.readRaster(imageIndex, param); + } + catch (IndexOutOfBoundsException knownIssue) { + // com.sun.imageio.plugins.jpeg.JPEGBuffer doesn't do proper sanity check of input data. + throw new IIOException("Corrupt JPEG data: Bad segment length", knownIssue); + } } @Override diff --git a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java index e8762f5a..533f308e 100644 --- a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java +++ b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java @@ -40,6 +40,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import static com.twelvemonkeys.imageio.metadata.jpeg.JPEGSegmentUtil.isKnownJPEGMarker; import static com.twelvemonkeys.lang.Validate.notNull; import static java.util.Arrays.copyOf; @@ -105,24 +106,25 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { // Scan forward while (true) { - long realPosition = stream.getStreamPosition(); - int trash = 0; int marker = stream.readUnsignedByte(); - // Skip bad padding before the marker - while (marker != 0xff) { - marker = stream.readUnsignedByte(); - trash++; - realPosition++; - } + while (!isKnownJPEGMarker(marker)) { + marker &= 0xff; - marker = 0xff00 | stream.readUnsignedByte(); + // Skip bad padding before the marker + while (marker != 0xff) { + marker = stream.readUnsignedByte(); + trash++; + } - // Skip over 0xff padding between markers - while (marker == 0xffff) { - realPosition++; marker = 0xff00 | stream.readUnsignedByte(); + + // Skip over 0xff padding between markers + while (marker == 0xffff) { + marker = 0xff00 | stream.readUnsignedByte(); + trash++; + } } if (trash != 0) { @@ -131,6 +133,8 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { processWarningOccured(String.format("Corrupt JPEG data: %d extraneous bytes before marker 0x%02x", trash, marker & 0xff)); } + long realPosition = stream.getStreamPosition() - 2; + // We are now handling all important segments ourselves, except APP1/Exif and APP14/Adobe, // as these segments affects image decoding. boolean appSegmentMarker = isAppSegmentMarker(marker); @@ -380,6 +384,7 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { fetchSegment(); } catch (EOFException ignore) { + segments.add(new Segment(0, segment.realEnd(), segment.end(), Integer.MAX_VALUE * 2L - segment.realEnd())); // This might happen if the segment lengths in the stream are bad. // We MUST leave internal state untouched in this case. // We ignore this exception here, but client code will get @@ -415,7 +420,7 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { repositionAsNecessary(); long bytesLeft = segment.end() - streamPos; // If no more bytes after reposition, we're at EOF - int count = bytesLeft == 0 ? -1 : segment.read(stream, b, off + total, (int) Math.min(len - total, bytesLeft)); + int count = bytesLeft <= 0 ? -1 : segment.read(stream, b, off + total, (int) Math.min(len - total, bytesLeft)); if (count == -1) { // EOF diff --git a/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReaderTest.java b/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReaderTest.java index 350703ad..2f28168e 100644 --- a/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReaderTest.java +++ b/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReaderTest.java @@ -119,8 +119,15 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTestUoqNIa`_OFMaLBsTYViT` z65oJ}1J14W_t5;`C+-?Fj=_fI#?6bDuCw{!^Wy!(&0s_+|@Cs2829S|k$hso8%msBRZ8+m^j6uBMJC&2J zvJt4`SnGP)6=Za7Pou9#eSoYHl%M1@q?07hkjn?c-dJg`y0 z647g8v$as8732=)4PWx=QXR5WElR1_r47Wd>~x1Dh0A8*_88M9M^< zTA(BYP_4j%RX__ry{mU-(0In|Df$03{sTFZ5Pw3w2ht4l7TAx^4nw^GR0(%viX+fdO~0N` ze*skPoFWhNA|a45m>9vmpZSipb)Dp3Egm6HEo<3Eri z2?;GM-hze!)EhvRa7U&%0zK9A>-qE-K)ak%Yf)0nr zH>wXvGzC>NC@p}K0Pb`FNp8?|v4TMXoc=&TN!@e>2?*SQ08A6esgZ*54$#0kmSInTTVPKQuYGcm8AgN)jsH_X-I|2=E f`t^MJ3sS9VkiJI*fH+0{KhP=vHU0zLg=94V5u$4t literal 0 HcmV?d00001 diff --git a/imageio/imageio-jpeg/src/test/resources/broken-jpeg/514e4122-02a8-11e7-8c03-0830d60cd585.jpg b/imageio/imageio-jpeg/src/test/resources/broken-jpeg/514e4122-02a8-11e7-8c03-0830d60cd585.jpg new file mode 100644 index 0000000000000000000000000000000000000000..27032008e7a8cac18c7e2b586f09744411097b57 GIT binary patch literal 1641 zcmex=WElR1_r47Wd>~x1Dh0A8*_88M9M^< zTA(BYP_4j%RX__ry{mU-(0In|Df$03{sTFZ5Pw3w2ht4l7TAx^4nw^GR0(%viX+fdO~0N` ze*skPoFWhNA|alHV|M+V^|AVi&ARVF2lAgy>-e>H#WcU|?vN*5A<6wP4FLxUoWElRxcP z3NRML>FWQrQu6<6{0FL*goHLUkdeIw4MV6mfGXjROmPHys_EDB=`Vm`=bR!B^CBUq zGGdEhu)~ppG#405FxTF@N6_K$^n&UG5=}wX3`$|(B!oL%K$0uX(-kBja0db~O<<;W z0-0#QJq2|1Aj>TEq@_6=sRdZz4D!q}avcsT>Oh%g0(#Xjv@?q=sLG*p%`%!5@7_IFqt7uVIt2g$d=LNt literal 0 HcmV?d00001 diff --git a/imageio/imageio-jpeg/src/test/resources/broken-jpeg/516c2778-02a8-11e7-9d50-e916ce831200.jpg b/imageio/imageio-jpeg/src/test/resources/broken-jpeg/516c2778-02a8-11e7-9d50-e916ce831200.jpg new file mode 100644 index 0000000000000000000000000000000000000000..f9a294d97fa3fddc84e5c04031d9bde1c6ee20e7 GIT binary patch literal 8223 zcmeHMJ#Q015S_~}aEBs;I0OhQ1__D6%^_ih2tkiT<8F&IC@sRp9jOpPGBiOE5^{;s z(4@fy4J};?kP(I|0g4nfNHojr$9?bm&Ub6a2baQevU__ovu|hKo28%VH)!8++Bcn! z13=#o_r{xVPWL}PaMwn?yQ7z2Tk^{d%UaoP{scIr$H2xc90QxrXA^^Qz;U-rw!Gb) z-;+@f0KA=n_w>nd{}7iaz?Od`mS;=IRU=Og8^9)a8czRS?{@I~0km6J9kStcI(&dU z$2Z{OfO8weJv9H${j`s-@bf zWJvF0n0#w<7#{4q@Sra@X2w-C?w7*F0C6E4(MKQmG>W!dVAdNY9+zN!tp)ps5@1KC zE~3%J!G0=&BV%B9XTywUxADL)IoRiG_$`?V=5R_g3~#VRluR$WYYoytj9 z*$Grta891%CYd2ohRW*6Ri|tO8Qq9bH~dSLu9#eSn9_BzM1@q?07hkjTR_}xJg`y0 z647f@v$as8732=)4PWt^QXR5ZnhNdXla2ozV!81@N!P{Xmi-lq(GS4o@-Oo4(ITMzZF; z%W?w|*USZSPpr|2(p<%B%T=s0tRbgEs+K~1qUD{WaiXNWh#YvelJ#$a5fvaGn0 sjp~4$Cz{{xVm#j_tEMm1(%l!c7ib|#m-#FF^%-x5xdwU{UTJs9UqDg-9RL6T literal 0 HcmV?d00001 diff --git a/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/jpeg/JPEGSegmentUtil.java b/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/jpeg/JPEGSegmentUtil.java index ea8db62f..8bf362bb 100644 --- a/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/jpeg/JPEGSegmentUtil.java +++ b/imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/jpeg/JPEGSegmentUtil.java @@ -165,22 +165,24 @@ public final class JPEGSegmentUtil { // int trash = 0; int marker = stream.readUnsignedByte(); - // Skip trash padding before the marker - while (marker != 0xff) { - marker = stream.readUnsignedByte(); + while (!isKnownJPEGMarker(marker)) { + // Skip trash padding before the marker + while (marker != 0xff) { + marker = stream.readUnsignedByte(); // trash++; - } + } // if (trash != 0) { // TODO: Issue warning? // System.err.println("trash: " + trash); // } - marker = 0xff00 | stream.readUnsignedByte(); - - // Skip over 0xff padding between markers - while (marker == 0xffff) { marker = 0xff00 | stream.readUnsignedByte(); + + // Skip over 0xff padding between markers + while (marker == 0xffff) { + marker = 0xff00 | stream.readUnsignedByte(); + } } if ((marker >> 8 & 0xff) != 0xff) { @@ -192,7 +194,7 @@ public final class JPEGSegmentUtil { byte[] data; if (segmentIdentifiers.containsKey(marker)) { - data = new byte[length - 2]; + data = new byte[Math.max(0, length - 2)]; stream.readFully(data); } else { @@ -218,6 +220,57 @@ public final class JPEGSegmentUtil { return new JPEGSegment(marker, data, length); } + public static boolean isKnownJPEGMarker(final int marker) { + switch (marker) { + case JPEG.SOI: + case JPEG.EOI: + case JPEG.DHT: + case JPEG.SOS: + case JPEG.DQT: + case JPEG.COM: + case JPEG.SOF0: + case JPEG.SOF1: + case JPEG.SOF2: + case JPEG.SOF3: + case JPEG.SOF5: + case JPEG.SOF6: + case JPEG.SOF7: + case JPEG.SOF9: + case JPEG.SOF10: + case JPEG.SOF11: + case JPEG.SOF13: + case JPEG.SOF14: + case JPEG.SOF15: + case JPEG.SOF55: + case JPEG.APP0: + case JPEG.APP1: + case JPEG.APP2: + case JPEG.APP3: + case JPEG.APP4: + case JPEG.APP5: + case JPEG.APP6: + case JPEG.APP7: + case JPEG.APP8: + case JPEG.APP9: + case JPEG.APP10: + case JPEG.APP11: + case JPEG.APP12: + case JPEG.APP13: + case JPEG.APP14: + case JPEG.APP15: + case JPEG.DRI: + case JPEG.TEM: + case JPEG.DAC: + case JPEG.DHP: + case JPEG.DNL: + case JPEG.EXP: + case JPEG.LSE: + return true; + default: + return false; + } + } + private static class AllIdsList extends ArrayList { @Override public String toString() {