Bound the remaining reads sized by untrusted counts

Reviewing the load command bounds turned up three more of the same kind elsewhere.

A symbol, indirect symbol or relocation count was multiplied by an entry size in 32-bit arithmetic. A count large enough to wrap that product gave a small, plausible length that passed the coverage check, and the loop then read past what had been read. Those products are now computed in 64 bits, so the wrap cannot happen and an impossible length is reported instead. The symbol table count reached the caller as an ArgumentOutOfRangeException rather than a diagnostic.

A universal binary header's slice count was used to size an allocation before anything checked the file was big enough to hold that many. It is now checked against the bytes actually there.

Signing changed a good deal before it could fail: it removed the previous signature's content, moved the command, resized __LINKEDIT and cleared the stale flag, then wrote the image to take the digests, and that write can fail. What it changes is now put back if it does, so a failed signing leaves the image exactly as it was rather than half signed.

- src/LibObjectFile/MachO/MachOFile.Symbols.cs: lengths computed in 64 bits
- src/LibObjectFile/MachO/MachOFatFile.cs: the slice count checked before it sizes anything
- src/LibObjectFile/MachO/MachOFile.Sign.cs: signing restores what it changed if it fails

Author: ProjectSynchro

src/LibObjectFile.Tests/MachO/MachOSigningTests.cs

@@ -209,6 +209,23 @@ public void RejectsWhatCannotBeSigned()
         // An object file has no __LINKEDIT, so there is nowhere for a signature to live.
         Assert.ThrowsExactly<InvalidOperationException>(() => LoadMachO("helloworld_x86_64.o").AdHocSign("helloworld"));
         Assert.ThrowsExactly<ArgumentException>(() => LoadMachO("unixthread_i386").AdHocSign(string.Empty));
+
+        // Signing changes a good deal before it can fail, and a half-signed image is worse than
+        // an unsigned one, so a failure has to leave the image exactly as it was.
+        var file = LoadMachO("unixthread_i386");
+        var before = WriteToArray(file);
+        var contents = file.Content.Count;
+        var commands = file.LoadCommands.Count;
+
+        var section = file.FindSegment("__TEXT")!.Sections[0];
+        section.Address += 4;
+        Assert.ThrowsExactly<ObjectFileException>(() => file.AdHocSign("swkotor"));
+        section.Address -= 4;
+
+        Assert.AreEqual(contents, file.Content.Count, "a failed signing left content behind");
+        Assert.AreEqual(commands, file.LoadCommands.Count, "a failed signing left its load command behind");
+        Assert.IsFalse(file.IsCodeSignatureStale, "a failed signing left the image marked stale");
+        ByteArrayAssert.AreEqual(before, WriteToArray(file), "a failed signing changed the image");
     }
 
     private static Span<byte> FindCodeDirectory(byte[] image, uint signatureOffset)

src/LibObjectFile.Tests/MachO/MachOSimpleTests.cs

@@ -496,6 +496,19 @@ static byte[] Patch(Action<byte[]> corrupt)
         var tooManySections = Patch(b => Write(b, 28 + 56 + 48, 40));
         Assert.IsFalse(MachOFile.TryRead(new MemoryStream(tooManySections), out _, out var d3));
         Assert.IsTrue(d3.Messages.Any(m => m.Id == DiagnosticId.MACHO_ERR_InvalidLoadCommandSize), string.Join("; ", d3.Messages));
+
+        // A count large enough that multiplying it by an entry size wraps a 32-bit product would
+        // otherwise pass a length check as a small number and then be read past.
+        var overflowing = LoadMachO("helloworld_x86_64");
+        overflowing.LoadCommands.OfType<MachOSymbolTableCommand>().Single().SymbolCount = 0x10000001;
+        Assert.ThrowsExactly<ObjectFileException>(() => overflowing.ReadSymbolTable());
+
+        // A universal binary header claiming more slices than the file holds must not be trusted
+        // to size anything before that is checked.
+        var fat = File.ReadAllBytes(GetFile("helloworld_fat"));
+        BitConverter.GetBytes(0x10000000u).Reverse().ToArray().CopyTo(fat, 4);
+        Assert.IsFalse(MachOFatFile.TryRead(new MemoryStream(fat), out _, out var d4));
+        Assert.IsTrue(d4.Messages.Any(m => m.Id == DiagnosticId.MACHO_ERR_InvalidFatHeader), string.Join("; ", d4.Messages));
     }
 
     [TestMethod]

src/LibObjectFile.Tests/MachO/TempClass.cs

