microsoft / infersharp

Infer# is an interprocedural and scalable static code analyzer for C#. Via the capabilities of Facebook's Infer, this tool detects null dereferences, resource leaks, and thread-safety violations. It also performs taint flow tracking to detect critical security vulnerabilities like SQL injections.
MIT License
727 stars 28 forks source link

False positive on stream leak in HttpClient StreamContent and CryptoStream? #187

Open akatakritos opened 1 year ago

akatakritos commented 1 year ago

Found this in a real project. We're uploading files to a rather interesting HTTP API that expects its content to be base64 encoded. Rather than loading large files into memory and running Convert.ToBase64String I found some guidance on how to use CryptoStream to create a stream that base64 encodes on the fly.

using System.Security.Cryptography;

await MakeApiCall();

async Task MakeApiCall()
{
    using var client = new HttpClient(); // get this from IHttpClientFactory in the real world

    using var request = new HttpRequestMessage(HttpMethod.Post, "https://example.com/")
    {
        Content = GetBase64FileContent()
    };

    using var response = await client.SendAsync(request);
    var responseContent = await response.Content.ReadAsStringAsync();
    Console.WriteLine(responseContent);
}

StreamContent GetBase64FileContent()
{
    // Leak reported here, but I think StreamContent will Dispose CryptoStream which will Dispose the FileStream
    return new StreamContent(new CryptoStream(File.OpenRead("C:\\temp\\test.txt"), new ToBase64Transform(), CryptoStreamMode.Read));
}

Ran with docker:

docker run -v /Users/matt.burke/projects/ConsoleApp3/ConsoleApp3/bin/Debug/net6.0:/infersharp/binary_path --rm mcr.microsoft.com/infersharp:v1.4 /bin/bash -c "./run_infersharp.sh binary_path; cp infer-out/report.txt /infersharp/binary_path/report.txt"

Output

/Users/matt.burke/projects/ConsoleApp3/ConsoleApp3/Program.cs:23: error: Pulse Resource Leak
  Resource dynamically allocated by constructor System.Security.Cryptography.CryptoStream() on line 23 is not closed after the last access at line 23, column 5.

Found 1 issue
                Issue Type(ISSUED_TYPE_ID): #
  Pulse Resource Leak(PULSE_RESOURCE_LEAK): 1

Reading through all the framework code, I think that when HttpRequestMessage is disposed, it will dispose the StreamContent which will dispose the CryptoStream which will dispose the FileStream.

matjin commented 1 year ago

Thanks for posting this issue! It looks like you're correct -- the Httprequest message/streamcontent/cryptostream all in turn dispose of the underlying. We'll add this into our next batch of infer backend model updates.