Attachment 430 Details for Bug 1138 – Fix many smb signing issues with Samba

[patch] Fix many smb signing issues with Samba

sign.patch (text/plain), 14.69 KB, created by Andrew Bartlett on 2004-03-09 05:40:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2004-03-09 05:40:49 UTC

Size: 14.69 KB

patch

obsolete

>Author: abartlet
>Mofified: source/include/smb.h source/lib/util_sock.c source/libsmb/cliconnect.c source/libsmb/clientgen.c source/libsmb/smb_signing.c source/smbd/password.c source/smbd/sesssetup.c
>Added:   
>Removed:   
>
>
>Commit to HEAD the updates to smb signing code that I was propsing for 3.0.
>
>This code implements 'opportunistic signing' in our client (when the
>server supports it, we will use it), and correct downgrading on both
>the client and server for the 'enabled' (rather than required) signing
>level.
>
>This means that we can actually set 'server signing = yes' and not
>have the world fall apart.  We had a number of bugs in that code, and
>it certainly looks like most of the testing was with the 'requried'
>setting.
>
>While the changes are reasonable, I'm putting this into HEAD rather
>than 3.0 for the timebeing.  SMB signing, like NTLMSSP, tends to have
>gotchas in it :-)
>
>(I also need to give it a workout with more than smbclient before I
>move it across).
>
>Andrew Bartlett
>
>
>Index: source/libsmb/smb_signing.c
>===================================================================
>RCS file: /cvsroot/samba/source/libsmb/smb_signing.c,v
>retrieving revision 1.20
>retrieving revision 1.21
>diff -u -r1.20 -r1.21
>--- source/libsmb/smb_signing.c	17 Jan 2004 00:30:28 -0000	1.20
>+++ source/libsmb/smb_signing.c	9 Mar 2004 12:37:05 -0000	1.21
>@@ -150,7 +150,7 @@
>  SMB signing - NULL implementation - check a MAC sent by server.
> ************************************************************/
> 
>-static BOOL null_check_incoming_message(char *inbuf, struct smb_sign_info *si)
>+static BOOL null_check_incoming_message(char *inbuf, struct smb_sign_info *si, BOOL expected_ok)
> {
> 	return True;
> }
>@@ -197,25 +197,39 @@
> }
> 
> 
>-static BOOL signing_good(char *inbuf, struct smb_sign_info *si, BOOL good, uint32 seq) 
>+static BOOL signing_good(char *inbuf, struct smb_sign_info *si, BOOL good, uint32 seq, BOOL expected_ok) 
> {
>-	if (good && !si->doing_signing) {
>-		si->doing_signing = True;
>-	}
>+	if (good) {
> 
>-	if (!good) {
>-		if (si->doing_signing) {
>-			struct smb_basic_signing_context *data = si->signing_context;
>+		if (!si->doing_signing) {
>+			si->doing_signing = True;
>+		}
>+		
>+		if (!si->seen_valid) {
>+			si->seen_valid = True;
>+		}
> 
>-			/* W2K sends a bad first signature but the sign engine is on.... JRA. */
>-			if (data->send_seq_num > 1)
>-				DEBUG(1, ("signing_good: SMB signature check failed on seq %u!\n",
>-							(unsigned int)seq ));
>+	} else {
>+		if (!si->mandatory_signing && !si->seen_valid) {
> 
>-			return False;
>-		} else {
>-			DEBUG(3, ("signing_good: Peer did not sign reply correctly\n"));
>+			if (!expected_ok) {
>+				return True;
>+			}
>+			/* Non-mandatory signing - just turn off if this is the first bad packet.. */
>+			DEBUG(5, ("signing_good: signing negotiated but not required and the other side \
>+isn't sending correct signatures. Turning signatures off.\n"));
>+			si->negotiated_smb_signing = False;
>+			si->allow_smb_signing = False;
>+			si->doing_signing = False;
> 			free_signing_context(si);
>+			return True;
>+		} else if (!expected_ok) {
>+			/* This packet is known to be unsigned */
>+			return True;
>+		} else {
>+			/* Mandatory signing or bad packet after signing started - fail and disconnect. */
>+			if (seq)
>+				DEBUG(0, ("signing_good: BAD SIG: seq %u\n", (unsigned int)seq));
> 			return False;
> 		}
> 	}
>@@ -323,7 +337,7 @@
>  SMB signing - Client implementation - check a MAC sent by server.
> ************************************************************/
> 
>-static BOOL client_check_incoming_message(char *inbuf, struct smb_sign_info *si)
>+static BOOL client_check_incoming_message(char *inbuf, struct smb_sign_info *si, BOOL expected_ok)
> {
> 	BOOL good;
> 	uint32 reply_seq_number;
>@@ -381,7 +395,7 @@
> 		DEBUG(10, ("client_check_incoming_message: seq %u: got good SMB signature of\n", (unsigned int)reply_seq_number));
> 		dump_data(10, (const char *)server_sent_mac, 8);
> 	}
>-	return signing_good(inbuf, si, good, saved_seq);
>+	return signing_good(inbuf, si, good, saved_seq, expected_ok);
> }
> 
> /***********************************************************
>@@ -415,7 +429,7 @@
> 
> BOOL cli_simple_set_signing(struct cli_state *cli,
> 			    const DATA_BLOB user_session_key,
>-			    const DATA_BLOB response, int initial_send_seq_num)
>+			    const DATA_BLOB response)
> {
> 	struct smb_basic_signing_context *data;
> 
>@@ -453,7 +467,7 @@
> 	dump_data_pw("MAC ssession key is:\n", data->mac_key.data, data->mac_key.length);
> 
> 	/* Initialise the sequence number */
>-	data->send_seq_num = initial_send_seq_num;
>+	data->send_seq_num = 0;
> 
> 	/* Initialise the list of outstanding packets */
> 	data->outstanding_packet_list = NULL;
>@@ -535,7 +549,7 @@
>  SMB signing - TEMP implementation - check a MAC sent by server.
> ************************************************************/
> 
>-static BOOL temp_check_incoming_message(char *inbuf, struct smb_sign_info *si)
>+static BOOL temp_check_incoming_message(char *inbuf, struct smb_sign_info *si, BOOL expected_ok)
> {
> 	return True;
> }
>@@ -597,9 +611,9 @@
>  *         which had a bad checksum, True otherwise.
>  */
>  
>-BOOL cli_check_sign_mac(struct cli_state *cli) 
>+BOOL cli_check_sign_mac(struct cli_state *cli, BOOL expected_ok) 
> {
>-	if (!cli->sign_info.check_incoming_message(cli->inbuf, &cli->sign_info)) {
>+	if (!cli->sign_info.check_incoming_message(cli->inbuf, &cli->sign_info, expected_ok)) {
> 		free_signing_context(&cli->sign_info);	
> 		return False;
> 	}
>@@ -688,7 +702,7 @@
>  SMB signing - Server implementation - check a MAC sent by server.
> ************************************************************/
> 
>-static BOOL srv_check_incoming_message(char *inbuf, struct smb_sign_info *si)
>+static BOOL srv_check_incoming_message(char *inbuf, struct smb_sign_info *si, BOOL expected_ok)
> {
> 	BOOL good;
> 	struct smb_basic_signing_context *data = si->signing_context;
>@@ -762,25 +776,7 @@
> 		dump_data(10, (const char *)server_sent_mac, 8);
> 	}
> 
>-	if (!signing_good(inbuf, si, good, saved_seq)) {
>-		if (!si->mandatory_signing && (data->send_seq_num < 3)){
>-			/* Non-mandatory signing - just turn off if this is the first bad packet.. */
>-			DEBUG(5, ("srv_check_incoming_message: signing negotiated but not required and client \
>-isn't sending correct signatures. Turning off.\n"));
>-			si->negotiated_smb_signing = False;
>-			si->allow_smb_signing = False;
>-			si->doing_signing = False;
>-			free_signing_context(si);
>-			return True;
>-		} else {
>-			/* Mandatory signing or bad packet after signing started - fail and disconnect. */
>-			if (saved_seq)
>-				DEBUG(0, ("srv_check_incoming_message: BAD SIG: seq %u\n", (unsigned int)saved_seq));
>-			return False;
>-		}
>-	} else {
>-		return True;
>-	}
>+	return (signing_good(inbuf, si, good, saved_seq, expected_ok));
> }
> 
> /***********************************************************
>@@ -813,13 +809,13 @@
>  Called to validate an incoming packet from the client.
> ************************************************************/
> 
>-BOOL srv_check_sign_mac(char *inbuf)
>+BOOL srv_check_sign_mac(char *inbuf, BOOL expected_ok)
> {
> 	/* Check if it's a session keepalive. */
> 	if(CVAL(inbuf,0) == SMBkeepalive)
> 		return True;
> 
>-	return srv_sign_info.check_incoming_message(inbuf, &srv_sign_info);
>+	return srv_sign_info.check_incoming_message(inbuf, &srv_sign_info, expected_ok);
> }
> 
> /***********************************************************
>@@ -906,6 +902,42 @@
> {
> 	return srv_sign_info.doing_signing;
> }
>+
>+
>+/***********************************************************
>+ Returns whether signing is negotiated. We can't use it unless it was
>+ in the negprot.  
>+************************************************************/
>+
>+BOOL srv_is_signing_negotiated(void)
>+{
>+	return srv_sign_info.negotiated_smb_signing;
>+}
>+
>+/***********************************************************
>+ Returns whether signing is negotiated. We can't use it unless it was
>+ in the negprot.  
>+************************************************************/
>+
>+BOOL srv_signing_started(void)
>+{
>+	struct smb_basic_signing_context *data;
>+
>+	if (!srv_sign_info.doing_signing) {
>+		return False;
>+	}
>+
>+	data = (struct smb_basic_signing_context *)srv_sign_info.signing_context;
>+	if (!data)
>+		return False;
>+
>+	if (data->send_seq_num == 0) {
>+		return False;
>+	}
>+
>+	return True;
>+}
>+
> 
> /***********************************************************
>  Tell server code we are in a multiple trans reply state.
>
>Index: source/lib/util_sock.c
>===================================================================
>RCS file: /cvsroot/samba/source/lib/util_sock.c,v
>retrieving revision 1.89
>retrieving revision 1.90
>diff -u -r1.89 -r1.90
>--- source/lib/util_sock.c	20 Feb 2004 22:42:39 -0000	1.89
>+++ source/lib/util_sock.c	9 Mar 2004 12:37:05 -0000	1.90
>@@ -596,7 +596,7 @@
> 	}
> 
> 	/* Check the incoming SMB signature. */
>-	if (!srv_check_sign_mac(buffer)) {
>+	if (!srv_check_sign_mac(buffer, True)) {
> 		DEBUG(0, ("receive_smb: SMB Signature verification failed on incoming packet!\n"));
> 		if (smb_read_error == 0)
> 			smb_read_error = READ_BAD_SIG;
>
>Index: source/smbd/sesssetup.c
>===================================================================
>RCS file: /cvsroot/samba/source/smbd/sesssetup.c,v
>retrieving revision 1.118
>retrieving revision 1.119
>diff -u -r1.118 -r1.119
>--- source/smbd/sesssetup.c	26 Jan 2004 02:22:49 -0000	1.118
>+++ source/smbd/sesssetup.c	9 Mar 2004 12:37:05 -0000	1.119
>@@ -287,14 +287,14 @@
> 		
> 		SSVAL(outbuf, smb_uid, sess_vuid);
> 
>-		if (!server_info->guest) {
>+		if (!server_info->guest && !srv_signing_started()) {
> 			/* We need to start the signing engine
> 			 * here but a W2K client sends the old
> 			 * "BSRSPYL " signature instead of the
> 			 * correct one. Subsequent packets will
> 			 * be correct.
> 			 */
>-		       	srv_check_sign_mac(inbuf);
>+		       	srv_check_sign_mac(inbuf, False);
> 		}
> 	}
> 
>@@ -360,14 +360,15 @@
> 			
> 			SSVAL(outbuf,smb_uid,sess_vuid);
> 
>-			if (!server_info->guest) {
>+			if (!server_info->guest && !srv_signing_started()) {
> 				/* We need to start the signing engine
> 				 * here but a W2K client sends the old
> 				 * "BSRSPYL " signature instead of the
> 				 * correct one. Subsequent packets will
> 				 * be correct.
> 				 */
>-			       	srv_check_sign_mac(inbuf);
>+
>+				srv_check_sign_mac(inbuf, False);
> 			}
> 		}
> 	}
>@@ -907,7 +908,7 @@
> 		return ERROR_NT(NT_STATUS_LOGON_FAILURE);
> 	}
> 
>- 	if (!server_info->guest && !srv_check_sign_mac(inbuf)) {
>+ 	if (!server_info->guest && !srv_signing_started() && !srv_check_sign_mac(inbuf, True)) {
> 		exit_server("reply_sesssetup_and_X: bad smb signature");
> 	}
> 
>
>Index: source/include/smb.h
>===================================================================
>RCS file: /cvsroot/samba/source/include/smb.h,v
>retrieving revision 1.504
>retrieving revision 1.505
>diff -u -r1.504 -r1.505
>--- source/include/smb.h	3 Mar 2004 20:55:40 -0000	1.504
>+++ source/include/smb.h	9 Mar 2004 12:37:05 -0000	1.505
>@@ -1650,7 +1650,7 @@
> 
> typedef struct smb_sign_info {
> 	void (*sign_outgoing_message)(char *outbuf, struct smb_sign_info *si);
>-	BOOL (*check_incoming_message)(char *inbuf, struct smb_sign_info *si);
>+	BOOL (*check_incoming_message)(char *inbuf, struct smb_sign_info *si, BOOL expected_ok);
> 	void (*free_signing_context)(struct smb_sign_info *si);
> 	void *signing_context;
> 
>@@ -1658,6 +1658,7 @@
> 	BOOL allow_smb_signing;
> 	BOOL doing_signing;
> 	BOOL mandatory_signing;
>+	BOOL seen_valid; /* Have I ever seen a validly signed packet? */
> } smb_sign_info;
> 
> #endif /* _SMB_H */
>
>Index: source/libsmb/clientgen.c
>===================================================================
>RCS file: /cvsroot/samba/source/libsmb/clientgen.c,v
>retrieving revision 1.226
>retrieving revision 1.227
>diff -u -r1.226 -r1.227
>--- source/libsmb/clientgen.c	22 Nov 2003 13:29:01 -0000	1.226
>+++ source/libsmb/clientgen.c	9 Mar 2004 12:37:05 -0000	1.227
>@@ -117,7 +117,7 @@
> 		return ret;
> 	}
> 
>-	if (!cli_check_sign_mac(cli)) {
>+	if (!cli_check_sign_mac(cli, True)) {
> 		DEBUG(0, ("SMB Signature verification failed on incoming packet!\n"));
> 		cli->smb_rw_error = READ_BAD_SIG;
> 		close(cli->fd);
>
>Index: source/smbd/password.c
>===================================================================
>RCS file: /cvsroot/samba/source/smbd/password.c,v
>retrieving revision 1.278
>retrieving revision 1.279
>diff -u -r1.278 -r1.279
>--- source/smbd/password.c	1 Mar 2004 16:10:28 -0000	1.278
>+++ source/smbd/password.c	9 Mar 2004 12:37:05 -0000	1.279
>@@ -275,7 +275,7 @@
> 		vuser->homes_snum = -1;
> 	}
> 	
>-	if (lp_server_signing() && !vuser->guest && !srv_is_signing_active()) {
>+	if (srv_is_signing_negotiated() && !vuser->guest && !srv_signing_started()) {
> 		/* Try and turn on server signing on the first non-guest sessionsetup. */
> 		srv_set_signing(vuser->session_key, response_blob);
> 	}
>
>Index: source/libsmb/cliconnect.c
>===================================================================
>RCS file: /cvsroot/samba/source/libsmb/cliconnect.c,v
>retrieving revision 1.147
>retrieving revision 1.148
>diff -u -r1.147 -r1.148
>--- source/libsmb/cliconnect.c	15 Jan 2004 19:08:45 -0000	1.147
>+++ source/libsmb/cliconnect.c	9 Mar 2004 12:37:05 -0000	1.148
>@@ -325,7 +325,7 @@
> 			session_key = data_blob(NULL, 16);
> 			SMBsesskeygen_ntv1(nt_hash, NULL, session_key.data);
> 		}
>-		cli_simple_set_signing(cli, session_key, nt_response, 0); 
>+		cli_simple_set_signing(cli, session_key, nt_response); 
> 	} else {
> 		/* pre-encrypted password supplied.  Only used for 
> 		   security=server, can't do
>@@ -521,7 +521,7 @@
> 	file_save("negTokenTarg.dat", negTokenTarg.data, negTokenTarg.length);
> #endif
> 
>-	cli_simple_set_signing(cli, session_key_krb5, null_blob, 0); 
>+	cli_simple_set_signing(cli, session_key_krb5, null_blob); 
> 			
> 	blob2 = cli_session_setup_blob(cli, negTokenTarg);
> 
>@@ -643,13 +643,16 @@
> 		fstrcpy(cli->server_domain, ntlmssp_state->server_domain);
> 		cli_set_session_key(cli, ntlmssp_state->session_key);
> 
>-		/* Using NTLMSSP session setup, signing on the net only starts
>-		 * after a successful authentication and the session key has
>-		 * been determined, but with a sequence number of 2. This
>-		 * assumes that NTLMSSP needs exactly 2 roundtrips, for any
>-		 * other SPNEGO mechanism it needs adapting. */
>-
>-		cli_simple_set_signing(cli, key, null_blob, 2);
>+		if (cli_simple_set_signing(cli, key, null_blob)) {
>+			
>+			/* 'resign' the last message, so we get the right sequence numbers
>+			   for checking the first reply from the server */
>+			cli_calculate_sign_mac(cli);
>+			
>+			if (!cli_check_sign_mac(cli, True)) {
>+				nt_status = NT_STATUS_ACCESS_DENIED;
>+			}
>+		}
> 	}
> 
> 	/* we have a reference conter on ntlmssp_state, if we are signing
>@@ -1088,6 +1091,8 @@
> 			}
> 			cli->sign_info.negotiated_smb_signing = True;
> 			cli->sign_info.mandatory_signing = True;
>+		} else if (cli->sign_info.allow_smb_signing && cli->sec_mode & NEGOTIATE_SECURITY_SIGNATURES_ENABLED) {
>+			cli->sign_info.negotiated_smb_signing = True;
> 		}
> 
> 	} else if (cli->protocol >= PROTOCOL_LANMAN1) {
>

Actions: View

Attachments on bug 1138: 430