@@ -0,0 +1,21 @@
+using System; using System.IO; using System.Linq;
+using LibObjectFile.MachO;
+namespace LibObjectFile.Tests.MachO;
+[TestClass] public class TempClass {
+ static string F(string n)=>Path.Combine(AppContext.BaseDirectory,"MachO",n);
+ static byte[] W(MachOFile f){var m=new MemoryStream();f.Write(m);return m.ToArray();}
+ [TestMethod] public void SignFailureIsAtomic() {
+  var file=MachOFile.Read(new MemoryStream(File.ReadAllBytes(F("unixthread_i386"))));
+  var before=W(file);
+  var contentBefore=file.Content.Count;
+  var cmdsBefore=file.LoadCommands.Count;
+  // Break the image so signing's internal write fails after it has mutated.
+  file.FindSegment("__TEXT")!.Sections[0].Address += 4;
+  try{ file.AdHocSign("probe"); Console.WriteLine("signing did not fail (unexpected)"); }
+  catch(Exception e){ Console.WriteLine($"signing failed as expected: {e.GetType().Name}"); }
+  // Put the image back the way it was and check nothing else changed.
+  file.FindSegment("__TEXT")!.Sections[0].Address -= 4;
+  Console.WriteLine($"content {contentBefore}->{file.Content.Count}, commands {cmdsBefore}->{file.LoadCommands.Count}, stale={file.IsCodeSignatureStale}");
+  var after=W(file);
+  Console.WriteLine($"image unchanged after failed signing: {before.SequenceEqual(after)}");
+ }}

src/LibObjectFile/MachO/MachOFatFile.cs

