P/Invoke declarations should not be safe critical

ID

csharp.pinvoke_safe_critical

Severity

high

Resource

Privilege

Language

CSharp

Tags

CWE:693, FxCop:CA5122, NIST.SP.800-53, OWASP:2021:A1, PCI-DSS:6.5.8

Description

The [SecuritySafeCritical] attribute is used to mark a member sucha as a method as both security-critical and safe for use by transparent code.

Types or members that are marked with the SecuritySafeCritical attribute can be accessed by partially trusted types and members. These partially trusted types and members can be within any assembly that is marked with the SecurityTransparent or AllowPartiallyTrustedCallers (APTCA) attribute, or they are partially trusted for other reasons, such as being loaded into partial trust. Code that is marked with SecuritySafeCritical must be subject to a rigorous security audit to ensure that it can be used safely in a secure execution environment. Security-safe-critical code must validate the permissions of callers to determine whether they have authority to access protected resources used by the code.

Partially trusted code is no longer supported. The SecuritySafeCritical attribute has no effect in .NET Core.

Rationale

Methods are marked as SecuritySafeCritical when they perform a security sensitive operation, but are also safe for use by transparent code. One of the fundamental rules of the security transparency model is that transparent code may never directly call native code through a P/Invoke. Therefore, marking a P/Invoke as security safe critical will not enable transparent code to call it, and is misleading for security analysis.

When a P/Invoke is marked as [SecuritySafeCritical], it is not safe for use by transparent code.

[assembly: AllowPartiallyTrustedCallers]

internal sealed class NativeMethods
{
    private NativeMethods() { }

    // FLAW: P/Invoke is marked as SecuritySafeCritical
    [SecuritySafeCritical]
    [DllImport("kernel32.dll", EntryPoint = "GetCurrentProcess", CharSet = CharSet.Auto, SetLastError = true)]
    public static extern IntPtr GetCurrentProcessPInvoke();

    [SecuritySafeCritical]
    public static IntPtr GetCurrentProcess()
    {
        IntPtr Result = GetCurrentProcessPInvoke();
        return Result;
    }
}

Remediation

To make P/Invoke available to transparent code, expose a SecuritySafeCritical wrapper method, callable from transparent code:

[assembly: AllowPartiallyTrustedCallers]

internal sealed class NativeMethods
{
    private NativeMethods() { }

    // Fixed: P/Invoke is marked as SecurityCritical instead.
    // Transparent code can call the wrapper method GetCurrentProcess()
    [SecurityCritical]
    [DllImport("kernel32.dll", EntryPoint = "GetCurrentProcess", CharSet = CharSet.Auto, SetLastError = true)]
    public static extern IntPtr GetCurrentProcessPInvoke();

    [SecuritySafeCritical]
    public static IntPtr GetCurrentProcess()
    {
        IntPtr Result = GetCurrentProcessPInvoke();
        return Result;
    }
}

Configuration

The detector has no configurable options.

References