From 38b5da4e19359cb8353ba5d7a5ebcb0d87dab568 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Sat, 15 Jan 2022 20:18:42 +0100 Subject: [PATCH 1/7] Ignore invalid iptc tag values #1942 --- .../Metadata/Profiles/IPTC/IptcProfile.cs | 41 ++++--- .../Profiles/IPTC/IptcTagExtensions.cs | 103 +++++++++--------- .../Metadata/Profiles/IPTC/IptcValue.cs | 2 +- 3 files changed, 71 insertions(+), 75 deletions(-) diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs index e31842c537..d07cb11c92 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs @@ -16,6 +16,10 @@ public sealed class IptcProfile : IDeepCloneable { private Collection values; + private const byte IptcTagMarkerByte = 0x1c; + + private const uint MaxStandardDataTagSize = 0x7FFF; + /// /// Initializes a new instance of the class. /// @@ -78,7 +82,7 @@ public IEnumerable Values } /// - public IptcProfile DeepClone() => new IptcProfile(this); + public IptcProfile DeepClone() => new(this); /// /// Returns all values with the specified tag. @@ -207,7 +211,7 @@ public void SetDateTimeValue(IptcTag tag, DateTimeOffset dateTimeOffset) throw new ArgumentException("iptc tag is not a time or date type"); } - var formattedDate = tag.IsDate() + string formattedDate = tag.IsDate() ? dateTimeOffset.ToString("yyyyMMdd", System.Globalization.CultureInfo.InvariantCulture) : dateTimeOffset.ToString("HHmmsszzzz", System.Globalization.CultureInfo.InvariantCulture) .Replace(":", string.Empty); @@ -231,7 +235,7 @@ public void SetDateTimeValue(IptcTag tag, DateTimeOffset dateTimeOffset) /// public void UpdateData() { - var length = 0; + int length = 0; foreach (IptcValue value in this.Values) { length += value.Length + 5; @@ -242,7 +246,7 @@ public void UpdateData() int i = 0; foreach (IptcValue value in this.Values) { - this.Data[i++] = 28; + this.Data[i++] = IptcTagMarkerByte; this.Data[i++] = 2; this.Data[i++] = (byte)value.Tag; this.Data[i++] = (byte)(value.Length >> 8); @@ -264,34 +268,29 @@ private void Initialize() this.values = new Collection(); - if (this.Data == null || this.Data[0] != 0x1c) + if (this.Data == null || this.Data[0] != IptcTagMarkerByte) { return; } - int i = 0; - while (i + 4 < this.Data.Length) + int offset = 0; + while (offset + 4 < this.Data.Length) { - if (this.Data[i++] != 28) - { - continue; - } - - i++; - - var tag = (IptcTag)this.Data[i++]; + bool isValidTagMarker = this.Data[offset++] == IptcTagMarkerByte; + byte recordType = this.Data[offset++]; + var tag = (IptcTag)this.Data[offset++]; - int count = BinaryPrimitives.ReadInt16BigEndian(this.Data.AsSpan(i, 2)); - i += 2; + uint byteCount = BinaryPrimitives.ReadUInt16BigEndian(this.Data.AsSpan(offset, 2)); + offset += 2; - var iptcData = new byte[count]; - if ((count > 0) && (i + count <= this.Data.Length)) + if (isValidTagMarker && byteCount > 0 && (offset + byteCount <= this.Data.Length)) { - Buffer.BlockCopy(this.Data, i, iptcData, 0, count); + var iptcData = new byte[byteCount]; + Buffer.BlockCopy(this.Data, offset, iptcData, 0, (int)byteCount); this.values.Add(new IptcValue(tag, iptcData, false)); } - i += count; + offset += (int)byteCount; } } } diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcTagExtensions.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcTagExtensions.cs index b670591df7..4b18add74e 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcTagExtensions.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcTagExtensions.cs @@ -13,60 +13,57 @@ public static class IptcTagExtensions /// /// The tag to check the max length for. /// The maximum length. - public static int MaxLength(this IptcTag tag) + public static int MaxLength(this IptcTag tag) => tag switch { - return tag switch - { - IptcTag.RecordVersion => 2, - IptcTag.ObjectType => 67, - IptcTag.ObjectAttribute => 68, - IptcTag.Name => 64, - IptcTag.EditStatus => 64, - IptcTag.EditorialUpdate => 2, - IptcTag.Urgency => 1, - IptcTag.SubjectReference => 236, - IptcTag.Category => 3, - IptcTag.SupplementalCategories => 32, - IptcTag.FixtureIdentifier => 32, - IptcTag.Keywords => 64, - IptcTag.LocationCode => 3, - IptcTag.LocationName => 64, - IptcTag.ReleaseDate => 8, - IptcTag.ReleaseTime => 11, - IptcTag.ExpirationDate => 8, - IptcTag.ExpirationTime => 11, - IptcTag.SpecialInstructions => 256, - IptcTag.ActionAdvised => 2, - IptcTag.ReferenceService => 10, - IptcTag.ReferenceDate => 8, - IptcTag.ReferenceNumber => 8, - IptcTag.CreatedDate => 8, - IptcTag.CreatedTime => 11, - IptcTag.DigitalCreationDate => 8, - IptcTag.DigitalCreationTime => 11, - IptcTag.OriginatingProgram => 32, - IptcTag.ProgramVersion => 10, - IptcTag.ObjectCycle => 1, - IptcTag.Byline => 32, - IptcTag.BylineTitle => 32, - IptcTag.City => 32, - IptcTag.SubLocation => 32, - IptcTag.ProvinceState => 32, - IptcTag.CountryCode => 3, - IptcTag.Country => 64, - IptcTag.OriginalTransmissionReference => 32, - IptcTag.Headline => 256, - IptcTag.Credit => 32, - IptcTag.Source => 32, - IptcTag.CopyrightNotice => 128, - IptcTag.Contact => 128, - IptcTag.Caption => 2000, - IptcTag.CaptionWriter => 32, - IptcTag.ImageType => 2, - IptcTag.ImageOrientation => 1, - _ => 256 - }; - } + IptcTag.RecordVersion => 2, + IptcTag.ObjectType => 67, + IptcTag.ObjectAttribute => 68, + IptcTag.Name => 64, + IptcTag.EditStatus => 64, + IptcTag.EditorialUpdate => 2, + IptcTag.Urgency => 1, + IptcTag.SubjectReference => 236, + IptcTag.Category => 3, + IptcTag.SupplementalCategories => 32, + IptcTag.FixtureIdentifier => 32, + IptcTag.Keywords => 64, + IptcTag.LocationCode => 3, + IptcTag.LocationName => 64, + IptcTag.ReleaseDate => 8, + IptcTag.ReleaseTime => 11, + IptcTag.ExpirationDate => 8, + IptcTag.ExpirationTime => 11, + IptcTag.SpecialInstructions => 256, + IptcTag.ActionAdvised => 2, + IptcTag.ReferenceService => 10, + IptcTag.ReferenceDate => 8, + IptcTag.ReferenceNumber => 8, + IptcTag.CreatedDate => 8, + IptcTag.CreatedTime => 11, + IptcTag.DigitalCreationDate => 8, + IptcTag.DigitalCreationTime => 11, + IptcTag.OriginatingProgram => 32, + IptcTag.ProgramVersion => 10, + IptcTag.ObjectCycle => 1, + IptcTag.Byline => 32, + IptcTag.BylineTitle => 32, + IptcTag.City => 32, + IptcTag.SubLocation => 32, + IptcTag.ProvinceState => 32, + IptcTag.CountryCode => 3, + IptcTag.Country => 64, + IptcTag.OriginalTransmissionReference => 32, + IptcTag.Headline => 256, + IptcTag.Credit => 32, + IptcTag.Source => 32, + IptcTag.CopyrightNotice => 128, + IptcTag.Contact => 128, + IptcTag.Caption => 2000, + IptcTag.CaptionWriter => 32, + IptcTag.ImageType => 2, + IptcTag.ImageOrientation => 1, + _ => 256 + }; /// /// Determines if the given tag can be repeated according to the specification. diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcValue.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcValue.cs index 9e409ca064..5ba81bea70 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcValue.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcValue.cs @@ -101,7 +101,7 @@ public string Value byte[] valueBytes; if (this.Strict && value.Length > maxLength) { - var cappedValue = value.Substring(0, maxLength); + string cappedValue = value.Substring(0, maxLength); valueBytes = this.encoding.GetBytes(cappedValue); // It is still possible that the bytes of the string exceed the limit. From c476caace62e230a2bce92b97646bdddbb06c64a Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Sat, 15 Jan 2022 20:43:20 +0100 Subject: [PATCH 2/7] Add test for issue #1942 --- .../Formats/Jpg/JpegDecoderTests.Metadata.cs | 12 ++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + .../Input/Jpg/issues/Issue1942InvalidIptcTag.jpg | 3 +++ 3 files changed, 16 insertions(+) create mode 100644 tests/Images/Input/Jpg/issues/Issue1942InvalidIptcTag.jpg diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs index 0ef5090cc8..53c81631de 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs @@ -288,5 +288,17 @@ public void Decoder_Reads_Correct_Resolution_From_Exif(bool useIdentify) => Test Assert.Equal(72, imageInfo.Metadata.HorizontalResolution); Assert.Equal(72, imageInfo.Metadata.VerticalResolution); }); + + [Theory] + [WithFile(TestImages.Jpeg.Issues.InvalidIptcTag, PixelTypes.Rgba32)] + public void Decode_WithInvalidIptcTag_DoesNotThrowException(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + Exception ex = Record.Exception(() => + { + using Image image = provider.GetImage(JpegDecoder); + }); + Assert.Null(ex); + } } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 0860bb5aed..bce22799da 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -261,6 +261,7 @@ public static class Issues public const string WrongColorSpace = "Jpg/issues/Issue1732-WrongColorSpace.jpg"; public const string MalformedUnsupportedComponentCount = "Jpg/issues/issue-1900-malformed-unsupported-255-components.jpg"; public const string MultipleApp01932 = "Jpg/issues/issue-1932-app0-resolution.jpg"; + public const string InvalidIptcTag = "Jpg/issues/Issue1942InvalidIptcTag.jpg"; public static class Fuzz { diff --git a/tests/Images/Input/Jpg/issues/Issue1942InvalidIptcTag.jpg b/tests/Images/Input/Jpg/issues/Issue1942InvalidIptcTag.jpg new file mode 100644 index 0000000000..8b1926128c --- /dev/null +++ b/tests/Images/Input/Jpg/issues/Issue1942InvalidIptcTag.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:9c9db428c4d9d7d1aea6778f263d8deaeeabdcfa63c77ef6ce36ab0e47b364dd +size 93374 From 559feb26dfef6ba2f4b7d05d7108d5e98f0e48c2 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Sun, 16 Jan 2022 22:29:22 +0100 Subject: [PATCH 3/7] Break reading iptc data when Extended data set tag is encountered --- src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs index d07cb11c92..945d5d1034 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs @@ -282,6 +282,11 @@ private void Initialize() uint byteCount = BinaryPrimitives.ReadUInt16BigEndian(this.Data.AsSpan(offset, 2)); offset += 2; + if (byteCount > MaxStandardDataTagSize) + { + // Extended data set tag's are not supported. + break; + } if (isValidTagMarker && byteCount > 0 && (offset + byteCount <= this.Data.Length)) { From 805f9d014fd32a32be0d63d2e2e5866f344e0064 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Mon, 17 Jan 2022 20:19:14 +0100 Subject: [PATCH 4/7] Skip IPTC entry's which have no valid record number --- src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs index 945d5d1034..e88f54419c 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs @@ -277,8 +277,10 @@ private void Initialize() while (offset + 4 < this.Data.Length) { bool isValidTagMarker = this.Data[offset++] == IptcTagMarkerByte; - byte recordType = this.Data[offset++]; + byte recordNumber = this.Data[offset++]; + bool isValidRecordNumber = recordNumber is >= 1 and <= 9; var tag = (IptcTag)this.Data[offset++]; + bool isValidEntry = isValidTagMarker && isValidRecordNumber; uint byteCount = BinaryPrimitives.ReadUInt16BigEndian(this.Data.AsSpan(offset, 2)); offset += 2; @@ -288,7 +290,7 @@ private void Initialize() break; } - if (isValidTagMarker && byteCount > 0 && (offset + byteCount <= this.Data.Length)) + if (isValidEntry && byteCount > 0 && (offset + byteCount <= this.Data.Length)) { var iptcData = new byte[byteCount]; Buffer.BlockCopy(this.Data, offset, iptcData, 0, (int)byteCount); From 300ad3c36842c5867d211e3814db33857c714f0c Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Mon, 17 Jan 2022 20:31:14 +0100 Subject: [PATCH 5/7] Add Standard DataSet Tag description --- .../Metadata/Profiles/IPTC/IptcProfile.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs index e88f54419c..b456c66bbc 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs @@ -246,6 +246,23 @@ public void UpdateData() int i = 0; foreach (IptcValue value in this.Values) { + // Standard DataSet Tag + // +-----------+----------------+---------------------------------------------------------------------------------+ + // | Octet Pos | Name | Description | + // +==========-+================+=================================================================================+ + // | 1 | Tag Marker | Is the tag marker that initiates the start of a DataSet 0x1c. | + // +-----------+----------------+---------------------------------------------------------------------------------+ + // | 2 | Record Number | Octet 2 is the binary representation of the record number. Note that the | + // | | | envelope record number is always 1, and that the application records are | + // | | | numbered 2 through 6, the pre-object descriptor record is 7, the object record | + // | | | is 8, and the post - object descriptor record is 9. | + // +-----------+----------------+---------------------------------------------------------------------------------+ + // | 3 | DataSet Number | Octet 3 is the binary representation of the DataSet number. | + // +-----------+----------------+---------------------------------------------------------------------------------+ + // | 4 and 5 | Data Field | Octets 4 and 5, taken together, are the binary count of the number of octets in | + // | | Octet Count | the following data field(32767 or fewer octets). Note that the value of bit 7 of| + // | | | octet 4(most significant bit) always will be 0. | + // +-----------+----------------+---------------------------------------------------------------------------------+ this.Data[i++] = IptcTagMarkerByte; this.Data[i++] = 2; this.Data[i++] = (byte)value.Tag; From 02cc77360d0c7ad08e254f96c9204596358ebc6b Mon Sep 17 00:00:00 2001 From: Brian Popow <38701097+brianpopow@users.noreply.github.com> Date: Mon, 17 Jan 2022 20:37:13 +0100 Subject: [PATCH 6/7] Change while condition to: offset < this.Data.Length - 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs index b456c66bbc..4eb0409db2 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs @@ -291,7 +291,7 @@ private void Initialize() } int offset = 0; - while (offset + 4 < this.Data.Length) + while (offset < this.Data.Length - 4) { bool isValidTagMarker = this.Data[offset++] == IptcTagMarkerByte; byte recordNumber = this.Data[offset++]; From 6f495f1642e580ac6e4e0b401dddf1533744e30d Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Mon, 17 Jan 2022 20:40:30 +0100 Subject: [PATCH 7/7] Review suggestion from gfoidl: Change condition to (offset <= this.Data.Length - byteCount) --- src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs index 4eb0409db2..610e52d1ca 100644 --- a/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs +++ b/src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs @@ -307,7 +307,7 @@ private void Initialize() break; } - if (isValidEntry && byteCount > 0 && (offset + byteCount <= this.Data.Length)) + if (isValidEntry && byteCount > 0 && (offset <= this.Data.Length - byteCount)) { var iptcData = new byte[byteCount]; Buffer.BlockCopy(this.Data, offset, iptcData, 0, (int)byteCount);