@@ -133,7 +133,14 @@ public static bool TryRead(Stream stream, [NotNullWhen(true)] out MachOFatFile?
         var count = BinaryPrimitives.ReadUInt32BigEndian(header.Slice(4));
 
         var entrySize = GetSliceEntrySize(file.Is64BitOffsets);
-        var entries = new byte[count * (uint)entrySize];
+        if ((ulong)count * (ulong)entrySize > (ulong)(stream.Length - stream.Position))
+        {
+            bag.Error(DiagnosticId.MACHO_ERR_InvalidFatHeader, $"The universal binary header claims {count} architectures, which do not fit in the file");
+            file = null;
+            return false;
+        }
+
+        var entries = new byte[count * entrySize];
         try
         {
             stream.ReadExactly(entries);

src/LibObjectFile/MachO/MachOFile.Relocations.cs

@@ -30,7 +30,7 @@ public IReadOnlyList<MachORelocation> ReadRelocations(MachOSection section)
 
         if (section.NumberOfRelocations == 0) return [];
 
-        var bytes = ReadFileBytes(section.RelocationOffset, section.NumberOfRelocations * MachORelocation.EntrySize, $"relocations of {section.SegmentName},{section.Name}");
+        var bytes = ReadFileBytes(section.RelocationOffset, (ulong)section.NumberOfRelocations * MachORelocation.EntrySize, $"relocations of {section.SegmentName},{section.Name}");
         var relocations = new MachORelocation[section.NumberOfRelocations];
         for (var i = 0; i < relocations.Length; i++)
         {

src/LibObjectFile/MachO/MachOFile.Sign.cs

@@ -4,6 +4,7 @@
 
 using System;
 using System.IO;
+using System.Collections.Generic;
 using System.Linq;
 using LibObjectFile.MachO.CodeSign;
 using LibObjectFile.Utils;
@@ -41,72 +42,121 @@ public void AdHocSign(string identifier)
         var text = FindSegment("__TEXT")
             ?? throw new InvalidOperationException("The image has no __TEXT segment, so the signature cannot describe its executable range.");
 
+        // Everything below can fail, and a half-signed image is worse than an unsigned one, so
+        // what changes is remembered first and put back if it does. The state is captured before
+        // the command is added, since adding one is itself a change to undo.
+        var previousFileSize = linkEdit.FileSize;
+        var previousVmSize = linkEdit.VmSize;
+        var previousStale = IsCodeSignatureStale;
+        var added = new List<MachOContent>();
+        MachOContent? removed = null;
+        var removedIndex = -1;
+        MachOLinkEditDataCommand? addedCommand = null;
+
         var command = CodeSignature;
-        if (command is null)
+        var previousOffset = command?.DataOffset ?? 0;
+        var previousSize = command?.DataSize ?? 0;
+
+        try
         {
-            command = new MachOLinkEditDataCommand
+            if (command is null)
+            {
+                command = new MachOLinkEditDataCommand
+                {
+                    Type = MachOLoadCommandType.CodeSignature,
+                    Size = MachOLinkEditDataCommand.CommandSize,
+                };
+                AppendCommand(command);
+                addedCommand = command;
+            }
+            // A signature covers everything before it, so it has to be the last thing in the file.
+            // Whatever occupied that place before, including a previous signature, is replaced.
+            if (command.DataOffset != 0)
             {
-                Type = MachOLoadCommandType.CodeSignature,
-                Size = MachOLinkEditDataCommand.CommandSize,
+                for (var i = Content.Count - 1; i >= 0; i--)
+                {
+                    if (Content[i].Position == command.DataOffset)
+                    {
+                        removed = Content[i];
+                        removedIndex = i;
+                        Content.RemoveAt(i);
+                        break;
+                    }
+                }
+            }
+
+            var contentEnd = ComputeFileSize();
+            var signatureOffset = AlignHelper.AlignUp((uint)contentEnd, (uint)MachOCodeSignatureConstants.SignatureAlignment);
+
+            var builder = new MachOAdHocSignatureBuilder(identifier)
+            {
+                CodeLimit = signatureOffset,
+                ExecSegmentBase = text.FileOffset,
+                ExecSegmentLimit = text.FileSize,
+                ExecSegmentFlags = FileType == MachOFileType.Execute ? MachOCodeSignatureConstants.ExecSegMainBinary : 0,
             };
-            AppendCommand(command);
-        }
+            var signatureSize = builder.ComputeSize();
 
-        // A signature covers everything before it, so it has to be the last thing in the file.
-        // Whatever occupied that place before, including a previous signature, is replaced.
-        if (command.DataOffset != 0)
-        {
-            for (var i = Content.Count - 1; i >= 0; i--)
+            command.DataOffset = signatureOffset;
+            command.DataSize = signatureSize;
+
+            linkEdit.FileSize = signatureOffset + signatureSize - linkEdit.FileOffset;
+            linkEdit.VmSize = Math.Max(linkEdit.VmSize, linkEdit.FileSize);
+
+            // Aligning the signature can leave a gap, and every byte of the file has to belong to
+            // some content, so the padding is added rather than left as a hole in the list.
+            if (signatureOffset > contentEnd)
             {
-                if (Content[i].Position == command.DataOffset)
+                var gap = new MachOStreamContent(new MemoryStream(new byte[signatureOffset - contentEnd]))
                 {
-                    Content.RemoveAt(i);
-                    break;
-                }
+                    Position = contentEnd,
+                };
+                Content.Add(gap);
+                added.Add(gap);
             }
-        }
 
-        var contentEnd = ComputeFileSize();
-        var signatureOffset = AlignHelper.AlignUp((uint)contentEnd, (uint)MachOCodeSignatureConstants.SignatureAlignment);
+            var placeholder = new MachOStreamContent(new MemoryStream(new byte[signatureSize])) { Position = signatureOffset };
+            Content.Add(placeholder);
+            added.Add(placeholder);
 
-        var builder = new MachOAdHocSignatureBuilder(identifier)
-        {
-            CodeLimit = signatureOffset,
-            ExecSegmentBase = text.FileOffset,
-            ExecSegmentLimit = text.FileSize,
-            ExecSegmentFlags = FileType == MachOFileType.Execute ? MachOCodeSignatureConstants.ExecSegMainBinary : 0,
-        };
-        var signatureSize = builder.ComputeSize();
-
-        command.DataOffset = signatureOffset;
-        command.DataSize = signatureSize;
-
-        linkEdit.FileSize = signatureOffset + signatureSize - linkEdit.FileOffset;
-        linkEdit.VmSize = Math.Max(linkEdit.VmSize, linkEdit.FileSize);
-
-        // Aligning the signature can leave a gap, and every byte of the file has to belong to
-        // some content, so the padding is added rather than left as a hole in the list.
-        if (signatureOffset > contentEnd)
+            // Signing is what makes the image match its signature again, so the edit is settled here
+            // rather than after the digests are taken. Writing checks this, and the digests are taken
+            // by writing the image out.
+            IsCodeSignatureStale = false;
+
+            // The digests cover the image as it will finally be written, so the header, the command
+            // table and the signature's own offsets all have to be settled before they are taken.
+            var image = new MemoryStream();
+            Write(image);
+            placeholder.Content = new MemoryStream(builder.Build(image.GetBuffer().AsSpan(0, (int)signatureOffset)));
+        }
+        catch
         {
-            Content.Add(new MachOStreamContent(new MemoryStream(new byte[signatureOffset - contentEnd]))
+            foreach (var content in added)
             {
-                Position = contentEnd,
-            });
-        }
+                Content.Remove(content);
+            }
 
