diff --git a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs index b232e7b74..302e42a92 100644 --- a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs +++ b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs @@ -67,6 +67,26 @@ public virtual bool TryCopyBlobContentStream( Action writeAction, out BlobHydrationFailureCategory failureCategory) { + // Short-circuit a malformed SHA (for example a corrupt placeholder's all-NUL + // content-id) before the retry loop. GitRepo already rejects it as a clean miss, + // but a bogus SHA can never be downloaded either (the server returns 404), so + // attempting it would only produce doomed download retries. Because the read is + // never satisfied, the caller re-requests it endlessly, which turns one corrupt + // placeholder into an unbounded error/retry storm. Fail fast and cheap instead. + // The cause stays categorized as Unexpected (no dedicated category); the caller + // tags its terminal telemetry from failureCategory below. + if (!SHA1Util.IsValidShaFormat(sha)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(sha)); + metadata.Add("RequestSource", requestSource.ToString()); + metadata.Add(TracingConstants.MessageKey.WarningMessage, "TryCopyBlobContentStream: Refusing to hydrate blob with malformed SHA"); + this.Tracer.RelatedEvent(EventLevel.Warning, nameof(this.TryCopyBlobContentStream) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + + failureCategory = BlobHydrationFailureCategory.Unexpected; + return false; + } + // Track the outcome of the most recent attempt so that the terminal failure // telemetry can attribute the failure to a cause (network vs. object-missing vs. // local copy) that is otherwise collapsed into the bool return value below. The @@ -188,6 +208,27 @@ private DownloadAndSaveObjectResult TryDownloadAndSaveObject( RequestSource requestSource, bool retryOnFailure) { + // Defense in depth for a malformed object id (for example a corrupt placeholder's + // all-NUL content-id). On .NET Framework Path.Combine threw ArgumentException on + // such a value; on modern .NET it does not, so a malformed SHA silently misses the + // local object store and would otherwise be sent to the cache server, which rejects + // the URL with HTTP 400 - and GVFS then erases a valid credential (HttpRequestor + // treats 400 as an auth failure), producing a credential-prompt storm. Callers other + // than blob hydration reach this method WITHOUT going through the + // TryCopyBlobContentStream guard - the git.exe read-object hook (NamedPipeMessage, + // via InProcessMount) and the gitattributes GVFSVerb - so reject a malformed SHA here + // for every caller before any request is built. + if (!SHA1Util.IsValidShaFormat(objectId)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(objectId)); + metadata.Add("RequestSource", requestSource.ToString()); + metadata.Add(TracingConstants.MessageKey.WarningMessage, nameof(this.TryDownloadAndSaveObject) + ": Refusing to download object with malformed SHA"); + this.Tracer.RelatedEvent(EventLevel.Warning, nameof(this.TryDownloadAndSaveObject) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + + return DownloadAndSaveObjectResult.Error; + } + if (objectId == GVFSConstants.AllZeroSha) { return DownloadAndSaveObjectResult.Error; diff --git a/GVFS/GVFS.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index 302b0e13e..055f85f61 100644 --- a/GVFS/GVFS.Common/Git/GitRepo.cs +++ b/GVFS/GVFS.Common/Git/GitRepo.cs @@ -113,6 +113,19 @@ public virtual bool CommitAndRootTreeExists(string commitSha, out string rootTre /// public virtual bool LooseObjectExists(string sha) { + // Guard against a malformed SHA (for example a corrupt placeholder's all-NUL + // content-id) so Path.Combine cannot throw ArgumentException below. Emit the same + // greppable Warning as the other malformed-SHA guards so a silent "does not exist" + // answer is still diagnosable in telemetry. + if (!SHA1Util.IsValidShaFormat(sha)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(sha)); + metadata.Add(TracingConstants.MessageKey.WarningMessage, nameof(this.LooseObjectExists) + ": Malformed SHA cannot exist as a loose object"); + this.tracer.RelatedEvent(EventLevel.Warning, nameof(this.LooseObjectExists) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + return false; + } + if (GVFSPlatform.Instance.Constants.CaseSensitiveFileSystem) { sha = sha.ToLower(); @@ -343,6 +356,33 @@ private LooseBlobState GetLooseBlobStateAtPath(string blobPath, Action writeAction, out long size) { + // A corrupt placeholder can carry a malformed content-id (for example 40 NUL + // bytes instead of a hex SHA). Reject it up front and report an invalid loose + // object, which the callers treat as a clean, non-retryable miss. + // + // The behavior of Path.Combine below is runtime-dependent, so validating here + // (rather than relying on an exception) is required on modern .NET: + // - On .NET Framework, Path.Combine throws ArgumentException ("Illegal + // characters in path") on the NUL bytes. ArgumentException is not handled by + // RetryWrapper, so it bypasses both the retry logic and the download fallback + // and fails the hydration permanently (a retry storm - the original symptom). + // - On modern .NET (.NET Core/5+), Path.Combine no longer validates path + // characters, so it does NOT throw; the bogus path simply misses on disk and + // the request would fall through to a server download that the gateway rejects + // with HTTP 400 (ICM 850075166). The download path is guarded separately in + // GVFSGitObjects.TryDownloadAndSaveObject. + if (!SHA1Util.IsValidShaFormat(blobSha)) + { + size = -1; + + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(blobSha)); + metadata.Add(TracingConstants.MessageKey.WarningMessage, nameof(this.GetLooseBlobState) + ": Refusing to build loose object path from malformed blob SHA"); + this.tracer.RelatedEvent(EventLevel.Warning, nameof(this.GetLooseBlobState) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + + return LooseBlobState.Invalid; + } + // Ensure SHA path is lowercase for case-sensitive filesystems if (GVFSPlatform.Instance.Constants.CaseSensitiveFileSystem) { diff --git a/GVFS/GVFS.Common/SHA1Util.cs b/GVFS/GVFS.Common/SHA1Util.cs index 0fc20019d..01a2ba230 100644 --- a/GVFS/GVFS.Common/SHA1Util.cs +++ b/GVFS/GVFS.Common/SHA1Util.cs @@ -9,7 +9,37 @@ public static class SHA1Util { public static bool IsValidShaFormat(string sha) { - return sha.Length == 40 && sha.All(c => Uri.IsHexDigit(c)); + return sha != null && sha.Length == 40 && sha.All(c => Uri.IsHexDigit(c)); + } + + /// + /// Returns a log-safe rendering of a value that was expected to be a + /// 40-character hex SHA but is not. Non-hex characters (for example the + /// NUL bytes of a corrupt placeholder content-id) are escaped as \uXXXX + /// so the value stays greppable in telemetry and carries no control + /// characters. + /// + public static string ToLoggableShaString(string sha) + { + if (sha == null) + { + return "(null)"; + } + + StringBuilder builder = new StringBuilder(sha.Length); + foreach (char c in sha) + { + if (Uri.IsHexDigit(c)) + { + builder.Append(c); + } + else + { + builder.AppendFormat("\\u{0:x4}", (int)c); + } + } + + return builder.ToString(); } public static string SHA1HashStringForUTF8String(string s) diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs index 7a6bad6f2..c99a60205 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs @@ -733,7 +733,7 @@ public HResult GetFileDataCallback( metadata.Add("streamGuid", streamGuid); metadata.Add("triggeringProcessId", triggeringProcessId); metadata.Add("triggeringProcessImageFileName", triggeringProcessImageFileName); - metadata.Add("sha", sha); + metadata.Add("sha", SHA1Util.ToLoggableShaString(sha)); metadata.Add("placeholderVersion", placeholderVersion); metadata.Add("commandId", commandId); diff --git a/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs b/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs index 90127fb99..3d868bcf7 100644 --- a/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs +++ b/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs @@ -1,6 +1,7 @@ using GVFS.Common; using GVFS.Tests.Should; using NUnit.Framework; +using System.Linq; using System.Text; namespace GVFS.UnitTests.Common @@ -31,6 +32,30 @@ public void IsValidFullSHAIsFalseForEmptyString() SHA1Util.IsValidShaFormat(string.Empty).ShouldEqual(false); } + [TestCase] + public void IsValidShaFormatIsFalseForNull() + { + SHA1Util.IsValidShaFormat(null).ShouldEqual(false); + } + + [TestCase] + public void ToLoggableShaStringEscapesNonHexCharacters() + { + SHA1Util.ToLoggableShaString(null).ShouldEqual("(null)"); + SHA1Util.ToLoggableShaString(new string('\0', 3)).ShouldEqual("\\u0000\\u0000\\u0000"); + SHA1Util.ToLoggableShaString("abc\0").ShouldEqual("abc\\u0000"); + SHA1Util.ToLoggableShaString("abcDEF123").ShouldEqual("abcDEF123"); + + // Control characters and non-ASCII / high code points must be escaped and padded to 4 hex digits. + SHA1Util.ToLoggableShaString("a\tb\n").ShouldEqual("a\\u0009b\\u000a"); + SHA1Util.ToLoggableShaString("\u00e9\u1234").ShouldEqual("\\u00e9\\u1234"); + + // The realistic corrupt-content-id shape: a full 40-char value that is partly valid + // hex and partly NUL, rendered with the hex kept and the NULs escaped. + SHA1Util.ToLoggableShaString(new string('a', 20) + new string('\0', 20)) + .ShouldEqual(new string('a', 20) + string.Concat(Enumerable.Repeat("\\u0000", 20))); + } + [TestCase] public void IsValidFullSHAIsFalseForHexStringsNot40Chars() { diff --git a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs index 205d2b4de..9abe55752 100644 --- a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs @@ -330,6 +330,86 @@ public void FailsNullBytePackDownloads() gitObjects => gitObjects.TryDownloadCommit("object0")); } + [TestCase] + public void TryCopyBlobContentStreamFailsCleanlyForCorruptAllNullByteSha() + { + // A corrupt placeholder can present a content-id of 40 NUL bytes instead of a + // hex SHA. Those characters are illegal in a file path, so building a loose + // object path from them used to throw an unhandled ArgumentException that + // bypassed retry and download fallback and permanently failed (and retry-stormed) + // the hydration. The read must now fail cleanly with no exception. + this.AssertMalformedShaHydrationFailsCleanly(new string('\0', 40)); + } + + [TestCase] + public void TryCopyBlobContentStreamFailsCleanlyForOtherMalformedShas() + { + this.AssertMalformedShaHydrationFailsCleanly(string.Empty); + this.AssertMalformedShaHydrationFailsCleanly("0123456789"); + this.AssertMalformedShaHydrationFailsCleanly(new string('0', 39)); + this.AssertMalformedShaHydrationFailsCleanly("000000000000000000000000000000000000000g"); + + // 40 chars long but with a NUL embedded among hex digits — the realistic + // corrupt-content-id shape that actually reproduces the original "Illegal + // characters in path" ArgumentException (length passes, hex check fails). + this.AssertMalformedShaHydrationFailsCleanly(new string('0', 20) + "\0" + new string('0', 19)); + + // 40 chars long with an embedded backslash. Unlike NUL this would NOT have thrown + // pre-fix (backslash is a legal path separator), but it is still non-hex, so the + // guard must reject it as a clean miss rather than probe a bogus path. + this.AssertMalformedShaHydrationFailsCleanly(new string('a', 20) + "\\" + new string('a', 19)); + } + + [TestCase] + public void TryDownloadAndSaveObjectDoesNotSendMalformedShaToServer() + { + // Regression for the customer HTTP-400 mode (ICM 850075166). On modern .NET a + // corrupt placeholder's all-NUL SHA does not throw in Path.Combine, so it misses + // locally and, without this guard, is sent to the cache server, which rejects the + // URL with HTTP 400 - and GVFS then erases a valid credential, producing a GCM + // prompt storm. This download path is reached by callers OTHER than blob hydration + // (the git.exe read-object hook via NamedPipeMessage, and the gitattributes + // GVFSVerb), which do not go through the TryCopyBlobContentStream guard, so it must + // be rejected at the download method itself for every request source. + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => false; + fileSystem.OnOpenFileStream = (path, mode, access) => new MemoryStream(); + + MockHttpGitObjects httpObjects = new MockHttpGitObjects(); + GVFSGitObjects dut = this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out MockTracer tracer); + + string[] malformedShas = + { + new string('\0', 40), + string.Empty, + new string('0', 39), + new string('0', 20) + "\0" + new string('0', 19), + }; + + GVFSGitObjects.RequestSource[] sources = + { + GVFSGitObjects.RequestSource.FileStreamCallback, + GVFSGitObjects.RequestSource.NamedPipeMessage, + GVFSGitObjects.RequestSource.GVFSVerb, + }; + + foreach (string malformedSha in malformedShas) + { + foreach (GVFSGitObjects.RequestSource source in sources) + { + GitObjects.DownloadAndSaveObjectResult result = GitObjects.DownloadAndSaveObjectResult.Success; + Assert.DoesNotThrow( + () => result = dut.TryDownloadAndSaveObject(malformedSha, source), + "TryDownloadAndSaveObject must not throw for a malformed SHA"); + result.ShouldEqual(GitObjects.DownloadAndSaveObjectResult.Error); + } + } + + // No malformed SHA reached the network, and the rejection is diagnosable. + httpObjects.TryDownloadObjectsCallCount.ShouldEqual(0); + tracer.RelatedEventNames.ShouldContain(e => e == "TryDownloadAndSaveObject_MalformedBlobSha"); + } + [TestCase] public void CoalescesMultipleConcurrentRequestsForSameObject() { @@ -751,6 +831,62 @@ private void AssertRetryableExceptionOnDownload( } } + private void AssertMalformedShaHydrationFailsCleanly(string malformedSha) + { + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => + { + Assert.Fail("A malformed SHA must never be turned into a filesystem path"); + return false; + }; + + MockTracer tracer = new MockTracer(); + GVFSEnlistment enlistment = new GVFSEnlistment(TestEnlistmentRoot, "https://fakeRepoUrl", "fakeGitBinPath", authentication: null); + enlistment.InitializeCachePathsFromKey(TestLocalCacheRoot, TestObjectRoot); + GitRepo repo = new GitRepo(tracer, enlistment, fileSystem, () => new MockLibGit2Repo(tracer)); + GVFSContext context = new GVFSContext(tracer, fileSystem, repo, enlistment); + GVFSGitObjects gitObjects = new UnsafeGVFSGitObjects(context, new MockHttpGitObjects()); + + // GitRepo layer: must not throw ArgumentException ("Illegal characters in path"), + // and must report a clean miss. + bool repoResult = true; + Assert.DoesNotThrow( + () => repoResult = repo.TryCopyBlobContentStream( + malformedSha, + (stream, length) => Assert.Fail("Should not copy any content for a malformed SHA")), + "GitRepo.TryCopyBlobContentStream must not throw for a malformed SHA"); + repoResult.ShouldEqual(false); + + // GVFSGitObjects layer: must fail fast with no throw. The out category stays + // Unexpected (a malformed SHA gets no dedicated telemetry category). + bool copied = true; + GVFSGitObjects.BlobHydrationFailureCategory failureCategory = GVFSGitObjects.BlobHydrationFailureCategory.None; + Assert.DoesNotThrow( + () => copied = gitObjects.TryCopyBlobContentStream( + malformedSha, + new CancellationToken(), + GVFSGitObjects.RequestSource.FileStreamCallback, + (stream, length) => Assert.Fail("Should not copy any content for a malformed SHA"), + out failureCategory), + "GVFSGitObjects.TryCopyBlobContentStream must not throw for a malformed SHA"); + copied.ShouldEqual(false); + failureCategory.ShouldEqual(GVFSGitObjects.BlobHydrationFailureCategory.Unexpected); + + // The short-circuit must happen BEFORE the retry/download loop: a malformed SHA + // must never reach a server download. The retrier logs "Failed to provide blob + // contents" on every attempt, so its absence proves no download/retry ran. + bool anyDownloadAttemptLogged = tracer.RelatedErrorEvents + .Concat(tracer.RelatedWarningEvents) + .Any(e => e.Contains("Failed to provide blob contents")); + anyDownloadAttemptLogged.ShouldEqual(false); + + // The corrupt-placeholder read must stay diagnosable: both guard layers emit their + // distinct greppable *_MalformedBlobSha event. Assert the emission so a regression + // that silently dropped the warning would fail here. + tracer.RelatedEventNames.ShouldContain(e => e == "GetLooseBlobState_MalformedBlobSha"); + tracer.RelatedEventNames.ShouldContain(e => e == "TryCopyBlobContentStream_MalformedBlobSha"); + } + private GVFSGitObjects CreateTestableGVFSGitObjects(GitObjectsHttpRequestor httpObjects, MockFileSystemWithCallbacks fileSystem) { return this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out _); @@ -798,6 +934,10 @@ private MockHttpGitObjects(MockGVFSEnlistment enlistment) public HttpStatusCode? StatusCodeToReturn { get; set; } public byte[] ContentBytesToServe { get; set; } + // Number of times a network download was actually attempted. Lets a test prove a + // malformed SHA is rejected before any request reaches the server. + public int TryDownloadObjectsCallCount { get; private set; } + public static MemoryStream GetRandomStream(int size) { Random randy = new Random(0); @@ -827,6 +967,8 @@ public override RetryWrapper.InvocationResult TryDownloadOb Action.ErrorEventArgs> onFailure, bool preferBatchedLooseObjects) { + this.TryDownloadObjectsCallCount++; + if (this.StatusCodeToReturn.HasValue) { // Simulate the server returning a non-OK status (e.g. 404) so callers can exercise diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index c04be4204..d933584e9 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs @@ -16,6 +16,7 @@ public MockTracer() this.RelatedInfoEvents = new List(); this.RelatedWarningEvents = new List(); this.RelatedErrorEvents = new List(); + this.RelatedEventNames = new List(); } public MockTracer StartActivityTracer { get; private set; } @@ -25,6 +26,10 @@ public MockTracer() public List RelatedWarningEvents { get; } public List RelatedErrorEvents { get; } + // Names of events reported via RelatedEvent (which, unlike RelatedInfo/Warning/Error, + // do not otherwise get recorded). Lets tests assert a specific diagnostic event fired. + public List RelatedEventNames { get; } + public void WaitForRelatedEvent() { this.waitEvent.WaitOne(); @@ -32,6 +37,7 @@ public void WaitForRelatedEvent() public void RelatedEvent(EventLevel error, string eventName, EventMetadata metadata) { + this.RelatedEventNames.Add(eventName); if (eventName == this.WaitRelatedEventName) { this.waitEvent.Set(); @@ -40,6 +46,7 @@ public void RelatedEvent(EventLevel error, string eventName, EventMetadata metad public void RelatedEvent(EventLevel error, string eventName, EventMetadata metadata, Keywords keyword) { + this.RelatedEventNames.Add(eventName); if (eventName == this.WaitRelatedEventName) { this.waitEvent.Set();