diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4f65577c..b6f6d61a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -392,6 +392,13 @@ jobs: # #59: nameof must not masquerade as a capture/escape). echo "$out" | grep -qE "OWN001.*'nofLeak' is never disposed" \ || { echo "FAIL: expected OWN001 on the nameof-in-lambda local (not a capture)"; exit 1; } + # owning-factory RECALL (crypto): a System.Security.Cryptography static `Create*` factory + # returning an IDisposable is an owning acquire like File.Open*/Create* -> an undisposed + # one leaks ('rngLeak' = RandomNumberGenerator.Create()); the disposed sibling + # ('shaClean' = SHA256.Create() + Dispose) stays silent. Reduced from the SECOND, + # previously-missed leak in ShareX's DeriveCryptoData. + echo "$out" | grep -qE "OWN001.*'rngLeak' is never disposed" \ + || { echo "FAIL: expected OWN001 on the undisposed crypto-factory acquire"; exit 1; } # dispose-optional (Task), disposed/escaping locals, a `for` loop whose # disposable is disposed after it (`looped`, balanced), a balanced # acquire+dispose in a loop (`whileClean`), a try/finally dispose (`tfClean`, @@ -413,7 +420,7 @@ jobs: # case disposes (no default) -> last case is the tail, no phantom no-match leak. # `ncf`: `ncf?.Dispose()` (null-conditional) in a threaded finally IS a release # (member-binding form), so it is disposed on the return path -> silent (Codex review). - for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured; do + for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean; do if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi done echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, beyond flat)" diff --git a/corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs b/corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs index ab680682..09dbfd12 100644 --- a/corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs +++ b/corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs @@ -2,11 +2,12 @@ // ShareX.UploadersLib/FileUploaders/Vault_ooo.cs:216 (the Vault.ooo file uploader's // DeriveCryptoData), found by mining (docs/notes/real-world-mining.md). // -// Two crypto IDisposables are created on the upload path and never disposed: -// * Rfc2898DeriveBytes — the PBKDF2 deriver, which holds an HMAC -> CAUGHT (OWN001); -// * RandomNumberGenerator.Create() — a FACTORY-acquired IDisposable the extractor -// does not yet recognise (it knows `new` and File.Open*/Create*, not arbitrary -// `X.Create()` factories) -> a known recall gap, see notes.md. +// Two crypto IDisposables are created on the upload path and never disposed, both now +// CAUGHT (OWN001): +// * Rfc2898DeriveBytes — the PBKDF2 deriver, which holds an HMAC (a `new` acquire); +// * RandomNumberGenerator.Create() — a static crypto FACTORY acquire (the extractor +// recognises System.Security.Cryptography `Create*` factories that return an +// IDisposable, alongside `new` and File.Open*/Create*; see notes.md). // // It is a genuine oversight, not a deliberate pattern: the sibling EncryptBytes() in // the same file wraps its aes / MemoryStream / CryptoStream in `using`. The deriver diff --git a/corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md b/corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md index cb3b5a3f..954016e4 100644 --- a/corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md +++ b/corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md @@ -45,15 +45,16 @@ Vault_ooo.cs:216: error: [OWN001] IDisposable local 'rfcDeriver' is never dispos `Dispose()`. Because the deriver is never released on any path the wording is "is never disposed" (vs the partial-path "may not be disposed on every path"). -## The honest caveat — a second leak the extractor misses (recall gap) +## The second leak — also caught (crypto owning-factory) -`DeriveCryptoData` actually leaks **two** crypto disposables. The extractor flags -`rfcDeriver` (acquired via `new`) but **not** `rng = RandomNumberGenerator.Create()`, -an `IDisposable` acquired via a static **factory**. The flow detector recognises `new` -and the `System.IO.File.Open*/Create*` factories (`IsOwningFactory`), but not arbitrary -`X.Create()` factories. Extending the owning-factory set to the common BCL crypto -factories (`RandomNumberGenerator.Create()`, `SHA256.Create()`, `Aes.Create()`, …) is a -separate recall slice; `after.cs` disposes both so the fix is genuinely clean. +`DeriveCryptoData` leaks **two** crypto disposables: `rfcDeriver` (acquired via `new`) +and `rng = RandomNumberGenerator.Create()` (acquired via a static **factory**). The flow +detector now catches both — `IsOwningFactory` recognises the +`System.Security.Cryptography` static `Create*` factories that return an `IDisposable` +(`RandomNumberGenerator.Create()`, `SHA256.Create()`, `Aes.Create()`, …) alongside `new` +and the `System.IO.File.Open*/Create*` factories. (Originally `rng` was a documented +recall gap; the crypto owning-factory slice closed it.) `after.cs` disposes both, so the +fix is genuinely clean. ## Files diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index da6116ba..e38a1677 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -454,7 +454,7 @@ static bool LowerFlowStmt(StatementSyntax st, HashSet tracked, SemanticM && (v.Initializer?.Value is ObjectCreationExpressionSyntax or ImplicitObjectCreationExpressionSyntax || IsPoolRent(v.Initializer?.Value, model) // ArrayPool Rent - || IsOwningFactory(v.Initializer?.Value, model))) // File.Open*/Create* factory + || IsOwningFactory(v.Initializer?.Value, model))) // File / crypto Create* factory nodes.Add(new { op = "acquire", var = v.Identifier.Text, line = LineOf(v) }); return true; case ExpressionStatementSyntax es: @@ -801,26 +801,46 @@ e is InvocationExpressionSyntax i // A factory call that CREATES and hands back a fresh owned IDisposable the caller must // release — recognised via the resolved symbol (curated, the same spirit as -// IsDisposableType is for `new`). System.IO.File.Open*/Create*/*Text return a NEW -// FileStream / StreamReader / StreamWriter that the caller owns exactly as if it had -// `new`'d one, so a local bound to one is an acquire. Curated + symbol-resolved, so a -// borrowed/cached disposable handed back by some other API is never mistaken for an -// owned acquire (precision over recall — the set grows only as ownership is certain). +// IsDisposableType is for `new`). Two families: +// * System.IO.File.Open*/Create*/*Text -> a NEW FileStream / StreamReader / StreamWriter +// the caller owns exactly as if it had `new`'d one. +// * System.Security.Cryptography static `Create*` factories -> a NEW owned IDisposable: +// RandomNumberGenerator.Create(), Aes.Create(), SHA256.Create(), RSA.Create(), +// IncrementalHash.CreateHash(), ... (guarded by static + Create-prefixed + the RESULT +// implementing IDisposable + the crypto namespace, so an instance `CreateEncryptor()` or +// a non-IDisposable `CreateFromName()` is never mistaken for one). +// Curated + symbol-resolved, so a borrowed/cached disposable handed back by some other API is +// never mistaken for an owned acquire (precision over recall — the set grows only as +// ownership is certain). static bool IsOwningFactory(ExpressionSyntax? e, SemanticModel model) { if (e is not InvocationExpressionSyntax i - || model.GetSymbolInfo(i).Symbol is not IMethodSymbol sym - || sym.Name is not ("OpenRead" or "OpenWrite" or "Open" or "Create" - or "OpenText" or "CreateText" or "AppendText")) + || model.GetSymbolInfo(i).Symbol is not IMethodSymbol sym) return false; - INamedTypeSymbol? ct = sym.ContainingType; - if (ct is null || ct.Name != "File") - return false; - INamespaceSymbol? ns = ct.ContainingNamespace; // System.IO.File -> IO - if (ns is null || ns.Name != "IO") - return false; - ns = ns.ContainingNamespace; // IO -> System - return ns is { Name: "System" } && ns.ContainingNamespace is { IsGlobalNamespace: true }; + if (sym.Name is "OpenRead" or "OpenWrite" or "Open" or "Create" + or "OpenText" or "CreateText" or "AppendText" + && sym.ContainingType is { Name: "File" } ft + && IsInNamespace(ft, "System", "IO")) + return true; + if (sym.IsStatic + && sym.Name.StartsWith("Create", StringComparison.Ordinal) + && ImplementsIDisposable(sym.ReturnType) + && IsInNamespace(sym.ContainingType, "System", "Security", "Cryptography")) + return true; + return false; +} + +// Is the type `t` declared in the namespace named by `parts` (outermost-first), e.g. +// IsInNamespace(t, "System", "IO") for System.IO? Walks the containing-namespace chain +// and requires it to bottom out at the global namespace (so `System.IO` matches but a +// nested `Foo.System.IO` would not). +static bool IsInNamespace(INamedTypeSymbol? t, params string[] parts) +{ + var ns = t?.ContainingNamespace; + for (var k = parts.Length - 1; k >= 0; k--, ns = ns?.ContainingNamespace) + if (ns is null || ns.Name != parts[k]) + return false; + return ns is { IsGlobalNamespace: true }; } // The local names of arguments handed to a first-party CONSUMER at this call — a method @@ -1433,7 +1453,7 @@ or ImplicitObjectCreationExpressionSyntax } init candidates.Add(v.Identifier.Text); poolBuffers.Add(v.Identifier.Text); } - else if (IsOwningFactory(v.Initializer?.Value, model)) // File.Open*/Create* factory + else if (IsOwningFactory(v.Initializer?.Value, model)) // File / crypto Create* factory candidates.Add(v.Identifier.Text); } if (candidates.Count == 0) diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index 33a5ec1d..ca4f0f15 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -1,5 +1,6 @@ using System; using System.IO; +using System.Security.Cryptography; using System.Threading; using System.Threading.Tasks; @@ -441,6 +442,26 @@ public void NameofInLambda() Action log = () => System.Console.WriteLine(nameof(nofLeak)); log(); } + + // OWN001 (recall): a crypto IDisposable acquired via a static FACTORY, not `new` — the + // extractor recognises System.Security.Cryptography `Create*` factories that return an + // IDisposable as owning acquires (like File.Open*/Create*). `rngLeak` is created and never + // disposed -> leak. Reduced from the SECOND, previously-missed leak in ShareX's + // DeriveCryptoData (RandomNumberGenerator.Create()). + public void CryptoFactoryLeaks() + { + var rngLeak = RandomNumberGenerator.Create(); + rngLeak.GetBytes(new byte[8]); + } + + // clean: the same kind of factory acquire, disposed on every path -> silent (it is an + // acquire exactly like `new`, so a matching Dispose balances it; not a false positive). + public void CryptoFactoryDisposed() + { + var shaClean = SHA256.Create(); + shaClean.ComputeHash(new byte[1]); + shaClean.Dispose(); + } } // A domain exception type literally named `Exception`, in a non-System namespace — the