SMB 3.1.1 support for server

Review Request #2536 — Created April 15, 2020 and updated

ams
illumos-gate
12513
general

Added these features:
- Preauthentication integrity
- AES-128-GCM encryption cipher

Tested using Win10 with Wireshark on client side and omnios with different combination of properties of smb/server smf service:
smbd/max_protocol ( "", "2.1", "3.02", "3.11" )
smbd/encrypt ("", "disabled", "enabled", "required")
smbd/encypt_cipher ("", "aes-128-ccm", "aes-128-gcm")

  • 0
  • 0
  • 20
  • 1
  • 21
Description From Last Updated
mbarden
  1. 
      
  2. usr/src/man/man4/smb.4 (Diff revision 1)
     
     

    I'd like this to be updated to reflect the fact that selecting aes128-gcm enables both aes128-gcm and aes128-ccm, preferring gcm, and selecting aes128-ccm only enables CCM.

  3. You might consider using smb_mbc_put_align(&sr->reply, 8), which makes sure that there's enough space in the MBC (this is what smb uses for other alignment cases)

  4. typo ('reserved')

  5. A short comment (or #define?) indicating where the 64 comes from would be nice

  6. Is there a reason all of this function's callers ignore the return code?

    1. I think it is ok. We can get error here if there is something wrong with crypto stuff. It is kinda internal error. For normal usage it shouldn't be happened. Otherwise we just get wrong hashval and client terminates the session.

  7. usr/src/uts/common/fs/smbsrv/smb3_kdf.c (Diff revision 1)
     
     

    Typo ('Label')

  8. usr/src/uts/common/fs/smbsrv/smb3_kdf.c (Diff revision 1)
     
     

    A comment (or #define) indicating where '89' comes from would be helpful

  9. 
      
gwr
  1. 
      
  2. Does this imply a possible bug in KCF?

    1. Yes, this change is a workaround.

  3. 
      
mbarden
  1. There are a few other 3.1.1 changes that appear to be mandatory that don't seem to be in this changeset.
    
    3.3.4.4 Sending an Error Response
    
            The format for returning an error changed in 3.1.1. Error context is used by smb2_qinfo_sec (and maybe other query_info functions). See: 3.3.5.20.3 for an example. The current one is implemented by smb2sr_put_error_data().
    
            'NULL errors' also changed (i.e. 3.3.5.20.1 returns an error with bytecount set to 8, instead of 0)
    
    3.3.5.4 Receiving an SMB2 NEGOTIATE Request
    	store the entire ClientDialects array on the Connection (smb_session_t) object.
    
    3.3.5.15.12 Handling a Validate Negotiate Info Request
    	If 3.1.1 was negotiated, disconnect.
    	Otherwise, compare the ClientDialects array in the request to the stored array (instead of re-calculating the best dialect and comparing to the negotiated dialect)
        
    
    Then there are a few other 3.1.1 changes that we might not want to implement faithfully, but are important to consider:
    
    3.3.5.7 Receiving an SMB2 TREE_CONNECT Request
    
            "If Connection.Dialect is "3.1.1" and Session.IsAnonymous and Session.IsGuest are set to FALSE and the request is not signed or not encrypted, then the server MUST disconnect the connection."
    
            I'm not sure how strict we want this to be. You might consider enforcing this only if the server (or both client and server?) enables signing. (The spec no longer even conceptualizes a server or client that doesn't support signing, so it's not clear if any 3.1.1 clients are capable of disabling signing)
    
    3.3.5.2.9 Verifying the Session
            The spec says that servers that support 3.1.1 are not supposed to allow 'optional' encryption: either it's required, or its disabled. I suppose that's not something we want to follow, though.
  2. These types are a bit odd. Several members (salt, _resrvd, data) are unused, and they're all only used in this one function. The encrypt_caps ctx is only one member.
    
    You might consider just using locals for these.
  3. Unfortunately, Sessions and Connections are not one-to-one; Even without Multichannel, you can have more than one session per connection (and with multichannel, more than one connection per session). The Connection.preauth_hashval should only contain the negotiate request and response, and then Session.preauth_hashval should be initialized to Connection.preauth_hashval, and then updated with the Session Setup request/responses.

    1. I don't understand what you are talking about, preauth hash value is needed for session setup only. Obviously, there should be 1-to-1 relation between preauth_hashval and session object. How connection is relatated to that?

    2. In our smb server, the 'smb_session_t' object is the SMB2 Connection object, and the 'smb_user_t' object is the SMB2 Session object.

      In the specification for preauthentication integrity checking, the Connection object has its own preauth_hashval; it is the result of hashing the SMB2 Negotiate Request and Response. This value won't change after the Negotiate finishes, but it will be used multiple times - each time a new Session created on this connection. The Session object's each has its own preauth_hashval; it is initialized to Connection->preauth_hashval, but then it is extended using the Session Setup Requests and Responses for that session, which are unique to that particular Session. If a second Session object is created on the Connection (which the specification allows without Multichannel), then the two Sessions will each have a unique preauth_hashval, which is derived from the Connection's (unchanged) preauth_hashval.

      In your implementation, every new Session created on the Connection modifies the Connection's preauth_hashval - this means that every Session after the first has a preauth_hashval derived not from Connection.preauth_hashval, but from the previous Session's preauth_hashval, and so it will derive the wrong keys, and signing will not work for that Session.

  4. usr/src/uts/common/smbsrv/smb_ktypes.h (Diff revision 1)
     
     

    The smb31_preauth_salt member is unused

  5. 
      
jbk
  1. 
      
  2. Probably -- unfortunately, the almost complete lack of documentation on KCF (and the relevant ARC cases never made it out) makes it hard to know which one is wrong (this has caused me no end of frustration whenever I've touched tht code).

    For reading of a crypto_data_t, I think cd_offset is supposed to be the effective offset into the blob of data (which could be a single raw buffer, split across mblk_ts or iovec_ts) and cd_length is the total length of all the data summed across all the buffers the crypto_data_t contains.

    I'm less sure how they should be interpreted for writing -- e.g. should cd_offset be the current write position and cd_length the total length of all the buffers, or should cd_length be the current amount written. Though for either case, I can't point at anything to support my guesses.

  3. 
      
ams
ams
mbarden
  1. 
      
  2. usr/src/uts/common/fs/smbsrv/smb2_dispatch.c (Diff revisions 2 - 3)
     
     

    Because not all errors are 'sticky' in a compound, should these be reset to 0 at some point during compound processing? I imagine we only want to include the error contexts for the particular request/command that created them. Similiarly, I suppose it's possible for multiple requests in a compound to need to use error contexts...

  3. usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revisions 2 - 3)
     
     

    This should not be restricted to 3.1.1; the requirement is for 'servers that implement' 3.1.1, rather than for connections that negotiate 3.1.1. Same for the changes at line 915.

    This function shouldn't ever be called on 3.1.1 - its a protocol error (3.3.5.15.12 "If Connection.Dialect is "3.1.1", the server MUST terminate the transport connection and free the Connection object")

  4. usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revisions 2 - 3)
     
     

    Is there a reason this isn't just a simple compare i.e.

    for (int i = 0 ; i < num_dialects; i++)
        if (dialects[i] != s->cli_dialects[i])
            goto drop;
    

    It's simpler, and has fewer loops.

    I worry that the 'hash' approach could be manipulated to obtain a value of 0, even when the arrays differ. I.e. If the 'real' string is 0x300, 0x302, An attacker could provide the values 0x212, 0x210, and this particular check would 'pass'.

    1. Intention was to make comparison to be dialects order agnostic.
      I will redo it as array comparison.

  5. usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revisions 2 - 3)
     
     

    With the dialect array comparison check (without the >= 3.1.1 check), the 'best dialect' check is no longer needed.

  6. 
      
