Skip to content

feat(extractor): recognise crypto Create* factories as owning acquires (recall)#60

Merged
PhysShell merged 1 commit into
mainfrom
claude/crypto-owning-factory
Jun 21, 2026
Merged

feat(extractor): recognise crypto Create* factories as owning acquires (recall)#60
PhysShell merged 1 commit into
mainfrom
claude/crypto-owning-factory

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 21, 2026

Copy link
Copy Markdown
Owner

The recall gap (from #58)

The corpus case locked in #58 documented a gap: ShareX's DeriveCryptoData leaks two crypto disposables, but we only caught one —

RandomNumberGenerator rng = RandomNumberGenerator.Create();   // leak #2 — MISSED (factory)
rng.GetBytes(salt);
Rfc2898DeriveBytes rfcDeriver = new Rfc2898DeriveBytes(...);   // leak #1 — caught (`new`)

rng is an IDisposable acquired via a static factory, which the flow detector didn't recognise (it knew new, ArrayPool Rent, and System.IO.File.Open*/Create*, not X.Create()).

The fix

IsOwningFactory now recognises a second owning-factory family: a System.Security.Cryptography static Create* method whose result implements IDisposableRandomNumberGenerator.Create(), Aes.Create(), SHA256.Create(), RSA.Create(), IncrementalHash.CreateHash(), … A local bound to one is an acquire exactly like new, so an undisposed one is OWN001.

if (sym.IsStatic
    && sym.Name.StartsWith("Create", StringComparison.Ordinal)
    && ImplementsIDisposable(sym.ReturnType)
    && IsInNamespace(sym.ContainingType, "System", "Security", "Cryptography"))
    return true;

Kept tight for precision — the guard is static + Create-prefixed + the result implements IDisposable + the declaring type is in the crypto namespace, so an instance aes.CreateEncryptor() (not static) and a non-IDisposable CryptoConfig.CreateFromName() (returns object) are never mistaken for an owned acquire. The File and crypto namespace checks are factored through a small IsInNamespace helper (the File branch stays equivalent).

Pinned in CI

Two FlowLocalsSample.cs cases in the wpf-extractor --flow-locals step:

method shape verdict
CryptoFactoryLeaks RandomNumberGenerator.Create(), never disposed OWN001
CryptoFactoryDisposed SHA256.Create() + Dispose() silent (control — proves the factory result is an acquire, not a false positive)

The sharex-rfc2898 corpus before.cs now catches both leaks; its notes/comment are updated to mark the gap closed (the catch count is unchanged — still one case — so --min-recall stays 10). Validated locally: rngLeak → OWN001, shaClean silent, corpus 8/8. The extractor source→facts step is validated in CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED


Generated by Claude Code

Summary by CodeRabbit

  • New Features

    • Extended detection to recognize cryptographic factory methods as disposable resources requiring proper cleanup.
  • Bug Fixes

    • Improved leak detection for cryptography objects created via factory methods.
  • Tests

    • Added test cases demonstrating disposal and non-disposal scenarios for cryptographic objects.
  • Documentation

    • Updated documentation to reflect enhanced support for cryptography factory method detection.

Closes the recall gap documented by #58: ShareX's DeriveCryptoData leaks TWO crypto
disposables — rfcDeriver (`new Rfc2898DeriveBytes`, already caught) and
rng = RandomNumberGenerator.Create() (a static factory we missed). IsOwningFactory now
recognises a second owning-factory family alongside System.IO.File.Open*/Create*: a
System.Security.Cryptography static `Create*` method whose result implements IDisposable
(RandomNumberGenerator.Create(), Aes.Create(), SHA256.Create(), RSA.Create(),
IncrementalHash.CreateHash(), ...). A local bound to one is an acquire exactly like `new`,
so an undisposed one is OWN001.

Kept tight for precision: the guard is static + Create-prefixed + the RESULT type
implements IDisposable + the declaring type is in System.Security.Cryptography, so an
instance `aes.CreateEncryptor()` (not static) and a non-IDisposable `CryptoConfig
.CreateFromName()` (returns object) are never mistaken for an owned acquire. Factored the
File and crypto namespace checks through a small IsInNamespace helper.

Pinned by two FlowLocalsSample cases in the wpf-extractor --flow-locals step:
CryptoFactoryLeaks (RandomNumberGenerator.Create() never disposed -> OWN001) and
CryptoFactoryDisposed (SHA256.Create() + Dispose -> silent, the control proving the factory
result is modelled as an acquire, not a false positive). The sharex-rfc2898 corpus before.cs
now catches both leaks; its notes/comment are updated to mark the gap closed. Validated
locally: rngLeak -> OWN001, shaClean silent, corpus 8/8.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c63b9ec-78f6-4809-834f-53763d0e54f2

📥 Commits

Reviewing files that changed from the base of the PR and between 31b04ca and 2de9470.

📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs
  • corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md
  • frontend/roslyn/OwnSharp.Extractor/Program.cs
  • frontend/roslyn/samples/FlowLocalsSample.cs

📝 Walkthrough

Walkthrough

IsOwningFactory in the Roslyn extractor is extended to recognize static Create* methods in System.Security.Cryptography namespaces as owning disposables, alongside the existing System.IO.File family. Two sample methods illustrating the leak and clean paths are added, CI flow-locals assertions are updated, and corpus documentation is corrected.

Changes

Crypto Factory Ownership Detection

Layer / File(s) Summary
IsOwningFactory extension and IsInNamespace refactor
frontend/roslyn/OwnSharp.Extractor/Program.cs
IsOwningFactory gains a second branch that recognizes static Create* methods in System.Security.Cryptography whose return types implement IDisposable. IsInNamespace is refactored to use structured namespace-chain matching. Two call-site comments are updated to reflect "File / crypto Create* factory" coverage.
FlowLocalsSample crypto factory test methods
frontend/roslyn/samples/FlowLocalsSample.cs
Adds using System.Security.Cryptography and two new methods: CryptoFactoryLeaks() (undisposed RandomNumberGenerator.Create()) and CryptoFactoryDisposed() (disposed SHA256.Create()).
CI assertions and corpus documentation
.github/workflows/ci.yml, corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs, corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md
CI adds an OWN001 assertion for rngLeak and adds shaClean to the silent allowlist. Corpus comments and notes are corrected to reflect that both rfcDeriver and rng are now detected disposables.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • PhysShell/Own.NET#54: Introduced the original IsOwningFactory logic covering System.IO.File Open*/Create*/*Text patterns that this PR directly extends with the System.Security.Cryptography branch.

Poem

🐇 Hop, hop! The factory's caught at last,
RandomNumberGenerator.Create() held fast.
No leak escapes my twitching nose,
From SHA256 to where the cipher goes.
Both crypto friends disposed so clean —
The tidiest extractor I have seen! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: extending the extractor to recognize crypto Create* factories as owning acquires, which directly addresses the recall gap described in the PR objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/crypto-owning-factory

Comment @coderabbitai help to get the list of available commands and usage tips.

// DeriveCryptoData (RandomNumberGenerator.Create()).
public void CryptoFactoryLeaks()
{
var rngLeak = RandomNumberGenerator.Create();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants