From 81cfbb6740ee1408336fbc1e60106930ac00840e Mon Sep 17 00:00:00 2001 From: Jason Nelson Date: Wed, 28 Mar 2018 08:52:21 -0700 Subject: [PATCH] Improve overrun handling, variable names, and validate data on construction --- .../MetaData/Profiles/Exif/ExifProfile.cs | 6 +- .../MetaData/Profiles/Exif/ExifReader.cs | 104 +++++++++++------- .../MetaData/Profiles/Exif/ExifValue.cs | 4 +- .../Profiles/Exif/ExifProfileTests.cs | 7 +- .../MetaData/Profiles/Exif/ExifReaderTests.cs | 11 +- 5 files changed, 77 insertions(+), 55 deletions(-) diff --git a/src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs b/src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs index fcedb4a9c..de70c41f5 100644 --- a/src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs +++ b/src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs @@ -265,8 +265,10 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return; } - var reader = new ExifReader(); - this.values = reader.Read(this.data); + var reader = new ExifReader(this.data); + + this.values = reader.ReadValues(); + this.invalidTags = new List(reader.InvalidTags); this.thumbnailOffset = (int)reader.ThumbnailOffset; this.thumbnailLength = (int)reader.ThumbnailLength; diff --git a/src/ImageSharp/MetaData/Profiles/Exif/ExifReader.cs b/src/ImageSharp/MetaData/Profiles/Exif/ExifReader.cs index e4b1efa3b..407030196 100644 --- a/src/ImageSharp/MetaData/Profiles/Exif/ExifReader.cs +++ b/src/ImageSharp/MetaData/Profiles/Exif/ExifReader.cs @@ -19,15 +19,22 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif /// internal sealed class ExifReader { + private delegate TDataType ConverterMethod(ReadOnlySpan data); + private readonly List invalidTags = new List(); - private byte[] exifData; + private readonly byte[] exifData; private int position; private Endianness endianness = Endianness.BigEndian; private uint exifOffset; private uint gpsOffset; private int startIndex; - private delegate TDataType ConverterMethod(ReadOnlySpan data); + public ExifReader(byte[] exifData) + { + DebugGuard.NotNull(exifData, nameof(exifData)); + + this.exifData = exifData; + } /// /// Gets the invalid tags. @@ -56,28 +63,23 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return 0; } - return this.exifData.Length - (int)this.position; + return this.exifData.Length - this.position; } } /// /// Reads and returns the collection of EXIF values. /// - /// The data. /// /// The . /// - public List Read(byte[] data) + public List ReadValues() { - DebugGuard.NotNull(data, nameof(data)); - var values = new List(); - this.exifData = data; - - if (this.GetString(4) == "Exif") + if (this.ReadString(4) == "Exif") { - if (this.ReadShort() != 0) + if (this.ReadUInt16() != 0) { return values; } @@ -89,12 +91,12 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif this.position = 0; } - if (this.GetString(2) == "II") + if (this.ReadString(2) == "II") { this.endianness = Endianness.LittleEndian; } - if (this.ReadShort() != 0x002A) + if (this.ReadUInt16() != 0x002A) { return values; } @@ -163,12 +165,12 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif /// /// The values. /// The index. - private void AddValues(IList values, int index) + private void AddValues(List values, int index) { this.position = this.startIndex + index; - ushort count = this.ReadShort(); + int count = this.ReadUInt16(); - for (ushort i = 0; i < count; i++) + for (int i = 0; i < count; i++) { if (!this.TryReadValue(out ExifValue value)) { @@ -241,10 +243,10 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif case ExifDataType.Long: if (numberOfComponents == 1) { - return this.ConvertToUInt64(buffer); + return this.ConvertToUInt32(buffer); } - return ToArray(dataType, buffer, this.ConvertToUInt64); + return ToArray(dataType, buffer, this.ConvertToUInt32); case ExifDataType.Rational: if (numberOfComponents == 1) { @@ -269,10 +271,10 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif case ExifDataType.SignedLong: if (numberOfComponents == 1) { - return this.ToInt32(buffer); + return this.ConvertToInt32(buffer); } - return ToArray(dataType, buffer, this.ToInt32); + return ToArray(dataType, buffer, this.ConvertToInt32); case ExifDataType.SignedRational: if (numberOfComponents == 1) { @@ -308,6 +310,8 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif private bool TryReadValue(out ExifValue exifValue) { + // 2 | 2 | 4 | 4 + // tag | type | count | value offset if (this.RemainingLength < 12) { exifValue = default; @@ -315,17 +319,21 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return false; } - ExifTag tag = this.ToEnum(this.ReadShort(), ExifTag.Unknown); - ExifDataType dataType = this.ToEnum(this.ReadShort(), ExifDataType.Unknown); - object value; + ExifTag tag = this.ToEnum(this.ReadUInt16(), ExifTag.Unknown); + uint type = this.ReadUInt16(); - if (dataType == ExifDataType.Unknown) + // Ensure that the data type is valid + if (type == 0 || type > 12) { - exifValue = new ExifValue(tag, dataType, null, false); + exifValue = new ExifValue(tag, ExifDataType.Unknown, null, false); return true; } + var dataType = (ExifDataType)type; + + object value; + uint numberOfComponents = this.ReadUInt32(); // Issue #132: ExifDataType == Undefined is treated like a byte array. @@ -337,12 +345,26 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif uint size = numberOfComponents * ExifValue.GetSize(dataType); - this.TryReadSpan(4, out ReadOnlySpan data); + this.TryReadSpan(4, out ReadOnlySpan offsetBuffer); if (size > 4) { int oldIndex = this.position; - this.position = (int)this.ConvertToUInt64(data) + this.startIndex; + + uint newIndex = this.ConvertToUInt32(offsetBuffer) + (uint)this.startIndex; + + // Ensure that the new index does not overrun the data + if (newIndex > int.MaxValue) + { + this.invalidTags.Add(tag); + + exifValue = default; + + return false; + } + + this.position = (int)newIndex; + if (this.RemainingLength < size) { this.invalidTags.Add(tag); @@ -353,14 +375,14 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return false; } - this.TryReadSpan((int)size, out ReadOnlySpan innerData); + this.TryReadSpan((int)size, out ReadOnlySpan dataBuffer); - value = this.ConvertValue(dataType, innerData, numberOfComponents); + value = this.ConvertValue(dataType, dataBuffer, numberOfComponents); this.position = oldIndex; } else { - value = this.ConvertValue(dataType, data, numberOfComponents); + value = this.ConvertValue(dataType, offsetBuffer, numberOfComponents); } exifValue = new ExifValue(tag, dataType, value, isArray: value != null && numberOfComponents > 1); @@ -382,7 +404,7 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif private bool TryReadSpan(int length, out ReadOnlySpan span) { - if (this.position + length > this.exifData.Length || this.position < 0) + if (this.RemainingLength < length) { span = default; @@ -390,7 +412,7 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif } span = new ReadOnlySpan(this.exifData, this.position, length); - + this.position += length; return true; @@ -400,18 +422,18 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif { // Known as Long in Exif Specification return this.TryReadSpan(4, out ReadOnlySpan span) - ? this.ConvertToUInt64(span) + ? this.ConvertToUInt32(span) : default; } - private ushort ReadShort() + private ushort ReadUInt16() { return this.TryReadSpan(2, out ReadOnlySpan span) ? this.ConvertToShort(span) : default; } - private string GetString(int length) + private string ReadString(int length) { if (this.TryReadSpan(length, out ReadOnlySpan span) && span.Length != 0) { @@ -453,7 +475,7 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return *((double*)&intValue); } - private uint ConvertToUInt64(ReadOnlySpan buffer) + private uint ConvertToUInt32(ReadOnlySpan buffer) { // Known as Long in Exif Specification if (buffer.Length < 4) @@ -499,8 +521,8 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return default; } - uint numerator = this.ConvertToUInt64(buffer.Slice(0, 4)); - uint denominator = this.ConvertToUInt64(buffer.Slice(4, 4)); + uint numerator = this.ConvertToUInt32(buffer.Slice(0, 4)); + uint denominator = this.ConvertToUInt32(buffer.Slice(4, 4)); return new Rational(numerator, denominator, false); } @@ -510,7 +532,7 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return unchecked((sbyte)buffer[0]); } - private int ToInt32(ReadOnlySpan buffer) // SignedLong in Exif Specification + private int ConvertToInt32(ReadOnlySpan buffer) // SignedLong in Exif Specification { if (buffer.Length < 4) { @@ -529,8 +551,8 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif return default; } - int numerator = this.ToInt32(buffer.Slice(0, 4)); - int denominator = this.ToInt32(buffer.Slice(4, 4)); + int numerator = this.ConvertToInt32(buffer.Slice(0, 4)); + int denominator = this.ConvertToInt32(buffer.Slice(4, 4)); return new SignedRational(numerator, denominator, false); } diff --git a/src/ImageSharp/MetaData/Profiles/Exif/ExifValue.cs b/src/ImageSharp/MetaData/Profiles/Exif/ExifValue.cs index 6d76619e7..7b51c6d21 100644 --- a/src/ImageSharp/MetaData/Profiles/Exif/ExifValue.cs +++ b/src/ImageSharp/MetaData/Profiles/Exif/ExifValue.cs @@ -51,8 +51,6 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif this.DataType = dataType; this.IsArray = isArray && dataType != ExifDataType.Ascii; this.Value = value; - - // this.CheckValue(value); } /// @@ -698,7 +696,7 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Exif { return description; } - + switch (this.DataType) { case ExifDataType.Ascii: diff --git a/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifProfileTests.cs b/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifProfileTests.cs index 1ffd75c0e..d98c61279 100644 --- a/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifProfileTests.cs +++ b/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifProfileTests.cs @@ -261,11 +261,11 @@ namespace SixLabors.ImageSharp.Tests junk.Append("I"); } - Image image = new Image(100, 100); + var image = new Image(100, 100); image.MetaData.ExifProfile = new ExifProfile(); image.MetaData.ExifProfile.SetValue(ExifTag.ImageDescription, junk.ToString()); - using (MemoryStream memStream = new MemoryStream()) + using (var memStream = new MemoryStream()) { Assert.Throws(() => image.SaveAsJpeg(memStream)); } @@ -274,6 +274,9 @@ namespace SixLabors.ImageSharp.Tests [Fact] public void ExifTypeUndefined() { + // This image contains an 802 byte EXIF profile + // It has a tag with an index offset of 18,481,152 bytes (overrunning the data) + Image image = TestFile.Create(TestImages.Jpeg.Progressive.Bad.ExifUndefType).CreateImage(); Assert.NotNull(image); diff --git a/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifReaderTests.cs b/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifReaderTests.cs index 25ac60831..c9542a98a 100644 --- a/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifReaderTests.cs +++ b/tests/ImageSharp.Tests/MetaData/Profiles/Exif/ExifReaderTests.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. using System.Collections.Generic; -using System.Collections.ObjectModel; using SixLabors.ImageSharp.MetaData.Profiles.Exif; using Xunit; @@ -13,10 +12,9 @@ namespace SixLabors.ImageSharp.Tests [Fact] public void Read_DataIsEmpty_ReturnsEmptyCollection() { - var reader = new ExifReader(); - byte[] data = new byte[] { }; + var reader = new ExifReader(new byte[] { }); - IList result = reader.Read(data); + IList result = reader.ReadValues(); Assert.Equal(0, result.Count); } @@ -24,10 +22,9 @@ namespace SixLabors.ImageSharp.Tests [Fact] public void Read_DataIsMinimal_ReturnsEmptyCollection() { - var reader = new ExifReader(); - byte[] data = new byte[] { 69, 120, 105, 102, 0, 0 }; + var reader = new ExifReader(new byte[] { 69, 120, 105, 102, 0, 0 }); - IList result = reader.Read(data); + IList result = reader.ReadValues(); Assert.Equal(0, result.Count); }