Merge pull request #1927 from EvotecIT/fix-unbounded-xml-parsing-in-visio-comments

PrzemyslawKlys · web-flow · commit e562ded58022 · 2026-06-16T18:28:39.000+02:00
Harden Visio comments loading to prevent XML DoS
diff --git a/OfficeIMO.Tests/Visio.Comments.cs b/OfficeIMO.Tests/Visio.Comments.cs
@@ -1,7 +1,9 @@
 using System;
+using System.Collections.Generic;
 using System.IO;
 using System.IO.Compression;
 using System.Linq;
+using System.Text;
 using System.Xml.Linq;
 using OfficeIMO.Visio;
 using OfficeIMO.Visio.Fluent;
@@ -170,6 +172,74 @@ public void CommentsCanBeReviewedUpdatedReopenedAndRemovedInLoadedDocuments() {
             AssertCommentXml(fluentPath, "Follow-up resolved", done: true, expectedEdited: finalResolvedAt);
         }
 
+        [Fact]
+        public void LoadRejectsCommentsPartWithTooMuchXml() {
+            string filePath = CreateDocumentWithComment();
+            string oversizedCommentText = new('x', checked((int)VisioDocument.MaxCommentsXmlCharacters));
+            ReplaceCommentsXml(filePath, CreateCommentsXml(oversizedCommentText));
+
+            Assert.ThrowsAny<System.Xml.XmlException>(() => VisioDocument.Load(filePath));
+        }
+
+        [Fact]
+        public void LoadRejectsTooManyNativeComments() {
+            string filePath = CreateDocumentWithComment();
+            ReplaceCommentsXml(filePath, CreateCommentsXml(Enumerable.Range(0, VisioDocument.MaxLoadedComments + 1)
+                .Select(index => "Comment " + index.ToString())));
+
+            InvalidDataException exception = Assert.Throws<InvalidDataException>(() => VisioDocument.Load(filePath));
+            Assert.Contains(VisioDocument.MaxLoadedComments.ToString(), exception.Message);
+        }
+
+        [Fact]
+        public void LoadRejectsOversizedNativeCommentText() {
+            string filePath = CreateDocumentWithComment();
+            string oversizedCommentText = new('x', VisioDocument.MaxCommentTextCharacters + 1);
+            ReplaceCommentsXml(filePath, CreateCommentsXml(oversizedCommentText));
+
+            InvalidDataException exception = Assert.Throws<InvalidDataException>(() => VisioDocument.Load(filePath));
+            Assert.Contains(VisioDocument.MaxCommentTextCharacters.ToString(), exception.Message);
+        }
+
+        [Fact]
+        public void SaveRejectsTooManyNativeComments() {
+            string filePath = Path.Combine(Path.GetTempPath(), Guid.NewGuid() + ".vsdx");
+            VisioDocument document = VisioDocument.Create(filePath);
+            VisioPage page = document.AddPage("Review", 11, 8.5);
+            for (int index = 0; index <= VisioDocument.MaxLoadedComments; index++) {
+                page.Comments.Add(new VisioComment("Comment " + index.ToString()) {
+                    AuthorName = "Operations",
+                    AuthorInitials = "OP"
+                });
+            }
+
+            InvalidDataException exception = Assert.Throws<InvalidDataException>(() => document.Save());
+            Assert.Contains(VisioDocument.MaxLoadedComments.ToString(), exception.Message);
+        }
+
+        [Fact]
+        public void SaveRejectsOversizedNativeCommentText() {
+            string filePath = Path.Combine(Path.GetTempPath(), Guid.NewGuid() + ".vsdx");
+            VisioDocument document = VisioDocument.Create(filePath);
+            VisioPage page = document.AddPage("Review", 11, 8.5);
+            page.AddComment(new string('x', VisioDocument.MaxCommentTextCharacters + 1), "Operations", "OP");
+
+            InvalidDataException exception = Assert.Throws<InvalidDataException>(() => document.Save());
+            Assert.Contains(VisioDocument.MaxCommentTextCharacters.ToString(), exception.Message);
+        }
+
+        [Fact]
+        public void SaveRejectsCommentsPartThatExceedsUtf8ByteLimit() {
+            string filePath = Path.Combine(Path.GetTempPath(), Guid.NewGuid() + ".vsdx");
+            VisioDocument document = VisioDocument.Create(filePath);
+            VisioPage page = document.AddPage("Review", 11, 8.5);
+            VisioComment comment = page.AddComment("Byte budget", "Operations", "OP");
+            comment.AuthorName = new string('\u20ac', checked((int)(VisioDocument.MaxCommentsPartBytes / 3L + 1024L)));
+
+            InvalidDataException exception = Assert.Throws<InvalidDataException>(() => document.Save());
+            Assert.Contains(VisioDocument.MaxCommentsPartBytes.ToString(), exception.Message);
+        }
+
         private static void AssertNativeCommentPackage(
             string filePath,
             string expectedText,
@@ -244,6 +314,49 @@ private static void AssertCommentTextAbsent(string filePath, string text) {
             Assert.DoesNotContain(comments.Descendants(v + "CommentEntry"), entry => entry.Value == text);
         }
 
+        private static string CreateDocumentWithComment() {
+            string filePath = Path.Combine(Path.GetTempPath(), Guid.NewGuid() + ".vsdx");
+            VisioDocument document = VisioDocument.Create(filePath);
+            VisioPage page = document.AddPage("Review", 11, 8.5);
+            page.AddComment("Initial comment", "Operations", "OP");
+            document.Save();
+            return filePath;
+        }
+
+        private static void ReplaceCommentsXml(string filePath, string commentsXml) {
+            using ZipArchive archive = ZipFile.Open(filePath, ZipArchiveMode.Update);
+            ZipArchiveEntry entry = archive.GetEntry("visio/comments.xml") ?? throw new InvalidOperationException("Missing comments part.");
+            entry.Delete();
+            ZipArchiveEntry replacement = archive.CreateEntry("visio/comments.xml", CompressionLevel.Optimal);
+            using Stream stream = replacement.Open();
+            using StreamWriter writer = new(stream, new UTF8Encoding(false));
+            writer.Write(commentsXml);
+        }
+
+        private static string CreateCommentsXml(string commentText) {
+            return CreateCommentsXml(new[] { commentText });
+        }
+
+        private static string CreateCommentsXml(IEnumerable<string> commentTexts) {
+            XNamespace v = "http://schemas.microsoft.com/office/visio/2012/main";
+            int id = 1;
+            XDocument comments = new(new XElement(v + "Comments",
+                new XAttribute("xmlns", v.NamespaceName),
+                new XElement(v + "AuthorList",
+                    new XElement(v + "AuthorEntry",
+                        new XAttribute("ID", "1"),
+                        new XAttribute("Name", "Operations"),
+                        new XAttribute("Initials", "OP"))),
+                new XElement(v + "CommentList",
+                    commentTexts.Select(text => new XElement(v + "CommentEntry",
+                        new XAttribute("IX", id++),
+                        new XAttribute("AuthorID", "1"),
+                        new XAttribute("PageID", "0"),
+                        text)))));
+
+            return comments.ToString(SaveOptions.DisableFormatting);
+        }
+
         private static string FormatExpectedDate(DateTimeOffset value) {
             return System.Xml.XmlConvert.ToString(value.UtcDateTime, System.Xml.XmlDateTimeSerializationMode.Utc);
         }
diff --git a/OfficeIMO.Visio/VisioDocument.LoadCore.Comments.cs b/OfficeIMO.Visio/VisioDocument.LoadCore.Comments.cs
@@ -1,13 +1,26 @@
 using System;
 using System.Collections.Generic;
 using System.Globalization;
+using System.IO;
 using System.IO.Packaging;
 using System.Linq;
 using System.Xml;
 using System.Xml.Linq;
 
 namespace OfficeIMO.Visio {
     public partial class VisioDocument {
+        internal const long MaxCommentsPartBytes = 12_000_000;
+        internal const long MaxCommentsXmlCharacters = 10_000_000;
+        internal const int MaxLoadedComments = 10_000;
+        internal const int MaxCommentTextCharacters = 32_768;
+
+        private static readonly XmlReaderSettings CommentsXmlReaderSettings = new() {
+            DtdProcessing = DtdProcessing.Prohibit,
+            XmlResolver = null,
+            MaxCharactersInDocument = MaxCommentsXmlCharacters,
+            MaxCharactersFromEntities = 0,
+        };
+
         private static void LoadComments(Package package, PackagePart documentPart, VisioDocument document) {
             PackageRelationship? commentsRel = documentPart.GetRelationshipsByType(CommentsRelationshipType).FirstOrDefault();
             if (commentsRel == null) {
@@ -20,7 +33,7 @@ private static void LoadComments(Package package, PackagePart documentPart, Visi
             }
 
             PackagePart commentsPart = package.GetPart(commentsUri);
-            XDocument commentsXml = XDocument.Load(commentsPart.GetStream());
+            XDocument commentsXml = LoadCommentsXml(commentsPart);
             XElement? root = commentsXml.Root;
             if (root == null) {
                 return;
@@ -40,7 +53,14 @@ private static void LoadComments(Package package, PackagePart documentPart, Visi
             }
 
             XElement? commentList = root.Elements().FirstOrDefault(element => IsVisioElement(element, "CommentList"));
+            int loadedCommentCount = 0;
             foreach (XElement commentElement in commentList?.Elements().Where(element => IsVisioElement(element, "CommentEntry")) ?? Enumerable.Empty<XElement>()) {
+                loadedCommentCount++;
+                if (loadedCommentCount > MaxLoadedComments) {
+                    throw new InvalidDataException($"Visio comments part contains more than {MaxLoadedComments} comments.");
+                }
+
+                string commentText = GetBoundedCommentText(commentElement);
                 if (!TryParseIntAttribute(commentElement, "PageID", out int pageId)) {
                     continue;
                 }
@@ -53,7 +73,7 @@ private static void LoadComments(Package package, PackagePart documentPart, Visi
                 TryParseIntAttribute(commentElement, "IX", out int commentId);
                 TryParseIntAttribute(commentElement, "AuthorID", out int authorId);
                 authors.TryGetValue(authorId, out var author);
-                VisioComment comment = new(commentElement.Value) {
+                VisioComment comment = new(commentText) {
                     Id = commentId > 0 ? commentId : GetNextLoadedCommentId(page),
                     AuthorName = author.Name,
                     AuthorInitials = author.Initials,
@@ -73,6 +93,25 @@ private static void LoadComments(Package package, PackagePart documentPart, Visi
             }
         }
 
+        private static XDocument LoadCommentsXml(PackagePart commentsPart) {
+            using Stream commentsStream = commentsPart.GetStream();
+            using Stream boundedStream = new BoundedReadStream(commentsStream, MaxCommentsPartBytes);
+            using XmlReader reader = XmlReader.Create(boundedStream, CommentsXmlReaderSettings);
+            return XDocument.Load(reader);
+        }
+
+        private static string GetBoundedCommentText(XElement commentElement) {
+            int textLength = 0;
+            foreach (XText text in commentElement.DescendantNodes().OfType<XText>()) {
+                textLength += text.Value.Length;
+                if (textLength > MaxCommentTextCharacters) {
+                    throw new InvalidDataException($"Visio comment text exceeds {MaxCommentTextCharacters} characters.");
+                }
+            }
+
+            return commentElement.Value;
+        }
+
         private static bool IsVisioElement(XElement element, string localName) {
             return string.Equals(element.Name.LocalName, localName, StringComparison.OrdinalIgnoreCase);
         }
@@ -134,5 +173,55 @@ private static bool ParseCommentBool(string? value) {
 
             return persistedId;
         }
+
+        private sealed class BoundedReadStream : Stream {
+            private readonly Stream _inner;
+            private readonly long _maxBytes;
+            private long _bytesRead;
+
+            internal BoundedReadStream(Stream inner, long maxBytes) {
+                _inner = inner ?? throw new ArgumentNullException(nameof(inner));
+                _maxBytes = maxBytes;
+            }
+
+            public override bool CanRead => _inner.CanRead;
+
+            public override bool CanSeek => false;
+
+            public override bool CanWrite => false;
+
+            public override long Length => throw new NotSupportedException();
+
+            public override long Position {
+                get => _bytesRead;
+                set => throw new NotSupportedException();
+            }
+
+            public override void Flush() {
+                _inner.Flush();
+            }
+
+            public override int Read(byte[] buffer, int offset, int count) {
+                int read = _inner.Read(buffer, offset, count);
+                _bytesRead += read;
+                if (_bytesRead > _maxBytes) {
+                    throw new InvalidDataException($"Visio comments part exceeds {_maxBytes} bytes.");
+                }
+
+                return read;
+            }
+
+            public override long Seek(long offset, SeekOrigin origin) {
+                throw new NotSupportedException();
+            }
+
+            public override void SetLength(long value) {
+                throw new NotSupportedException();
+            }
+
+            public override void Write(byte[] buffer, int offset, int count) {
+                throw new NotSupportedException();
+            }
+        }
     }
 }
diff --git a/OfficeIMO.Visio/VisioDocument.SaveCore.Comments.cs b/OfficeIMO.Visio/VisioDocument.SaveCore.Comments.cs
@@ -49,13 +49,17 @@ private static void WriteCommentsPart(
 
             foreach (VisioPage page in pages) {
                 foreach (VisioComment comment in page.Comments) {
+                    ValidateCommentForSave(comment);
                     CommentAuthorKey authorKey = new(comment.AuthorName, comment.AuthorInitials, comment.AuthorResolutionId);
                     if (!authorIds.TryGetValue(authorKey, out int authorId)) {
                         authorId = authorIds.Count + 1;
                         authorIds.Add(authorKey, authorId);
                     }
 
                     comments.Add((page, comment, authorId));
+                    if (comments.Count > MaxLoadedComments) {
+                        throw new InvalidDataException($"Visio comments part contains more than {MaxLoadedComments} comments.");
+                    }
                 }
             }
 
@@ -104,9 +108,30 @@ private static void WriteCommentsPart(
                 authorList,
                 commentList));
 
+            string serializedComments = commentsXml.Declaration + Environment.NewLine + commentsXml.ToString(SaveOptions.DisableFormatting);
+            ValidateCommentsXmlForSave(serializedComments);
+
             using Stream stream = commentsPart.GetStream(FileMode.Create, FileAccess.Write);
             using StreamWriter writer = new(stream, new UTF8Encoding(false));
-            writer.Write(commentsXml.Declaration + Environment.NewLine + commentsXml.ToString(SaveOptions.DisableFormatting));
+            writer.Write(serializedComments);
+        }
+
+        private static void ValidateCommentForSave(VisioComment comment) {
+            string text = comment.Text ?? string.Empty;
+            if (text.Length > MaxCommentTextCharacters) {
+                throw new InvalidDataException($"Visio comment text exceeds {MaxCommentTextCharacters} characters.");
+            }
+        }
+
+        private static void ValidateCommentsXmlForSave(string commentsXml) {
+            if (commentsXml.Length > MaxCommentsXmlCharacters) {
+                throw new InvalidDataException($"Visio comments part exceeds {MaxCommentsXmlCharacters} XML characters.");
+            }
+
+            int byteCount = Encoding.UTF8.GetByteCount(commentsXml);
+            if (byteCount > MaxCommentsPartBytes) {
+                throw new InvalidDataException($"Visio comments part exceeds {MaxCommentsPartBytes} bytes.");
+            }
         }
 
         private static int AssignSaveFallbackCommentId(VisioComment comment, XElement commentList) {
