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

Question

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).

Seems weird to both `RegisterForDispose` and use a `using` block that disposes? Then again, `IDisposable.Dispose` is supposed to be idempotent. — Ry-, Nov 13 '17 at 11:21
if you call `RegisterForDispose` - why you also manually dispose your stream? — Evk, Nov 13 '17 at 11:22
... desperation trying to make Veracode happy. Previously it was just the `using`. I will edit the question. — NeedHack, Nov 13 '17 at 11:26
If you keep only the `RegisterForDispose` and remove the `using`, does that fix it? — Ry-, Nov 13 '17 at 11:29
Did you ever get a solution for this? I am having the same code thrown in a very similar scenario. — VinnyGuitara, Jan 16 '18 at 21:14
@VinnyGuitara I did find a solution by making the request stream rewindable in a different way. I’ll try and dig the code out for you in the morning and post and answer as it may be useful to others. — NeedHack, Jan 16 '18 at 21:25

score 2 · Answer 1 · answered Jan 17 '18 at 08:22

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?

Thanks for this NeedHack,although this is not exactly what I was looking for. It seems the MemoryStream in my code is causing this CWE 404 error. Thanks so much for posting though I'm sure it will help someone. — VinnyGuitara, Jan 17 '18 at 13:24

score 0 · Answer 2 · answered May 22 '19 at 17:08

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.

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

2 Answers2