-        var placeholder = new MachOStreamContent(new MemoryStream(new byte[signatureSize])) { Position = signatureOffset };
-        Content.Add(placeholder);
+            if (removed is not null)
+            {
+                Content.Insert(removedIndex, removed);
+            }
 
-        // Signing is what makes the image match its signature again, so the edit is settled here
-        // rather than after the digests are taken. Writing checks this, and the digests are taken
-        // by writing the image out.
-        IsCodeSignatureStale = false;
+            if (addedCommand is not null)
+            {
+                LoadCommands.Remove(addedCommand);
+            }
+            else if (command is not null)
+            {
+                command.DataOffset = previousOffset;
+                command.DataSize = previousSize;
+            }
 
-        // The digests cover the image as it will finally be written, so the header, the command
-        // table and the signature's own offsets all have to be settled before they are taken.
-        var image = new MemoryStream();
-        Write(image);
-        placeholder.Content = new MemoryStream(builder.Build(image.GetBuffer().AsSpan(0, (int)signatureOffset)));
+            linkEdit.FileSize = previousFileSize;
+            linkEdit.VmSize = previousVmSize;
+            IsCodeSignatureStale = previousStale;
+            throw;
+        }
     }
 
 }

src/LibObjectFile/MachO/MachOFile.Symbols.cs

@@ -33,7 +33,7 @@ public IReadOnlyList<MachOSymbol> ReadSymbolTable()
         if (command is null || command.SymbolCount == 0) return [];
 
         var entrySize = MachOSymbolTableCommand.GetSymbolSize(Is64Bit);
-        var entries = ReadFileBytes(command.SymbolOffset, command.SymbolCount * entrySize, "symbol table");
+        var entries = ReadFileBytes(command.SymbolOffset, (ulong)command.SymbolCount * entrySize, "symbol table");
         var strings = ReadFileBytes(command.StringOffset, command.StringSize, "string table");
 
         var symbols = new List<MachOSymbol>((int)command.SymbolCount);
@@ -63,7 +63,7 @@ public IReadOnlyList<uint> ReadIndirectSymbolTable()
         var command = LoadCommands.OfType<MachODynamicSymbolTableCommand>().FirstOrDefault();
         if (command is null || command.IndirectSymbolCount == 0) return [];
 
-        var bytes = ReadFileBytes(command.IndirectSymbolOffset, command.IndirectSymbolCount * MachODynamicSymbolTableCommand.IndirectSymbolEntrySize, "indirect symbol table");
+        var bytes = ReadFileBytes(command.IndirectSymbolOffset, (ulong)command.IndirectSymbolCount * MachODynamicSymbolTableCommand.IndirectSymbolEntrySize, "indirect symbol table");
         var indices = new uint[command.IndirectSymbolCount];
         for (var i = 0; i < indices.Length; i++)
         {
@@ -118,10 +118,18 @@ private static string ReadString(byte[] strings, uint offset)
     /// <summary>
     /// Reads a run of bytes at a file offset out of whichever content covers it.
     /// </summary>
-    private byte[] ReadFileBytes(uint offset, uint length, string what)
+    private byte[] ReadFileBytes(uint offset, ulong length, string what)
     {
         if (length == 0) return [];
 
+        // The length is a count from the file multiplied by an entry size, so it is computed in
+        // 64 bits: a count large enough to wrap a 32-bit product would otherwise pass this check
+        // as a small length and then be read past.
+        if (length > int.MaxValue || offset + length > uint.MaxValue)
+        {
+            Throw(offset, length, what);
+        }
+
         foreach (var content in Content)
         {
             if (content is not MachOStreamContent stream || offset < content.Position) continue;
@@ -135,8 +143,16 @@ private byte[] ReadFileBytes(uint offset, uint length, string what)
             return buffer;
         }
 
+        Throw(offset, length, what);
+        return [];
+    }
+
+    [System.Diagnostics.CodeAnalysis.DoesNotReturn]
+    private static void Throw(uint offset, ulong length, string what)
+    {
+        var message = $"The {what} at 0x{offset:X} for 0x{length:X} bytes is not covered by any content of this image.";
         var diagnostics = new DiagnosticBag();
-        diagnostics.Error(DiagnosticId.MACHO_ERR_DataOutsideImage, $"The {what} at 0x{offset:X} for 0x{length:X} bytes is not covered by any content of this image.");
-        throw new ObjectFileException($"The {what} at 0x{offset:X} for 0x{length:X} bytes is not covered by any content of this image.", diagnostics);
+        diagnostics.Error(DiagnosticId.MACHO_ERR_DataOutsideImage, message);
+        throw new ObjectFileException(message, diagnostics);
     }
 }