11865 SMB2 CREATE should validate create context sizes

Review Request #2410 — Created Oct. 23, 2019 and discarded

andy_js
illumos-gate
11865
general
gdamore, gwr

The SMB2 server should check the create context sizes before attempting to decode their data:

"If the size of each individual create context is not equal to the DataLength of the create context, the server MUST fail the request with STATUS_INVALID_PARAMETER."

This impacts the following WPTS test case:

InvalidCreateRequestStructureSize

Prior to applying the fix the above test case failed. With the fix applied it now passes.

andy_js
gwr
  1. Can you provide some analysis about why smb2_decode_create_ctx does not already return invalid parameter for create contexts where the context-specific data is too short?

    1. It's not too short specifically but that it's not the right length or data was supplied when none was expected, or vice versa. In this particular instance we have trouble with the SMB2_CREATE_QUERY_MAXIMAL_ACCESS create context which can be either 0 or 8 bytes long and the InvalidCreateRequestStructureSize test case passes a length of 4 and expects to get STATUS_INVALID_PARAMETER in response, but instead gets STATUS_SUCCESS. When no data is supplied we also end up hitting the check on line 830 and skipping to the next context without ever checking to see if the context should have data.

    2. First, let me gipe a bit about the spec. here, and some of the tests that blindly follow what the spec. says.
      (begin rant)
      A lot of these checks that the spec. is saying the server should instead be specifications on what clients must do.
      For example, when sending a create context with allocation size, the data in the token should be 8 bytes.
      On the server side, the common practice would be to demand only that at least 8 bytes were sent. Nothing bad is going to happen on the server if it were to allow more than 8 bytes. Of course a client doing that should be prepared for a server to respond with "invalid parameter", but it's not actually necessary to specify that a server "MUST" do that.
      I really don't like spec. writers or test writers telling server implementors that things are required when they're really not.
      It breaks the traditional https://en.wikipedia.org/wiki/Robustness_principle common in protocol designs.
      Note that the people who wrote those MS specifications are generally not the protocol designers.

      A server that functions correctly with a correct client should really be OK from the standpoint of what specifications say.
      Any enforcment about what happens with incorrect clients really should all be optional "SHOULD" statements in the spec.
      So in general, I'm not wild about enforcing more than we have to in order to keep from doing stupid things.
      (end rant)

      Anyway, as far as your new error checks go, I'm mostly OK with them, but I'd like to suggest two things:

      1: Can we keep the knowledge of how long the per-context data should be closer to where the decode happens?
      That makes it a little easier to see that the length corresponds to what we're decoding.
      I'm thinking you could add the length checks in the second switch statement here:
      http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/smbsrv/smb2_create.c#846
      That switch does the decoding for create contexts that carry data.

      2: For the create contexts that don't carry data, do we really need to enforce? Why?
      For example, I think the spec. may be confused about SMB2_CREATE_QUERY_MAXIMAL_ACCESS,
      which should have length zero in a request, and length 8 in a reply.
      Do we actually care about create context data that are longer than necessary? if so why? (see rant above:)

  2. 
      
andy_js
andy_js
Review request changed

Status: Discarded

Loading...