Fix: unmanaged memory leak on parsing ill-formed certificates

[sstronin]

There is a memory leak - unmanaged! - because ParsePkcs12 is called from ctors and in case of an exception is thrown allocated resources won't be released inside the call nor by GC.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

There is a memory leak - unmanaged! - because ParsePkcs12 is called from ctors and in case of an exception is thrown allocated resources won't be released inside the call nor by GC.

Author:	sstronin
Assignees:	-
Labels:	area-System.Security, community-contribution
Milestone:	-

[sstronin]

@dotnet-policy-service agree

[martincostello]

Wouldn't the proper fix here be to implement a finalizer on the base class in UnixPkcs12Reader? That's also only touching one file, not 4.

[sstronin]

I've tried to do it the simplest way. The problem is in base class, true, but it also doesn't fully implement Dispose pattern so there will be more changes to separate managed/unmanaged cleanup - and without that it looks unsafe in finalizer.

[martincostello]

doesn't fully implement Dispose pattern

So shouldn't the fix be to make it so it does?

In discussion about a different approach in a different medium

[sstronin]

I'll rework it to proper Dispose pattern

[bartonjs]

I'll rework it to proper Dispose pattern

I think the better approach is to just remove the constructor argument and move it to a load function, that way Dispose will automatically fire and things just work.

using (Pkcs12Reader reader = new WhateverKind())
{
    reader.Load(data);
    ...
}

[sstronin]

That could be done although it'll break class contract. But I think proper Dispose - with finalizer - should be implemented anyway since there is an unmanaged resource and the class is not sealed.

[bartonjs]

This was addressed in #98331 by following the strategy outlined in #93647 (comment)