Keep Sensitive Data Off Heap

Question

While scanning an ASP.net MVC application using Checkmarx, I regularly see heap inspection vulnerabilities. So I started to wonder if I could use a custom model binder or the built-in ByteArrayModelBinder to keep passwords and other sensitive strings out of the heap, for controlled disposal. I came up with the following solution which posts and displays the sensitive data via a byte array, but I am wondering if this data is still making its way into the heap through a string somewhere. (Note: The display action is just for debugging.)

ViewModel

public class ByteArrayViewModel
{
    public byte[] SensitiveData { get; set; }        
}

Input View

@model MvcHandlingSensativeStrings.Models.ByteArrayViewModel

@{
    ViewBag.Title = "byte[] Post";
    Layout = "~/Views/Shared/_Layout.cshtml";
}

<h2>@ViewBag.Title</h2>
@using (Html.BeginForm("Post", "ByteArray", FormMethod.Post))
{
    @Html.TextBoxFor(m=>m.SensitiveData);
    <button type="submit">Send</button>
}

Controller

public class ByteArrayController : Controller
{
    public ActionResult Index()
    {
        return View(new ByteArrayViewModel());
    }

    [HttpPost]
    public ActionResult Post(ByteArrayViewModel viewModel)
    {
        try
        {
            // Handle sensitive data here
            return View("Display", viewModel);
        }
        catch
        {
            return View("Index");
        }
        finally
        {
            // Clear sensitive data from memory
            //Array.Clear(viewModel.SensitiveData, 0, viewModel.SensitiveData.Leng
        }
    }

    public ActionResult Display(ByteArrayViewModel viewModel)
    {
        return View(viewModel);
    }
}

Display View

@model MvcHandlingSensativeStrings.Models.ByteArrayViewModel

@{
    ViewBag.Title = "byte[] Display";
    Layout = "~/Views/Shared/_Layout.cshtml";
    string s = Convert.ToBase64String(Model.SensitiveData);
}

<h2>@ViewBag.Title</h2>
<p>@s</p>
<p>@Model.SensitiveData.GetType().ToString()</p>

Display Output

* Update *

This shows that before ByteArrayModelBinder or any other model binder executes, form parameters are stored in an array of strings and are thus susceptible to heap inspection.

* Update 2 *

If you peek at Microsoft's implementation of NetworkCredential, you will noticed that even though the Password property is a string, SecureString is used underneath for storage.

Answer 1

The answer is no, you are not making this one bit more secure.

To avoid storing secrets in memory that could be recovered, you need to use the SecureString class in C# (or the underlying crypto API in Windows). This will go through some effort to make the secrets harder to recover by erasing them on dispose, but also making sure they never end up in the pagefile.

But more importantly, printing the secrets out to the web page (even if using TLS) obviously exposes the secret to all layers of the web stack, making it vulnerable to multiple local attacks, as well as browser attacks on the client.

The solution is to never ever store passwords in clear text, and instead use salted hashes. Don't try to invent stuff like this yourself, use proven solutions for verifying and storing passwords securely. It is very easy to overlook something that completely voids the security of your application. For example, the crypto API has a facility for this. See for example here: How to store and retrieve credentials on windows using C#

If you absolutely need to accept "secret" user data coming from a form, and you want to protect against attacks on the server:

1. Protect the process against remote exploits in other processes on the same machine (run in isolated VM, run in Docker, run everything as separate (non-root) users
2. To protect against local attacks, make sure that the sensitive data does not end up in persistent storage (i.e. SecureString). This will be hard to do using your current implementation as already pointed out, as you need to harden all layers of your application, from SSL/TLS termination and down to the controller.

Answer 2

This is an example of how static analysis tools cannot always be taken seriously.

When passwords and sensitive data come into your application, guess what type of structure it is arriving in? A RequestObject, which comes in as a string. Guess what? It's already on the heap, and there is nothing you can do about it. Sure, you can play games trying to prevent it from being put on the heap again when you pass it on to your structures, but that is not going to change the fact that it is already uncontrolled elsewhere.

Don't be fooled into thinking SecureString and other gimmicks are the solution. Microsoft now acknowledges that SecureString isn't what it claims to be, and it is being dropped from .Net Core.

Bottom line: ignore tools like Checkmarx and Fortify when they complain about heap inspection vulnerabilities. These rules should be thrown out.