From 12e7aa58826edbf88b99d67e3bd4c6feaddc45b7 Mon Sep 17 00:00:00 2001 From: Jin Seok Park Date: Fri, 30 Mar 2018 18:07:30 +0900 Subject: [PATCH] ExifInterface: Prevent infinite loop A corrupted image file may create two problems. 1. A corrupted IFD pointer may point to an IFD that has already been read, thus creating an infinite loop and a stack overflow. 2. A corrupted IFD offset value may have a negative value, thus prompting a random reading of the file and creating an infinite loop. This CL addresses these issues. Bug: 63800695 Test: Run cts (ExifInterfaceTest) Change-Id: I706a0c3eae6af8301af69407333ea88e5681df3c --- media/java/android/media/ExifInterface.java | 52 +++++++++++++++------ 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/media/java/android/media/ExifInterface.java b/media/java/android/media/ExifInterface.java index bc0e43b5e9c67..65a18230edf7c 100644 --- a/media/java/android/media/ExifInterface.java +++ b/media/java/android/media/ExifInterface.java @@ -1222,7 +1222,7 @@ public class ExifInterface { TAG_F_NUMBER, TAG_DIGITAL_ZOOM_RATIO, TAG_EXPOSURE_TIME, TAG_SUBJECT_DISTANCE, TAG_GPS_TIMESTAMP)); // Mappings from tag number to IFD type for pointer tags. - private static final HashMap sExifPointerTagMap = new HashMap(); + private static final HashMap sExifPointerTagMap = new HashMap(); // See JPEG File Interchange Format Version 1.02. // The following values are defined for handling JPEG streams. In this implementation, we are @@ -1299,6 +1299,7 @@ public class ExifInterface { private final boolean mIsInputStream; private int mMimeType; private final HashMap[] mAttributes = new HashMap[EXIF_TAGS.length]; + private Set mAttributesOffsets = new HashSet<>(EXIF_TAGS.length); private ByteOrder mExifByteOrder = ByteOrder.BIG_ENDIAN; private boolean mHasThumbnail; // The following values used for indicating a thumbnail position. @@ -2933,8 +2934,9 @@ public class ExifInterface { } // See TIFF 6.0 Section 2: TIFF Structure, Figure 1. short numberOfDirectoryEntry = dataInputStream.readShort(); - if (dataInputStream.mPosition + 12 * numberOfDirectoryEntry > dataInputStream.mLength) { - // Return if the size of entries is too big. + if (dataInputStream.mPosition + 12 * numberOfDirectoryEntry > dataInputStream.mLength + || numberOfDirectoryEntry <= 0) { + // Return if the size of entries is either too big or negative. return; } @@ -3025,7 +3027,7 @@ public class ExifInterface { } // Recursively parse IFD when a IFD pointer tag appears. - Object nextIfdType = sExifPointerTagMap.get(tagNumber); + Integer nextIfdType = sExifPointerTagMap.get(tagNumber); if (DEBUG) { Log.d(TAG, "nextIfdType: " + nextIfdType + " byteCount: " + byteCount); } @@ -3059,9 +3061,20 @@ public class ExifInterface { if (DEBUG) { Log.d(TAG, String.format("Offset: %d, tagName: %s", offset, tag.name)); } + + // Check if the next IFD offset + // 1. Exists within the boundaries of the input stream + // 2. Does not point to a previously read IFD. if (offset > 0L && offset < dataInputStream.mLength) { - dataInputStream.seek(offset); - readImageFileDirectory(dataInputStream, (int) nextIfdType); + if (!mAttributesOffsets.contains((int) offset)) { + // Save offset of current IFD to prevent reading an IFD that is already read + mAttributesOffsets.add(dataInputStream.mPosition); + dataInputStream.seek(offset); + readImageFileDirectory(dataInputStream, nextIfdType); + } else { + Log.w(TAG, "Skip jump into the IFD since it has already been read: " + + "IfdType " + nextIfdType + " (at " + offset + ")"); + } } else { Log.w(TAG, "Skip jump into the IFD since its offset is invalid: " + offset); } @@ -3103,16 +3116,27 @@ public class ExifInterface { if (DEBUG) { Log.d(TAG, String.format("nextIfdOffset: %d", nextIfdOffset)); } - // The next IFD offset needs to be bigger than 8 - // since the first IFD offset is at least 8. - if (nextIfdOffset > 8 && nextIfdOffset < dataInputStream.mLength) { - dataInputStream.seek(nextIfdOffset); - if (mAttributes[IFD_TYPE_THUMBNAIL].isEmpty()) { + // Check if the next IFD offset + // 1. Exists within the boundaries of the input stream + // 2. Does not point to a previously read IFD. + if (nextIfdOffset > 0L && nextIfdOffset < dataInputStream.mLength) { + if (!mAttributesOffsets.contains(nextIfdOffset)) { + // Save offset of current IFD to prevent reading an IFD that is already read. + mAttributesOffsets.add(dataInputStream.mPosition); + dataInputStream.seek(nextIfdOffset); // Do not overwrite thumbnail IFD data if it alreay exists. - readImageFileDirectory(dataInputStream, IFD_TYPE_THUMBNAIL); - } else if (mAttributes[IFD_TYPE_PREVIEW].isEmpty()) { - readImageFileDirectory(dataInputStream, IFD_TYPE_PREVIEW); + if (mAttributes[IFD_TYPE_THUMBNAIL].isEmpty()) { + readImageFileDirectory(dataInputStream, IFD_TYPE_THUMBNAIL); + } else if (mAttributes[IFD_TYPE_PREVIEW].isEmpty()) { + readImageFileDirectory(dataInputStream, IFD_TYPE_PREVIEW); + } + } else { + Log.w(TAG, "Stop reading file since re-reading an IFD may cause an " + + "infinite loop: " + nextIfdOffset); } + } else { + Log.w(TAG, "Stop reading file since a wrong offset may cause an infinite loop: " + + nextIfdOffset); } } }