11665 SMB2 NEGOTIATE Security Mode handling is wrong

Review Request #2287 — Created Sept. 9, 2019 and submitted

11659, 11665, 11670
gdamore, gwr

11665 SMB2 NEGOTIATE Security Mode handling is wrong
11659 SMB2 protocol version negotiation needs work
11670 SMB2_FLAGS_SIGNED is not valid during NEGOTIATE

Before this change, running the Windows Protocol Test Suite with only the SMB2/3 Negotiate tests enable yields a result of:

59 / 107 Passed, 41 Failed, 7 Inconclusive

After this fix the result is:

100 / 107 Passed, 0 Failed, 7 Inconclusive

The inconclusive tests are inconclusive because the require SMB 3.1 support.

  • 0
  • 0
  • 5
  • 2
  • 7
Description From Last Updated
  1. Ship It!
  1. There was one weird smbtorture test that this code was dealing with. Let me also check on that and get back to you.

  2. How about we remove the return(-1) but leave the rest of the if (...status != 0) block?
    I believe the spec. still calls out some "invalid parameter" cases for which we might need to do an smb2sr_put_error call (at some point in the future).
    e.g. no matching protocol, I think. I know I've seen Windows return invalid protocol to negotiate, so I know it's possible.
    I'd rather not have smb2_status ignored here.

    1. Sure, I can put that part of the code back in. As for NEGOTIATE returning invalid parameter I have 2 more patches coming which add 3 new conditions where that error will get returned.

    2. Thanks. One bit of existing desigin of the code structure might not have been obvious:
      I was trying to have just one place do the smb2sr_put_error calls, generally in the top-level command handler dispatch function. Of course SMB2 Negotiate is special becauase of the couple ways the common part can be called, but this was one of those "top level" places where we wanted that, IIRC.
      One could also just sprinkle smb2sr_put_error calls everywhere (sort of like SMB1 did) but that doesn't work as nicely with bubbling up NT_STATUS values through various callers, hopefully ending in the top-level dispatch. (better for dtrace etc.)
      OK, enough about that.

  2. Oh, sorry. I advised you incorrectly.
    If we want to handle (e.g.) NT_STATUS_INVALID_PARAMETER here we need to do the put error here AND return zero.

    Perhaps also add a comment here that there are no protocol failures after this point.
    The rc!=0 test is just for the (probably impossible) case of running out of message space.

    1. Actually, maybe the caller(s) of smb2_negotiate_common is(are) a better place for the call to smb2sr_put_error.
      I probably want to look longer at this.

    2. I have a better idea. I'll update this to include one of my other fixes, which adds a case where NT_STATUS_INVALID_PARAMETER gets set and requires that the connection get dropped.

    3. Looks like I was wrong about having to drop the connection. Still, I think it's better to include the other fix anyway.

  2. OK, I think I'd recommend adding the put error call here, just before the send reply call:

        if (sr->smb2_status != 0)
            smb2sr_put_error(sr, sr->smb2_status);
  2. Also, if failure in smb2_negotiate_common is not going to cause connection drop, it might be better to change the return to a ulong NT_STATUS and let callers do the smb2sr_put_error like most other places.

  1. This is a good start, but I'd like to ask for some restructuring.

    We've tried to always place the START probes after everythign has been decoded (unless that's impractical for some reason) so that the START probe can examine the fully decoded state.
    See smb2_create for a complex example of the expected code structure.

    In general, every SMB2 dispatch handler should be structured like this:

    • Variable inializations and allocations
    • Decode fixed parts.
    • Decode variable parts. (generally, decode errors are a protocol violation and should cause a disconnect, as "all bets are off". Generally disconnect happens by returning a non-zero "dispatch code" to the common dispatch function.)
    • Call the dtrace START probe
    • Do error checking on what we decoded (errors after the START probe should goto a label just before the DONE probe)
    • Execute the "action" for the request, usually via some sub-function (possibly shared with SMB1)
    • Call the dtrace DONE probe
    • Encode the results (either success or failure). In general, encoding should not fail.
    • Common cleanup of locals (may be a label here for goto from decode section errors)
    • Return to common dispatch (where the reply is sent, either immediately or at the end of a compound).

    Note that Negotiate is unusual in that it's not called by the common dispatch function.
    As written smb2_negotiate_common was trying to be like a normal dispatch handler function,
    and smb2_newrq_negotiate and smb1_negotiate_smb2 were intended to contain the actions that
    would normally be expected in the caller of a normal dispatch handler function.

    Sorry that wasn't more clear. I thought about putting that in a big comment somewhere, but I'm not sure where it would go that people would actually see it. Reach out to me (privately) if you'd like to talk about any of this. Thanks! -GWR

    1. One thing I forgot in the above:
      Before the dtrace DONE probe is called, make sure smb2_status is set to whatever we're going to return to the client.
      Often we need to set smb2_status before the DONE probe and call smb2sr_put_error after it.

  2. See later comment about doing more of this in smb2_negotiate_common() if possible.

    Also a nit: why insert this one assignment here vs after the assignment of smb2_cmd_code?

    1. Funnily enough this was moved in an attempt to make things more closely resemble what is happening in smb2_dispatch.c. As for why I chose to do the assignment first, there's no reason. I'll move it in my next update.

  3. See my comment in smb2_negotiate_common, relevant to these additions.

  4. This move is breaking some rules (OK, unwritten rules) about the dtrace probes. See my longer comment about this in the general part of the review.

    1. While I'm not sure it really matters (since we aren't decoding the variable part into the smb request object) it's easy enough to change and so I'll change it in my next update.

  5. Would it be difficult to keep all the "build an SMB2 reply" work here in smb2_negotiate_common()?
    That was the intent of this funciton.

    1. I think it depends on how much you want the smb2_negotiate_common function to resemble a typical dispatch handler. As far as I can tell the other dispatch handlers only encode the body of the response.

    2. Yeah, that's not like other dispatch functions.  That was done just to try to put stuff common to the two callers in smb2_negotiate_common.
    3. I took a stab at moving the reply logic into smb2_negotiate_common (and with it, the version negotation logic too) but was not happy with the results. This was mainly because I didn't like that I'd have to deal with both errors in the request (such as unsupported dialect) and internal errors (such as smb_mbc_encodef failing). By making it the callers responsibility to encode the header I don't have to, and I think it also increases readability too.

    4. If it helps, smb_mbc_encodef won't fail unless the system can't allocate memory or something.
      That's why you see a lot of places just cast its return value to void.
      So don't spend a lot of time worrying about failure handling for that.

    5. I took another stab at it and think I've come up with a version which is not completely horrible and allows the reply logic to live in smb2_negotiate_common. This did mean moving the dialect version negotiation logic into smb2_negotiate_common and part of that involved changing smb1_negotiate_smb2 to create an SMB2-style array of dialects.

  1. Thanks for simplifying this change. I have just a few bits of feedback remaining.

  2. I think we want the final (overwrite=true) call to smb2_encode_header() after smb2_status is set and after any calls to smb2sr_put_error(sr, status);
    (Right before smb2_send_reply)

    At least that's how it happens elsewhere.
    Same for the later call at line 317

  3. Did you forget to limit the version_cnt value that's passed to smb_mbc_decodef?

    1. It's limited by the preceeding check in the if statement.

    2. Ah right, via if condition short-circuit evaluation. OK.

  4. If you change this to

    sr->smb2_hdr_flags |= SMB2_FLAGS_SERVER_TO_REDIR;

    you could get rid of saved_hdr_flags.

    1. I didn't want to return SMB2_FLAGS_SIGNED in the response.

    2. I see. I guess my nit here boils down to style, so take it or leave it, but I'd find it a little more obvious to do the signed bit test separately, i.e. after the version_cnt test:

           * [MS-SMB2] Verifying the Signature
           * "If the SMB2 header of the SMB2 NEGOTIATE request has the
           * SMB2_FLAGS_SIGNED bit set in the Flags field, the server
           * MUST fail the request with STATUS_INVALID_PARAMETER."
          if ((sr->smb2_hdr_flags & SMB2_FLAGS_SIGNED) != 0) {
              sr->smb2_hdr_flags &= ~SMB2_FLAGS_SIGNED;
              status = NT_STATUS_INVALID_PARAMETER;
              goto errout;
    3. And you'd probably want that check early (first?) so other error checks can't go out with the signed flag if there are multiple problems with the request.

  5. Same comment as on the earlier smb2_encode_header call. (line 165)

  2. One other thing I just noticed. We really should be checking SMB2_FLAGS_SERVER_TO_REDIR here too (like smb2_dispatch does). This is also a good place to do the "reserve header space" too (also like dispatch)

    +   if ((sr->smb2_hdr_flags & SMB2_FLAGS_SERVER_TO_REDIR) != 0)
    +       return (-1);
    +   sr->smb2_hdr_flags |= SMB2_FLAGS_SERVER_TO_REDIR;
    +   /* Reserve space for the reply header. */
    +   (void) smb2_encode_header(sr, B_FALSE);
  1. Thanks for your patience with all my nits etc.

Review request changed

Status: Closed (submitted)