TalAloni / SMBLibrary

Free, Open Source, User-Mode SMB 1.0/CIFS, SMB 2.0, SMB 2.1 and SMB 3.0 server and client library
GNU Lesser General Public License v3.0
712 stars 182 forks source link

Not enough credits due to connection resilience issues #226

Closed Sparin closed 10 months ago

Sparin commented 10 months ago

Bug template from dotnet/runtime

Description

SMB2Client throws a Not enough credits exception due to no response from the server on previous commands.

Reproduction steps

  1. Setup SMB share on Windows Server 2022
  2. Succsessfully connect to the SMB share using SMB2Client
  3. Infinitely repeat any request that consumes the client's credits, for example:
    var ntStatus = _fileStore.CreateFile(
    out fileHandle,
    out _,
    "file",
    AccessMask.GENERIC_WRITE | AccessMask.SYNCHRONIZE,
    FileAttributes.Normal,
    ShareAccess.None,
    CreateDisposition.FILE_SUPERSEDE,
    CreateOptions.FILE_NON_DIRECTORY_FILE,
    null);
  4. Disconnect your host physically or virtually from the SMB share's network (for example, connect to a VPN with full traffic redirection or unplug the Ethernet cable).

Example code: https://github.com/Sparin/OutOfCredits

https://github.com/TalAloni/SMBLibrary/assets/7052869/8f2c72af-c47e-4f07-bfe7-54644402f7f7

Expected behaviour

The SMB2Client should be capable of resuming work after a network failure and restoration or notifying the user of the API about the need to perform certain actions, such as reconnecting, for example. If the Not enough credits condition occurs, it MUST either proxy STATUS_INVALID_PARAMETER from the server or request more credits from it.

Actual behaviour

The SMB2Client is unable to proceed any TrySendCommand(SMB2Command request) and remains connected even if the connection is lost. It doesn't respect SocketException with the message An existing connection was forcibly closed by the remote host. image

Known Workarounds

Full reconnect with on STATUS_INVALID_SMB status or an Exception with message Not enough credits, then set m_availableCredits to default value (16) via reflection APIs. Or just create new SMB2Client and connect again.

Configuration

Other information

According to [MS-SMB2] 3.2.4.1.2 Requesting Credits from the Server,

The client MUST NOT decrease its credits to zero, and SHOULD request a sufficient number of credits to support implementation-defined local requirements

This implies that SMB2Client MUST NOT throw a Not enough credits exception. Instead, it should request more credits when necessary. If such a condition occurs, it should provide additional information indicating an unrecoverable condition, and the user should either reconnect or report the issue to the server's provider, referencing [MS-SMB2] 3.3.1.2, which details the Algorithm for the Granting of Credits

The server MUST ensure that the number of credits held by the client is never reduced to zero. If the condition occurs, there is no way for the client to send subsequent requests for more credits.

And the last one is [MS-SMB2] 3.3.5.2.5 Verifying the Credit Charge and the Payload Size

  • If CreditCharge is zero and the payload size of the request or the maximum response size is greater than 64 kilobytes, the server MUST fail the request with the error code STATUS_INVALID_PARAMETER.
  • If CreditCharge is greater than zero, the server MUST calculate the expected CreditCharge for the current operation using the formula specified in section 3.1.5.2. If the calculated credit number is greater than the CreditCharge, the server MUST fail the request with the error code STATUS_INVALID_PARAMETER.

This implies that the user of SMB2Client should expect the appropriate status as specified in the specifications and handle the situation accordingly. Alternatively, the client itself should handle the situation

TalAloni commented 10 months ago

In all cases so far, "Not Enough Credits" was the result of misusing the client (e.g. sending requests in parallel, changing the library code, using a buggy server and etc.)

Tal

Sparin commented 10 months ago

@TalAloni , the client implementation DOES NOT handle a situation where the remote host forcibly closes the connection. It completely ignores this scenario when sending a package. As a result, when the server is down, the client cannot proceed with any further commands, and it fails to report this to the user.

And please, take a look at the code example at least. There is no concurrent code or requests, no changing library, or using a buggy server from Microsoft.

TalAloni commented 10 months ago

I see your point

TalAloni commented 10 months ago

Thank you, a resolution for this issue was pushed to master.