Skip to content
Notifications
Clear all

Just built a rule to catch unsafe deserialization in our .NET code

5 Posts
5 Users
0 Reactions
27 Views
(@devops_contrarian_42)
Honorable Member
Joined: 6 months ago
Posts: 479
Topic starter   [#16279]

Everyone's rushing to bolt SAST onto their pipelines. Semgrep's the new darling. Fine. Built a rule for .NET's BinaryFormatter deserialization. The classic.

```yaml
rules:
- id: unsafe-binaryformatter-deserialize
message: Potential unsafe deserialization with BinaryFormatter.
languages:
- csharp
severity: WARNING
pattern: |
new BinaryFormatter().Deserialize(...)
```

Works. Catches the obvious. But let's be real: if your team is using `BinaryFormatter` in 2024, a lint rule is the least of your problems. You need a time machine back to 2010.

The real question: does adding this to a 500-rule Semgrep config actually improve security, or just create another batch of alerts for devs to ignore? Most teams would be better served by a one-line grep in a pre-commit hook.


Keep it simple


   
Quote
(@danielk)
Honorable Member
Joined: 3 months ago
Posts: 382
 

Agreed on the alert fatigue problem. But that rule's too basic - it misses `BinaryFormatter.Deserialize` called on an existing instance variable or passed as a parameter. Need a pattern-either.

More importantly, a SAST rule without a corresponding IaC rule to block the assembly load is just noise. You've found the code, but the deployment pipeline still allows `System.Runtime.Serialization.Formatters.Binary`.


Trust but verify, then don't trust.


   
ReplyQuote
(@code_weaver_max)
Reputable Member
Joined: 4 months ago
Posts: 370
 

Good point about `pattern-either`. My first version missed those cases too. Here's what I ended up using:

```yaml
pattern-either:
- pattern: new BinaryFormatter().Deserialize(...)
- pattern: $BF.Deserialize(...)
```

The IaC angle is critical though. I've seen teams "fix" the finding by wrapping the call in a try-catch and logging the error, which is... not the point. Blocking the assembly load is the real fix, otherwise it's just security theater.


Prompt engineering is the new debugging


   
ReplyQuote
(@bob88)
Reputable Member
Joined: 2 months ago
Posts: 241
 

You're right about alert fatigue being the hidden cost here. I've watched teams drown in Semgrep output because they cargo-culted a massive rule set without a remediation plan.

The one-line grep in a pre-commit hook is a better first step because it forces a conversation *before* the code lands. A SAST rule in the pipeline just becomes a ticket in the backlog, usually triaged as "won't fix" because replacing BinaryFormatter in some legacy module is a three-week refactor no one approved.

The real win isn't catching the pattern, it's having a sanctioned, drop-in replacement ready. If you don't have a Migration.CustomSerializer package sitting in your internal feed that teams can actually use, you're just measuring a problem you can't solve.


Migrate once, test twice.


   
ReplyQuote
(@jenniferl)
Trusted Member
Joined: 3 months ago
Posts: 31
 

Totally feel you on the alert fatigue. Throwing a rule into a massive config is like adding another alarm that everyone learns to mute.

But I think there's a middle ground: starting with a few high-signal rules like this one *and* pairing it with a documented fix. If the SAST finding links directly to a wiki page showing how to swap in MessagePack or Protobuf, it turns from noise into a guided fix.

You're right that a pre-commit hook forces the conversation earlier. Maybe the rule's real value is in the legacy scan, to build a business case for that refactor budget. Finding ten instances gets you the sprint allocation to actually replace them.


Always testing the next best thing.


   
ReplyQuote