"Improper Resource Shutdown or Release" error from Veracode, why?

user_44950 0
Viewed 3699

What is wrong with this code? Moreover how do I fix it?

public class BodyStreamMiddleware
{
    private readonly RequestDelegate _next;

    public BodyStreamMiddleware(RequestDelegate next) { _next = next; }

    public async Task Invoke(HttpContext context)
    {
        //  Replace the FrameRequestStream with a MemoryStream.
        //  This is because the MemoryStream is rewindable, the FrameRequestStream is not.
        //  This allows ExceptionFilters to read the body for logging purposes
        string bodyAsText;
        using (var bodyReader = new StreamReader(context.Request.Body))
        {
            bodyAsText = bodyReader.ReadToEnd();
        }
        var bytesToWrite = Encoding.UTF8.GetBytes(bodyAsText);

        using (var memoryStream = new MemoryStream())
        {
            memoryStream.Write(bytesToWrite, 0, bytesToWrite.Length);
            memoryStream.Seek(0, SeekOrigin.Begin);
            context.Request.Body = memoryStream;


            //  Tell ASP.NET core to dispose the memory stream when the request ends 
            // (only added in desperation)
            context.Response.RegisterForDispose(memoryStream);
            await _next.Invoke(context);
        }
    }
}

When I run a Veracode scan over the above it gives me

404 Improper Resource Shutdown or Release

I understand that a downstream process could grab a reference to the memory stream and hang onto it, but fail to see how that is any different to the default asp.net behaviour (i.e. something could grab hold of the FrameRequestStream).

veracode .net-core c#
2 Answers

Answering my own question for the benefit of others...

I managed to fix/sidestep this issue by making the request stream re-readable in a different way. This keeps Veracode happy, although I am not at all convinced there was anything wrong with the previous code.

Stick this in yer startup class...

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
    ...
    app.Use(async (context, next) => {
        context.Request.EnableRewind();
        await next();
    }
    ...
});

I have blogged about this here https://needhack.wordpress.com/2018/01/17/make-the-request-stream-re-readable-in-asp-net-core-with-c/, because we are all told to blog because if we do, we'll become the next Scott Hanselman, increase our rates and give up the daily commute, right?

user_449500

This is a problem within Veracode. It takes issue whenever you use an asynchronous action inside of a using block.

In the below example, DoWork is fine. DoWorkAsync is flagged as improper release.

    public static byte[] DoWork()
    {
        var buffer = new byte[10];
        using (var stream = new MemoryStream())
        {
            for (int i = 0; i < 10; i++)
            {
                stream.Write(buffer, i, 1);
            }

            return stream.ToArray();
        }
    }

    public static async Task<byte[]> DoWorkAsync()
    {
        var buffer = new byte[10];
        using (var stream = new MemoryStream())
        {
            for (int i = 0; i < 10; i++)
            {
                await stream.WriteAsync(buffer, i, 1);
            }

            return stream.ToArray();
        }
    }

Further, it seems to only identify this issue once per scan. This leads to even more confusion, as you try to figure out why one using block was flagged and others were not. You can prove this by commenting out the flagged block and running the scan again, and seeing that it now flags the same issue on a different using block.

If you look around Stack Overflow, you'll see that Veracode frequently produces false positives when it comes to asynchronous C#. I would recommend doing what you can to get this marked as mitigated as a false positive or mitigated by design, and not changing perfectly valid code for the sake of a faulty scan.

user_12388950
Related