gwr
  1. 
      
  2. I don't think we're ever supposed to increase max_bytes this way. (Encoding more than max_bytes can lead to panics elsewhere.)
    If we're at the limit of the space allowed for, we should error out.

    1. If I understand correctly an encoding always calls mbc_marchal_make_room() to check available space. If the space is not enough it just returns error EMSGSIZE. So there shouln't be panic. Am I wrong?

      The intention of the function is to add header to error contexts (errdata) and put the result into &session->raw_data. errdata itself can refer to &session->raw_data, so it should be stored in temporary mbuf_chain (errctx_data) before encoding.

      Anyway, what do you think could be correct way to do this?

  3. 
      
ams
mbarden
  1. 
      
  2. In general, we should verify that the offset actually exists within the current request. The way most places do that is by using smb_mbc_decodef(..., "#.", ...) to skip to the listed offset (See http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/smbsrv/smb2_tree_connect.c#51 for an example)

    In cases where we already know the offset is valid (like the above NEG_CTX_INFO_OFFSET, where we've already decoded that area once before), it's not necessary, but if we haven't verified it yet, we should.

  3. smb_mbc_put_align() modifies the buffer (it zero's the skipped data), which we probably don't want to do for the command buffer.

    You could either make a 'read' equivalent of smb_mbc_put_align() that uses smb_mbc_decodef(), or just use smb_mbc_decodef() directly.

    You could calculate ctx_end_off to 'round up' to an 8-byte boundary, then use smb_mbc_decodef(..., "#.", ...) to get your desired alignment, and still make sure that we're within the boundary of the request.

  4. neg_in_ctxs and neg_out_ctxs are no longer valid after this function returns; perhaps you should NULL nego2->neg_{in,out}_ctxs after the send_reply()?
  5. Since the "C" format calls MBC_SETUP() on the passed mbc, if errdata == &sr->raw_data (as it is for all of our calls), then chain_offset will be 0 here.

    I don't believe we'd currently end up with more than one error context, so it's not a problem 'right now'. However, a comment to that effect, and maybe an ASSERT(errdata != &sr->raw_data) before this ASSERT, should help future devs keep that restriction in mind.

  6. If we don't check the return code at call sites, could we log errors in this function? It'd be really nice to know 'right away' that something is going wrong here, instead of having to diagnose it from a vague 'access_denied' or signature error later.

  7. 'mech' needs to be kmem_free()'d here; You might consider just setting rc = -1 and break'ing.

  8. 
      
ams
mbarden
  1. Ship It!
    1. Thank you for all of your work on this!

  2. 
      
ams
gwr
  1. 
      
  2. usr/src/uts/common/fs/smbsrv/smb2_dispatch.c (Diff revisions 4 - 6)
     
     

    Shouldn't this be "wbblllC"?
    (likewise the args...)

    1. Yes, it should be. I will fix. Though, technically the result is the same.

  3. 
      
ams
Review request changed
gwr
  1. Ship It!
    1. Nice! A lot of thanks to everyone who spent his time reviewing this big change.

  2. 
      
Loading...