We have run into a situation where a customer's clients are requesting Session Key Protection via the above flag on a TREE_CONNECT_AND (see 3.2.4.2.5) of [MS-SMB]. This seems to be designed to prevent applications running on the server from divulging the client's actual session keys, but I don't know which registry key/keys are used to enable this. This seems to be related to a change in the behavior of Win-7/W2K08 with respect to the Local Security Policy->Local Policies->Security Options: Microsoft network client: Digitally sign communications (always) If you have it enabled (and have the other one, Digitally Sign communications (if server agrees enabled) then Win-7 and W2K8 does not like it if Samba does not implement Extended Signatures and resets the connection. W2K3 seemed to be happy to go on without the extended signatures.
It would seem that the best way to handle this would be to add a function to libsmb/smb_signing called something like smb_signing_protect_keys that performs the functions required in section 3.3.5.4 of [MS-SMB] under Session Key Protection. They are: ----------------------- If the client has set the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit in the Flags field of the SMB_COM_TREE_CONNECT_ANDX request, then the server MUST hash the session key of the calling user. This protects the key used for signing by making it unavailable to server-side applications. The one-way hash MUST be performed on the user session key by using the HMAC-MD5 algorithm, as specified in [RFC2104]. The steps are as follows: 1. Take the 16-byte user session key from Server.Session.SessionKey. - If this is an LM authentication where the session key is only 8 bytes, then zero extend it to 16 bytes. - If the session key is more than 16 bytes, then use only the first 16 bytes. 2. Calculate the one-way hash as follows: CALL hmac_md5( SSKeyHash, 256, session key, session key length, digest ) SET user session key = digest The resulting 16-byte digest is treated as the user's new session key and returned to the caller who requests it. SSKeyHash is the well-known constant array that is described in section 2.2.2.5. After the session key has been hashed, the server MUST place the hash into Server.Session.SessionKey and set Server.Session.SessionKeyState to Available, which allows applications to query the session key. If the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit is not set, then the Server.Session.SessionKey is not changed and Server.Session.SessionKeyState MUST be set to Available. ----------------------------------- SSKeyHash is defined elsewhere in the document.
The following patch might partially do the trick ... I will attach it and test it after adding some more code: --- libsmb/smb_signing.c.orig 2012-07-31 14:49:24.000000000 -0700 +++ libsmb/smb_signing.c 2012-07-31 15:37:11.000000000 -0700 @@ -45,6 +45,41 @@ struct smb_signing_state { uint32_t seqnum; }; +static unsigned char const SSKeyHash[256] = { + 0x53, 0x65, 0x63, 0x75, 0x72, 0x69, 0x74, 0x79, + 0x20, 0x53, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, + 0x72, 0x65, 0x20, 0x4b, 0x65, 0x79, 0x20, 0x55, + 0x70, 0x67, 0x72, 0x61, 0x64, 0x65, 0x79, 0x07, + 0x6e, 0x28, 0x2e, 0x69, 0x88, 0x10, 0xb3, 0xdb, + 0x01, 0x55, 0x72, 0xfb, 0x74, 0x14, 0xfb, 0xc4, + 0xc5, 0xaf, 0x3b, 0x41, 0x65, 0x32, 0x17, 0xba, + 0xa3, 0x29, 0x08, 0xc1, 0xde, 0x16, 0x61, 0x7e, + 0x66, 0x98, 0xa4, 0x0b, 0xfe, 0x06, 0x83, 0x53, + 0x4d, 0x05, 0xdf, 0x6d, 0xa7, 0x51, 0x10, 0x73, + 0xc5, 0x50, 0xdc, 0x5e, 0xf8, 0x21, 0x46, 0xaa, + 0x96, 0x14, 0x33, 0xd7, 0x52, 0xeb, 0xaf, 0x1f, + 0xbf, 0x36, 0x6c, 0xfc, 0xb7, 0x1d, 0x21, 0x19, + 0x81, 0xd0, 0x6b, 0xfa, 0x77, 0xad, 0xbe, 0x18, + 0x78, 0xcf, 0x10, 0xbd, 0xd8, 0x78, 0xf7, 0xd3, + 0xc6, 0xdf, 0x43, 0x32, 0x19, 0xd3, 0x9b, 0xa8, + 0x4d, 0x9e, 0xaa, 0x41, 0xaf, 0xcb, 0xc6, 0xb9, + 0x34, 0xe7, 0x48, 0x25, 0xd4, 0x88, 0xc4, 0x51, + 0x60, 0x38, 0xd9, 0x62, 0xe8, 0x8d, 0x5b, 0x83, + 0x92, 0x7f, 0xb5, 0x0e, 0x1c, 0x2d, 0x06, 0x91, + 0xc3, 0x75, 0xb3, 0xcc, 0xf8, 0xf7, 0x92, 0x91, + 0x0b, 0x3d, 0xa1, 0x10, 0x5b, 0xd5, 0x0f, 0xa8, + 0x3f, 0x5d, 0x13, 0x83, 0x0a, 0x6b, 0x72, 0x93, + 0x14, 0x59, 0xd5, 0xab, 0xde, 0x26, 0x15, 0x6d, + 0x60, 0x67, 0x71, 0x06, 0x6e, 0x3d, 0x0d, 0xa7, + 0xcb, 0x70, 0xe9, 0x08, 0x5c, 0x99, 0xfa, 0x0a, + 0x5f, 0x3d, 0x44, 0xa3, 0x8b, 0xc0, 0x8d, 0xda, + 0xe2, 0x68, 0xd0, 0x0d, 0xcd, 0x7f, 0x3d, 0xf8, + 0x73, 0x7e, 0x35, 0x7f, 0x07, 0x02, 0x0a, 0xb5, + 0xe9, 0xb7, 0x87, 0xfb, 0xa1, 0xbf, 0xcb, 0x32, + 0x31, 0x66, 0x09, 0x48, 0x88, 0xcc, 0x18, 0xa3, + 0xb2, 0x1f, 0x1f, 0x1b, 0x90, 0x4e, 0xd7, 0xe1 +}; + static void smb_signing_reset_info(struct smb_signing_state *si) { si->active = false; @@ -74,6 +109,42 @@ struct smb_signing_state *smb_signing_in return si; } +static bool smb_signing_protect_key(struct smb_signing_state *si) +{ + unsigned char digest[64]; + + /* If signing is not active, then this is wrong */ + if (!si->active) { + return false; + } + + /* The key cannot be more than 16 bytes in length */ + if (si->mac_key.length > 16) { + si->mac_key.length = 16; + } + + /* If the key is 8 bytes, zero fill to 16 */ + if (si->mac_key.length == 8) { + int i = 0; + + if (!data_blob_realloc(si, &si->mac_key, 16)) { + return false; + } + + for (i = 8; i < 16; i++) + si->mac_key.data[i] = 0; + + si->mac_key.length = 16; + } + + hmac_md5(si->mac_key.data, SSKeyHash, sizeof(SSKeyHash), digest); + + /* Replace the session key with the digest */ + memcpy(si->mac_key.data, digest, 16); + + return true; +} + static bool smb_signing_good(struct smb_signing_state *si, bool good, uint32_t seq) {
Created attachment 7732 [details] The first patch that might fix this ... prepare the new hash based on [MS-SMB2] This patch implements a function to calculate the new signing key and update it. Next up a small change in reply_tcon_andx or whatever is needed to call this function. Probably need some DEBUG calls in error paths as well.
OK, the master branch is lacking in this functionality as well. I will work up my patches against master.
(In reply to comment #2) > The following patch might partially do the trick ... I will attach it and test > it after adding some more code: > > --- libsmb/smb_signing.c.orig 2012-07-31 14:49:24.000000000 -0700 > +++ libsmb/smb_signing.c 2012-07-31 15:37:11.000000000 -0700 > @@ -45,6 +45,41 @@ struct smb_signing_state { > uint32_t seqnum; > }; > > +static unsigned char const SSKeyHash[256] = { > + 0x53, 0x65, 0x63, 0x75, 0x72, 0x69, 0x74, 0x79, > + 0x20, 0x53, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, > + 0x72, 0x65, 0x20, 0x4b, 0x65, 0x79, 0x20, 0x55, > + 0x70, 0x67, 0x72, 0x61, 0x64, 0x65, 0x79, 0x07, > + 0x6e, 0x28, 0x2e, 0x69, 0x88, 0x10, 0xb3, 0xdb, > + 0x01, 0x55, 0x72, 0xfb, 0x74, 0x14, 0xfb, 0xc4, > + 0xc5, 0xaf, 0x3b, 0x41, 0x65, 0x32, 0x17, 0xba, > + 0xa3, 0x29, 0x08, 0xc1, 0xde, 0x16, 0x61, 0x7e, > + 0x66, 0x98, 0xa4, 0x0b, 0xfe, 0x06, 0x83, 0x53, > + 0x4d, 0x05, 0xdf, 0x6d, 0xa7, 0x51, 0x10, 0x73, > + 0xc5, 0x50, 0xdc, 0x5e, 0xf8, 0x21, 0x46, 0xaa, > + 0x96, 0x14, 0x33, 0xd7, 0x52, 0xeb, 0xaf, 0x1f, > + 0xbf, 0x36, 0x6c, 0xfc, 0xb7, 0x1d, 0x21, 0x19, > + 0x81, 0xd0, 0x6b, 0xfa, 0x77, 0xad, 0xbe, 0x18, > + 0x78, 0xcf, 0x10, 0xbd, 0xd8, 0x78, 0xf7, 0xd3, > + 0xc6, 0xdf, 0x43, 0x32, 0x19, 0xd3, 0x9b, 0xa8, > + 0x4d, 0x9e, 0xaa, 0x41, 0xaf, 0xcb, 0xc6, 0xb9, > + 0x34, 0xe7, 0x48, 0x25, 0xd4, 0x88, 0xc4, 0x51, > + 0x60, 0x38, 0xd9, 0x62, 0xe8, 0x8d, 0x5b, 0x83, > + 0x92, 0x7f, 0xb5, 0x0e, 0x1c, 0x2d, 0x06, 0x91, > + 0xc3, 0x75, 0xb3, 0xcc, 0xf8, 0xf7, 0x92, 0x91, > + 0x0b, 0x3d, 0xa1, 0x10, 0x5b, 0xd5, 0x0f, 0xa8, > + 0x3f, 0x5d, 0x13, 0x83, 0x0a, 0x6b, 0x72, 0x93, > + 0x14, 0x59, 0xd5, 0xab, 0xde, 0x26, 0x15, 0x6d, > + 0x60, 0x67, 0x71, 0x06, 0x6e, 0x3d, 0x0d, 0xa7, > + 0xcb, 0x70, 0xe9, 0x08, 0x5c, 0x99, 0xfa, 0x0a, > + 0x5f, 0x3d, 0x44, 0xa3, 0x8b, 0xc0, 0x8d, 0xda, > + 0xe2, 0x68, 0xd0, 0x0d, 0xcd, 0x7f, 0x3d, 0xf8, > + 0x73, 0x7e, 0x35, 0x7f, 0x07, 0x02, 0x0a, 0xb5, > + 0xe9, 0xb7, 0x87, 0xfb, 0xa1, 0xbf, 0xcb, 0x32, > + 0x31, 0x66, 0x09, 0x48, 0x88, 0xcc, 0x18, 0xa3, > + 0xb2, 0x1f, 0x1f, 0x1b, 0x90, 0x4e, 0xd7, 0xe1 > +}; > + > static void smb_signing_reset_info(struct smb_signing_state *si) > { > si->active = false; > @@ -74,6 +109,42 @@ struct smb_signing_state *smb_signing_in > return si; > } > > +static bool smb_signing_protect_key(struct smb_signing_state *si) > +{ > + unsigned char digest[64]; > + > + /* If signing is not active, then this is wrong */ > + if (!si->active) { > + return false; > + } > + > + /* The key cannot be more than 16 bytes in length */ > + if (si->mac_key.length > 16) { > + si->mac_key.length = 16; > + } > + > + /* If the key is 8 bytes, zero fill to 16 */ > + if (si->mac_key.length == 8) { > + int i = 0; > + > + if (!data_blob_realloc(si, &si->mac_key, 16)) { > + return false; > + } > + > + for (i = 8; i < 16; i++) > + si->mac_key.data[i] = 0; > + > + si->mac_key.length = 16; > + } > + > + hmac_md5(si->mac_key.data, SSKeyHash, sizeof(SSKeyHash), digest); > + > + /* Replace the session key with the digest */ > + memcpy(si->mac_key.data, digest, 16); > + > + return true; > +} > + > static bool smb_signing_good(struct smb_signing_state *si, > bool good, uint32_t seq) > { I'm not sure this is correct. I read it as "the signing key is protected from the application" not "the signing key is changed". This means the change applies to the session key which is exposed to the named-pipe/RPC-layer. See https://gitweb.samba.org/?p=samba.git;a=blob;f=source3/smbd/smb2_sesssetup.c;hb=d4bce355a#l235 For the SMB2 example, it's the session_info->session_key which needs to be changed.
I have prepared our client support and added a TODO marker in the server code in this branch https://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-signing This needs testing of the client against Windows and a completed server implementation.
Created attachment 7733 [details] draft patches for master The commits without TODO are already in autobuild...
Sure, I was not sure what it was referring to, and I did not notice that the User Session Key was being protected. It seems that there are only three references, or so, to User Session Key and it is not explained. With reference to this: + if (session->smb1.application_key.length != 16) { + return NT_STATUS_INVALID_PARAMETER_MIX; + } This does not conform to the spec, which says that if the session is only 8 bytes it should be extended to 16 with zeros (only in the LM authentication case, I guess) and if it is greater than 16 bytes it should be truncated.
(In reply to comment #8) > Sure, I was not sure what it was referring to, and I did not notice that the > User Session Key was being protected. > > It seems that there are only three references, or so, to User Session Key and > it is not explained. > > With reference to this: > > + if (session->smb1.application_key.length != 16) { > + return NT_STATUS_INVALID_PARAMETER_MIX; > + } > > This does not conform to the spec, which says that if the session is only 8 > bytes it should be extended to 16 with zeros (only in the LM authentication > case, I guess) and if it is greater than 16 bytes it should be truncated. At that stage we have already expanded it to 16 bytes. See uint8_t session_key[16]; in smb1cli_session_set_session_key() But see my master3-signing branch for the latest code, I've fixed some minor bugs to make "make test" happy.
Should be fixed in master with commit 401860cab6ab3d88659361bd333f6667da071d7b, Richard can you please test current master?
(In reply to comment #10) > Should be fixed in master with commit 401860cab6ab3d88659361bd333f6667da071d7b, > Richard can you please test current master? OK, I will have a look at this next week. The simplest way to test it might be to back-port the change to 3.5.15 ...
(In reply to comment #10) > Should be fixed in master with commit 401860cab6ab3d88659361bd333f6667da071d7b, > Richard can you please test current master? Hmmm, it looks like there are about a dozen commits related to this ...
(In reply to comment #11) > (In reply to comment #10) > > Should be fixed in master with commit 401860cab6ab3d88659361bd333f6667da071d7b, > > Richard can you please test current master? > > OK, I will have a look at this next week. The simplest way to test it might be > to back-port the change to 3.5.15 ... OK, so I have tested this, and it seems that Samba no longer drops the connection, however, it is also not setting the bit in the response saying that extended signatures are enabled. This appears be be because in sourc3/smbd/reply.c:reply_tcon_and_X at around line 854 we have: if (session->global->application_key.length == 0 && session->global->signing_key.length > 0) However, in source3/smbd/sesssetup.c:reply_sesssetup_and_X at about line 959 we have: session->global->application_key = data_blob_talloc(session->global, session_key, sizeof(session_key)); So, the test above for application_key.length == 0 looks wrong. The application key will exist and be equal to the signing key. I think we need to defer setting the application key, probably (in which case it is the same as the signing key.)
Following up on my last comment, since [MS-SMB] 3.3.4.2 says that if Server.Session.SessionKeyState is Unavailable a request by an application for a User Session Key must be denied (STATUS_ACCESS_DENIED), and SessionKeyState is set to available only during TREE_CONNECT_ANDX processing and is set regardless of the state of the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit, This suggests that for SMB1 SESSION_SETUP_ANDX requests we should not set the Application Key, but rather we should set it during TREE_CONNECT_ANDX processing, and should set it based on the value of the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit and then, if TREE_CONNECT_ANDX_EXTENDED_SIGNATURE is set, or that into optional.
(In reply to comment #14) > Following up on my last comment, since [MS-SMB] 3.3.4.2 says that if > Server.Session.SessionKeyState is Unavailable a request by an application for a > User Session Key must be denied (STATUS_ACCESS_DENIED), and SessionKeyState is > set to available only during TREE_CONNECT_ANDX processing and is set regardless > of the state of the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit, > > This suggests that for SMB1 SESSION_SETUP_ANDX requests we should not set the > Application Key, but rather we should set it during TREE_CONNECT_ANDX > processing, and should set it based on the value of the > TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit and then, if > TREE_CONNECT_ANDX_EXTENDED_SIGNATURE is set, or that into optional. That's what we do for spnego session setups see line 306 in sesssetup.c, then only the first TCONX sets the application key and may return the SMB_EXTENDED_SIGNATURES bit. If tested that against windows and any following TCONX does not touch the application key and don't set the SMB_EXTENDED_SIGNATURES bit.
(In reply to comment #14) > Following up on my last comment, since [MS-SMB] 3.3.4.2 says that if > Server.Session.SessionKeyState is Unavailable a request by an application for a > User Session Key must be denied (STATUS_ACCESS_DENIED), and SessionKeyState is > set to available only during TREE_CONNECT_ANDX processing and is set regardless > of the state of the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit, The docs might be a bit unclear, but for the client is says in 3.2.5.3 Receiving an SMB_COM_SESSION_SETUP_ANDX Response: ... NTLM Authentication If the CAP_EXTENDED_SECURITY bit in Client.Connection.ServerCapabilities is not set ... ... The client MUST set Client.Session.SessionKeyState to Available. I mean we could try against windows really happens without a spnego session setup.
(In reply to comment #16) > (In reply to comment #14) > > Following up on my last comment, since [MS-SMB] 3.3.4.2 says that if > > Server.Session.SessionKeyState is Unavailable a request by an application for a > > User Session Key must be denied (STATUS_ACCESS_DENIED), and SessionKeyState is > > set to available only during TREE_CONNECT_ANDX processing and is set regardless > > of the state of the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit, > > The docs might be a bit unclear, but for the client is says in > 3.2.5.3 Receiving an SMB_COM_SESSION_SETUP_ANDX Response: > ... > NTLM Authentication > If the CAP_EXTENDED_SECURITY bit in Client.Connection.ServerCapabilities is not > set ... > ... The client MUST set Client.Session.SessionKeyState to Available. > > I mean we could try against windows really happens without a spnego session > setup. You are correct. I need to check WWWD as well. Also, I have not verified that before your changes things worked with my current setup. I will attempt to do this check today. As for switching off SPNEGO on Windows, if I have understood what you are saying, I am not sure I know how to do that.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > Following up on my last comment, since [MS-SMB] 3.3.4.2 says that if > > > Server.Session.SessionKeyState is Unavailable a request by an application for a > > > User Session Key must be denied (STATUS_ACCESS_DENIED), and SessionKeyState is > > > set to available only during TREE_CONNECT_ANDX processing and is set regardless > > > of the state of the TREE_CONNECT_ANDX_EXTENDED_SIGNATURE bit, > > > > The docs might be a bit unclear, but for the client is says in > > 3.2.5.3 Receiving an SMB_COM_SESSION_SETUP_ANDX Response: > > ... > > NTLM Authentication > > If the CAP_EXTENDED_SECURITY bit in Client.Connection.ServerCapabilities is not > > set ... > > ... The client MUST set Client.Session.SessionKeyState to Available. > > > > I mean we could try against windows really happens without a spnego session > > setup. > > You are correct. I need to check WWWD as well. > > Also, I have not verified that before your changes things worked with my > current setup. I will attempt to do this check today. > > As for switching off SPNEGO on Windows, if I have understood what you are > saying, I am not sure I know how to do that. Play with "use spnego = no" for smbd and "client use spnego = no" for "net". Against Windows I've tested net rpc user add protected A1b2C3d4P -Uadministrator%A1b2C3d4 -W BLA -S w2k8r2-219.bla.base net rpc user delete protected -Uadministrator%A1b2C3d4 -W BLA -S w2k8r2-219.bla.base compared to net rpc user add protected A1b2C3d4P -Uadministrator%A1b2C3d4 -W BLA -S w2k8r2-219.bla.base --option="client use spnego = no" --option="client ntlmv2 auth = no" net rpc user delete protected -Uadministrator%A1b2C3d4 -W BLA -S w2k8r2-219.bla.base --option="client use spnego = no" --option="client ntlmv2 auth = no"
Created attachment 7747 [details] Capture of net rpc * This is a capture of the 4 commands in comment 18.
We now have another OEM hitting this in a production site. We must get this fixed for 3.6.next (IMHO). Jeremy.
OK, I was wrong in my bug report. The problem I encountered came down to not setting "server signing = auto" I actually do not know how to provoke the problem that I originally opened this bug for. Can anyone describe how to actually provoke this problem?
If it can be fixed by "server signing = auto", we should think about changing the default to auto/if_required. In master in srv_init_signing() handle SMB_SIGNING_DEFAULT as SMB_SIGNING_IF_REQUIRED instead of SMB_SIGNING_OFF.
Ok, I've done some testing from a rpcclient against a Win7 server from master, and a modified 3.6.next, using SMB signed connections and SIGNED+SEALED rpc requests from rpcclient and always setting the TCONX_FLAG_EXTENDED_SIGNATURES bit. Essentially this is a no-op for the server, as neither the SMB signing code or the RPC signing/sealing code use the modified application key. So I think setting server signing = auto will fix it, and we can just add a fix inside the reply_tconx code in 3.6.x that just reflects back the TCONX_FLAG_EXTENDED_SIGNATURES bit if sent. Jeremy.
Created attachment 7764 [details] Proposed patch for 3.6.next ? Metze and Richard, what do you think of this for 3.6.next ? It's based on the code in master. Jeremy.
(In reply to comment #24) > Created attachment 7764 [details] > Proposed patch for 3.6.next ? > > Metze and Richard, what do you think of this for 3.6.next ? It's based on the > code in master. > > Jeremy. Thumbs up from me.
Comment on attachment 7764 [details] Proposed patch for 3.6.next ? Why do you want to say the session key is protected while it's not? We should only backport the EXTENDED_SIGNATURE code if it's really needed. And from Richards latest comments I think it's not needed.
(In reply to comment #23) > Ok, I've done some testing from a rpcclient against a Win7 server from master, > and a modified 3.6.next, using SMB signed connections and SIGNED+SEALED rpc > requests from rpcclient and always setting the TCONX_FLAG_EXTENDED_SIGNATURES > bit. > > Essentially this is a no-op for the server, as neither the SMB signing code or > the RPC signing/sealing code use the modified application key. If you use RPC level authentication, the inherited application key from SMB layer is not used. Also this is called application key, because it's used at the application layer, not at the SMB signing or RPC transport layer. The application key is used to encrypt the password fields in some SAMR/LSA calls. See the net rpc commands in comment 18, they demonstrate that it's not a no-op, if you just change one byte in SSKeyHash, you'll get NT_STATUS_WRONG_PASSWORD (or something similar) while setting the password. > So I think setting server signing = auto will fix it, and we can just add a fix > inside the reply_tconx code in 3.6.x that just reflects back the > TCONX_FLAG_EXTENDED_SIGNATURES bit if sent. If "server signing = auto" doesn't fix it without modifications, we need to backport the whole logic.
As "server signing = auto" should fix it, I'm marking it as invalid