diff --git a/documentation/TraceEvent/TraceEventProgrammersGuide.md b/documentation/TraceEvent/TraceEventProgrammersGuide.md index c7b1f7b95..d705ded65 100644 --- a/documentation/TraceEvent/TraceEventProgrammersGuide.md +++ b/documentation/TraceEvent/TraceEventProgrammersGuide.md @@ -844,6 +844,25 @@ Below are the steps in converting logging an event with a stack to a resolved sy 2. For operating system DLLs, the PDBS live on what is called a symbol server. To find these your `_NT_SYMBOL_PATH` must include the name for these symbol servers (the public Microsoft symbol server is `SRV*https://msdl.microsoft.com/download/symbols`). However to look up a DLL in the symbol server, **you need a special GUID associated with the DLL, and a RAW ETL file does NOT INCLUDE this GUID**!. If you try to look up the DLL's PDB on the machine where the DLL exists, `TraceEvent` can fetch the necessary GUID from the DLL itself, but if the ETL file was copied to another machine this will not work and the PDB cannot be fetched. Running the `TraceEventSource.MergeInPlace` operation rewrites the raw ETL file so that it includes the necessary DLL GUIDs and thus is a requirement if you move the data off the collection machine (and you want symbolic information for native code stacks). 3. For .NET code all the library code is precompiled (NGENed) and so is looked up using a PDB like the native case. However unlike native DLLs, the PDBs for the NGEN images are typically not saved on the Microsoft symbol server. Instead you must generate the PDBs for the NGEN images from the IL images as you need them. Again if you resolve the symbols on the machine where the collection happened, at the time you resolve the symbols `TraceEvent`'s `SymbolReader` class will automatically generate the NGEN image for you and cache it, however if you move the ETL file off the machine, you need to generate the NGEN PDBs as well as merge the ETL file to get the symbolic information for the .NET code in NGEN images. This is what the `SymbolReader.GenerateNGenSymbolsForModule` method can help you do. TODO MORE +`SymbolReader` has three independent authorization checks: + +- `SecurityCheck` controls whether a PDB found in an unsafe local location, such as beside an executable or in its build directory, may be used. The callback receives the PDB path. If the property is not set or returns `false`, the PDB is rejected. Applications should prompt the user or allow only explicitly trusted directories. +- `AuthorizeDownload` controls whether Source Link and HTTP source-server downloads may retrieve source code. The callback receives a `DownloadAuthorizationRequest` and runs immediately before the HTTP request; approval also covers redirects followed by the HTTP client. It does not run for symbol/PDB downloads, local source, embedded source, or a valid cached source file. If the property is not set, source downloads are denied by default. Applications should prompt the user or enforce an explicit host/repository allow-list. +- `AuthorizeSourceServerCommand` controls whether a validated command derived from PDB source-server data may execute. The callback receives a `SourceServerAuthorizationRequest` containing the exact rebuilt command. If the property is not set or returns `false`, the command is not run. Applications should prompt the user before execution or use a narrowly scoped policy. Before invoking the callback, TraceEvent independently verifies that the command uses a supported executable and allowed arguments. Returning `true` from the callback does not bypass that validation. + +```C# +using (var symbolReader = new SymbolReader(log)) +{ + symbolReader.SecurityCheck = pdbPath => + trustedPdbDirectories.Contains(Path.GetDirectoryName(pdbPath)); + symbolReader.AuthorizeDownload = request => + trustedSourceHosts.Contains(request.Uri.Host); + symbolReader.AuthorizeSourceServerCommand = request => false; +} +``` + +Each `true` result authorizes only the operation described by that callback invocation. Exceptions from `AuthorizeDownload` are logged and deny the download. + So in summary to get good stacks and have them work on any machine for any code you need to: 1. Collect the necessary ImageLoad and JIT compile events (including CAPTURE\_STATE so you get information about events that preceded data collection start). diff --git a/src/PerfView/App.cs b/src/PerfView/App.cs index 634a198d2..4a0bd5420 100755 --- a/src/PerfView/App.cs +++ b/src/PerfView/App.cs @@ -905,6 +905,17 @@ public static SymbolReader GetSymbolReader(string etlFilePath = null, SymbolRead return result == System.Windows.MessageBoxResult.Yes; }; + ret.AuthorizeDownload = request => + { + var result = XamlMessageBox.Show( + $"PerfView wants to download source code for:\n{request.BuildTimeFilePath}\n\n" + + $"From:\n{request.Uri.AbsoluteUri}\n\nDo you want to allow this download?", + "Source Code Download", + System.Windows.MessageBoxButton.YesNo); + + return result == System.Windows.MessageBoxResult.Yes; + }; + ret.AuthorizeSourceServerCommand = request => { var result = XamlMessageBox.Show( @@ -922,6 +933,12 @@ public static SymbolReader GetSymbolReader(string etlFilePath = null, SymbolRead #endif { ret.SecurityCheck = (pdbFile => true); +#if !PERFVIEW_COLLECT + ret.AuthorizeDownload = request => + { + return true; + }; +#endif ret.AuthorizeSourceServerCommand = request => { #if PERFVIEW_COLLECT diff --git a/src/PerfView/CommandLineArgs.cs b/src/PerfView/CommandLineArgs.cs index 0b785be93..df8e5f76e 100644 --- a/src/PerfView/CommandLineArgs.cs +++ b/src/PerfView/CommandLineArgs.cs @@ -561,7 +561,7 @@ private void SetupCommandLine(CommandLineParser parser) "in some scenarios where NGEN PDB are not working properly."); parser.DefineOptionalQualifier("NoV2Rundown", ref NoV2Rundown, "Don't do rundown for .NET (CLR) V2 processes."); - parser.DefineOptionalQualifier("TrustPdbs", ref TrustPdbs, "Normally PerfView does not trust PDBs outside the _NT_SYMBOL_PATH and pops a dialog box. Suppress this."); + parser.DefineOptionalQualifier("TrustPdbs", ref TrustPdbs, "Trust PDBs outside _NT_SYMBOL_PATH and authorize PDB-directed source downloads without prompting."); parser.DefineOptionalQualifier("AcceptEULA", ref AcceptEULA, "Accepts the EULA associated with PerfView."); parser.DefineOptionalQualifier("DataFile", ref DataFile, "FileName of the profile data to generate."); diff --git a/src/TraceEvent/Symbols/NativeSymbolModule.cs b/src/TraceEvent/Symbols/NativeSymbolModule.cs index 757106f40..000589313 100644 --- a/src/TraceEvent/Symbols/NativeSymbolModule.cs +++ b/src/TraceEvent/Symbols/NativeSymbolModule.cs @@ -567,11 +567,20 @@ protected override string GetSourceFromSrcServer() if (Uri.TryCreate(target, UriKind.Absolute, out Uri uri) && (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps)) { + var httpRequest = new DownloadAuthorizationRequest( + BuildTimeFilePath, + _symbolModule.SymbolFilePath, + uri); if (!TryCreateSafeSourceCacheDirectory(safeCachePath)) { return null; } + if (!_symbolModule.SymbolReader.CheckDownloadAuthorization(httpRequest)) + { + return null; + } + if (_symbolModule.SymbolReader.GetPhysicalFileFromServer( uri.GetComponents(UriComponents.SchemeAndServer, UriFormat.Unescaped), uri.AbsolutePath, diff --git a/src/TraceEvent/Symbols/SymbolReader.cs b/src/TraceEvent/Symbols/SymbolReader.cs index c12950285..16e2481b1 100644 --- a/src/TraceEvent/Symbols/SymbolReader.cs +++ b/src/TraceEvent/Symbols/SymbolReader.cs @@ -922,6 +922,18 @@ public SymbolReaderOptions Options /// public Func SecurityCheck { get; set; } + /// + /// We call back on this before downloading source code over HTTP. The callback receives a + /// describing the download and must return + /// true to allow it or false to deny it. Authorization of the initial URI also + /// covers redirects followed by the HTTP client. + /// + /// If this property is null, source downloads are denied by default. To enable source + /// downloads, set this property to an interactive prompt, an allow-list policy, or + /// request => true for fully trusted scenarios. + /// + public Func AuthorizeDownload { get; set; } + /// /// We call back on this before executing a source-server fetch command (e.g. tf.exe view ... or /// tf.exe git view ...) that was derived from PDB-supplied data. The callback receives a @@ -1234,6 +1246,36 @@ public void Dispose() } #region private + internal bool CheckDownloadAuthorization(DownloadAuthorizationRequest request) + { + var authorize = AuthorizeDownload; + if (authorize == null) + { + m_log.WriteLine("Source download denied by default because no authorization policy is installed: {0}", request.Uri.AbsoluteUri); + return false; + } + + bool authorized; + try + { + authorized = authorize(request); + } + catch (Exception exception) + { + m_log.WriteLine( + "Source download denied because the authorization policy threw an exception for {0}: {1}", + request.Uri.AbsoluteUri, + exception); + return false; + } + + m_log.WriteLine( + "Source download authorization {0}: {1}", + authorized ? "GRANTED" : "DENIED", + request.Uri.AbsoluteUri); + return authorized; + } + /// /// Returns true if 'filePath' exists and is a PDB that has pdbGuid and pdbAge. /// if pdbGuid == Guid.Empty, then the pdbGuid and pdbAge checks are skipped. @@ -2279,6 +2321,42 @@ private List> ParseSourceLinkJson(IEnumerable + /// Describes a source download that is ready to start. Passed to + /// so the caller can choose whether to allow it. + /// + /// + /// This is a class so that additional download context can be added in the future without changing + /// the delegate signature. + /// + public sealed class DownloadAuthorizationRequest + { + internal DownloadAuthorizationRequest( + string buildTimeFilePath, + string symbolFilePath, + Uri uri) + { + BuildTimeFilePath = buildTimeFilePath; + SymbolFilePath = symbolFilePath; + Uri = uri; + } + + /// + /// Gets the source file path recorded at build time. + /// + public string BuildTimeFilePath { get; } + + /// + /// Gets the path of the PDB that supplied the source information. + /// + public string SymbolFilePath { get; } + + /// + /// Gets the remote source URI. + /// + public Uri Uri { get; } + } + /// /// Describes a source-server fetch command that has been validated and is about to be executed. /// Passed to so the caller can choose whether @@ -2564,6 +2642,15 @@ protected virtual string GetSourceFromSrcServer() string url = Url; if (url != null) { + var request = new DownloadAuthorizationRequest( + BuildTimeFilePath, + _symbolModule.SymbolFilePath, + new Uri(url)); + if (!_symbolModule.SymbolReader.CheckDownloadAuthorization(request)) + { + return null; + } + var httpClient = _symbolModule.SymbolReader.HttpClient; HttpResponseMessage response = httpClient.GetAsync(url).Result; diff --git a/src/TraceEvent/TraceEvent.Tests/Symbols/SymbolReaderTests.cs b/src/TraceEvent/TraceEvent.Tests/Symbols/SymbolReaderTests.cs index 64b6165ba..22514705d 100644 --- a/src/TraceEvent/TraceEvent.Tests/Symbols/SymbolReaderTests.cs +++ b/src/TraceEvent/TraceEvent.Tests/Symbols/SymbolReaderTests.cs @@ -39,7 +39,10 @@ public SymbolReaderTests(ITestOutputHelper output) : base(output) { _handler = new InterceptingHandler(); - _symbolReader = new SymbolReader(TextWriter.Null, nt_symbol_path: null, httpClientDelegatingHandler: _handler); + _symbolReader = new SymbolReader(TextWriter.Null, nt_symbol_path: null, httpClientDelegatingHandler: _handler) + { + AuthorizeDownload = _ => true + }; PrepareTestData(); } @@ -343,6 +346,106 @@ public void SourceLinkSupportsWildcardAndExactPathMappings() Assert.Null(relativePath4); } + [Fact] + public void SourceLinkDownloadWithoutAuthorizerIsDeniedBeforeHttpRequest() + { + var handler = new InterceptingHandler(); + string cacheDirectory = CreateTemporaryDirectory(); + string pdbPath = Path.Combine(s_inputPdbDir, FileName_CsPortablePdb1); + + try + { + using (var reader = new SymbolReader(TextWriter.Null, httpClientDelegatingHandler: handler)) + { + reader.SourceCacheDirectory = cacheDirectory; + reader.SourcePath = string.Empty; + SourceFile sourceFile = GetPortableSourceFile(reader, pdbPath); + + string resolvedPath = null; + WithBuildTimeSourceUnavailable(sourceFile, () => resolvedPath = sourceFile.GetSourceFile()); + + Assert.Null(resolvedPath); + Assert.Empty(handler.Requests); + Assert.Empty(Directory.GetFiles(cacheDirectory)); + } + } + finally + { + Directory.Delete(cacheDirectory, recursive: true); + } + } + + [Fact] + public void SourceLinkDownloadDeniedBeforeHttpRequest() + { + var handler = new InterceptingHandler(); + DownloadAuthorizationRequest authorizationRequest = null; + string cacheDirectory = CreateTemporaryDirectory(); + string pdbPath = Path.Combine(s_inputPdbDir, FileName_CsPortablePdb1); + + try + { + using (var reader = new SymbolReader(TextWriter.Null, httpClientDelegatingHandler: handler)) + { + reader.AuthorizeDownload = request => + { + authorizationRequest = request; + return false; + }; + reader.SourceCacheDirectory = cacheDirectory; + reader.SourcePath = string.Empty; + SourceFile sourceFile = GetPortableSourceFile(reader, pdbPath); + + string resolvedPath = null; + WithBuildTimeSourceUnavailable(sourceFile, () => resolvedPath = sourceFile.GetSourceFile()); + + Assert.Null(resolvedPath); + Assert.NotNull(authorizationRequest); + Assert.Equal(sourceFile.BuildTimeFilePath, authorizationRequest.BuildTimeFilePath); + Assert.Equal(pdbPath, authorizationRequest.SymbolFilePath); + Assert.Equal( + "https://contoso.com/fake-source-link-url/CsPortablePdb1/Program.cs", + authorizationRequest.Uri.AbsoluteUri); + Assert.Empty(handler.Requests); + Assert.Empty(Directory.GetFiles(cacheDirectory)); + } + } + finally + { + Directory.Delete(cacheDirectory, recursive: true); + } + } + + [Fact] + public void SourceLinkDownloadAuthorizationExceptionDeniesBeforeHttpRequest() + { + var handler = new InterceptingHandler(); + string cacheDirectory = CreateTemporaryDirectory(); + string pdbPath = Path.Combine(s_inputPdbDir, FileName_CsPortablePdb1); + + try + { + using (var reader = new SymbolReader(TextWriter.Null, httpClientDelegatingHandler: handler)) + { + reader.AuthorizeDownload = _ => throw new InvalidOperationException("Authorization failed."); + reader.SourceCacheDirectory = cacheDirectory; + reader.SourcePath = string.Empty; + SourceFile sourceFile = GetPortableSourceFile(reader, pdbPath); + + string resolvedPath = null; + WithBuildTimeSourceUnavailable(sourceFile, () => resolvedPath = sourceFile.GetSourceFile()); + + Assert.Null(resolvedPath); + Assert.Empty(handler.Requests); + Assert.Empty(Directory.GetFiles(cacheDirectory)); + } + } + finally + { + Directory.Delete(cacheDirectory, recursive: true); + } + } + /// /// Tests that the checksum matching allows for different line endings. /// Open the PDB and try to retrieve the source code for one of the files, @@ -1715,6 +1818,44 @@ private static TraceLog CreateEmptyInMemoryTraceLog() return new TraceLog(etlxStream); } + private static string CreateTemporaryDirectory() + { + string directory = Path.Combine(Path.GetTempPath(), "SymbolReaderTests_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(directory); + return directory; + } + + private static SourceFile GetPortableSourceFile(SymbolReader reader, string pdbPath) + { + ManagedSymbolModule module = reader.OpenSymbolFile(pdbPath); + SourceLocation sourceLocation = module.SourceLocationForManagedCode(0x06000001, ilOffset: 0); + Assert.NotNull(sourceLocation); + Assert.NotNull(sourceLocation.SourceFile); + return sourceLocation.SourceFile; + } + + private static void WithBuildTimeSourceUnavailable(SourceFile sourceFile, Action action) + { + string renamedSourceFile = null; + try + { + if (File.Exists(sourceFile.BuildTimeFilePath)) + { + renamedSourceFile = sourceFile.BuildTimeFilePath + "." + Guid.NewGuid().ToString("N") + ".orig"; + File.Move(sourceFile.BuildTimeFilePath, renamedSourceFile); + } + + action(); + } + finally + { + if (renamedSourceFile != null) + { + File.Move(renamedSourceFile, sourceFile.BuildTimeFilePath); + } + } + } + /// /// A handler for the in that /// can be used by unit tests to intercept requests to symbol server (for PDB