TaskAnalyzer: MSBuildTask0003 misses analyzer-invisible path consumers such as AssemblyName.GetAssemblyName
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
### Summary
`MSBuildTask0003` monitors a fixed set of types: `File`, `Directory`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter`, `FileSystemWatcher`.
That set misses a category I'd call *analyzer-invisible path consumers*: APIs on unrelated types that take a path string and hit the file system. They are exactly as unsafe under multithreaded execution, and in practice they are what survives a migration.
### Evidence
While migrating ~150 tasks across dotnet/arcade, dotnet/source-build-assets and the dotnet/dotnet VMR, the analyzer was enabled and every build was clean (0 warnings, 0 errors). A manual audit afterwards found 9 real defects. **Two of the nine were the same missing API**, and it was the single most-repeated defect of the entire exercise:
```csharp
// dotnet/arcade GetAssemblyFullName
AssemblyName.GetAssemblyName(assemblyPath) // raw, relative-capable input
// dotnet/dotnet eng/tools CheckForPoison.cs:223
AssemblyName asm = AssemblyName.GetAssemblyName(fileToCheck);
if (... IsAssemblyFromSbrp(TaskEnvironment.GetAbsolutePath(fileToCheck))) // resolved
else if (IsAssemblyPoisoned(TaskEnvironment.GetAbsolutePath(fileToCheck))) // resolved
```
The second is worth dwelling on. The call is only an "is this an assembly?" probe, and the enclosing `catch` treats any exception as "not an assembly". So an unresolved relative path throws `FileNotFoundException`, gets swallowed, and **both poison checks are silently skipped** — a false negative in source-build's leak-detection gate. A wrong path did not produce a wrong path error; it produced a silently weakened security check.
### Suggested additions
| API | Notes |
|---|---|
| `AssemblyName.GetAssemblyName(string)` | highest observed frequency |
| `XDocument.Load(string)` / `XElement.Load(string)` / `XmlDocument.Load(string)` | string overloads only; stream overloads are safe |
| `XmlReader.Create(string)` / `XmlWriter.Create(string)` | as above |
| `ZipFile.OpenRead` / `Open` / `ExtractToDirectory` / `CreateFromDirectory` | |
| `X509CertificateLoader` / `X509Certificate2(string)` | |
| `File.ReadAllText` etc. are covered; `Path.GetDirectoryName` is fine (pure string op) | |
Distinguishing string overloads from stream overloads matters — in my own tooling the stream overloads were the dominant false-positive source.
### The deeper suggestion: invert the rule shape
An allowlist of sinks is unbounded; every library that takes a path is a potential entry. The *sources*, however, are bounded and known to the analyzer: task input properties (`string`, `ITaskItem`, `ITaskItem[]`).
A taint-style rule — "a task input property reaches a parameter that looks like a path without passing through `TaskEnvironment`" — is bounded, and catches APIs nobody has enumerated, including ones in third-party packages. That would additionally have caught a defect this API list still misses:
```csharp
// dotnet/arcade InstallDotNetTool
// DestinationPath / DotnetPath / WorkingDirectory flow raw through
// IFileSystem and ICommandFactory abstractions -- no System.IO type appears
// anywhere in the task.
```
Related: #11301.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.