r9505: Work on GENSEC and the code that calls it, for tighter interface

requirements, and for better error reporting.

In particular, the composite session setup (extended security/SPNEGO)
code now returns errors, rather than NT_STATUS_NO_MEMORY.  This is
seen particularly when GENSEC fails to start.

The tighter interface rules apply to NTLMSSP, which must be called
exactly the right number of times.  This is to match some of our other
less-tested modules, where adding flexablity is harder.  (and this is
security code, so let's just get it right).  As such, the DCE/RPC and
LDAP clients have been updated.

Andrew Bartlett
(This used to be commit 134550cf752b9edad66c3368750bfb4bbd9d55d1)

6 files changed, 165 insertions, 97 deletions

diff --git a/source4/auth/ntlmssp/ntlmssp.c b/source4/auth/ntlmssp/ntlmssp.c
index 82d6dd0e8f..ef870af3bf 100644
--- a/source4/auth/ntlmssp/ntlmssp.c
+++ b/source4/auth/ntlmssp/ntlmssp.c
@@ -123,7 +123,12 @@ static NTSTATUS gensec_ntlmssp_update(struct gensec_security *gensec_security,
 	*out = data_blob(NULL, 0);
 
 	if (gensec_ntlmssp_state->expected_state == NTLMSSP_DONE) {
-		return NT_STATUS_OK;
+		/* We are strict here because other modules, which we
+		 * don't fully control (such as GSSAPI) are also
+		 * strict, but are tested less often */
+
+		DEBUG(1, ("Called NTLMSSP after state machine was 'done'\n"));
+		return NT_STATUS_INVALID_PARAMETER;
 	}
 
 	if (!out_mem_ctx) {
diff --git a/source4/auth/ntlmssp/ntlmssp_client.c b/source4/auth/ntlmssp/ntlmssp_client.c
index 726885c82f..add774f84e 100644
--- a/source4/auth/ntlmssp/ntlmssp_client.c
+++ b/source4/auth/ntlmssp/ntlmssp_client.c
@@ -353,11 +353,13 @@ NTSTATUS ntlmssp_client_challenge(struct gensec_security *gensec_security,
 
 	gensec_ntlmssp_state->expected_state = NTLMSSP_DONE;
 
-	nt_status = ntlmssp_sign_init(gensec_ntlmssp_state);
-	if (!NT_STATUS_IS_OK(nt_status)) {
-		DEBUG(1, ("Could not setup NTLMSSP signing/sealing system (error was: %s)\n", 
-			  nt_errstr(nt_status)));
-		return nt_status;
+	if (gensec_security->want_features & GENSEC_FEATURE_SIGN|GENSEC_FEATURE_SEAL) {
+		nt_status = ntlmssp_sign_init(gensec_ntlmssp_state);
+		if (!NT_STATUS_IS_OK(nt_status)) {
+			DEBUG(1, ("Could not setup NTLMSSP signing/sealing system (error was: %s)\n", 
+				  nt_errstr(nt_status)));
+			return nt_status;
+		}
 	}
 
 	return nt_status;
diff --git a/source4/libcli/composite/connect.c b/source4/libcli/composite/connect.c
index adac9bcf67..4d35f5c73a 100644
--- a/source4/libcli/composite/connect.c
+++ b/source4/libcli/composite/connect.c
@@ -136,6 +136,9 @@ static NTSTATUS connect_session_setup(struct composite_context *c,
 
 	state->req = smb_raw_tcon_send(io->out.tree, state->io_tcon);
 	NT_STATUS_HAVE_NO_MEMORY(state->req);
+	if (state->req->state == SMBCLI_REQUEST_ERROR) {
+		return state->req->status;
+	}
 
 	state->req->async.fn = request_handler;
 	state->req->async.private = c;
@@ -171,6 +174,9 @@ static NTSTATUS connect_negprot(struct composite_context *c,
 
 	state->creq = smb_composite_sesssetup_send(state->session, state->io_setup);
 	NT_STATUS_HAVE_NO_MEMORY(state->creq);
+	if (state->creq->state == SMBCLI_REQUEST_ERROR) {
+		return state->creq->status;
+	}
 
 	state->creq->async.fn = composite_handler;
 	state->creq->async.private = c;
diff --git a/source4/libcli/composite/sesssetup.c b/source4/libcli/composite/sesssetup.c
index 832f7e3d60..b27290d5e4 100644
--- a/source4/libcli/composite/sesssetup.c
+++ b/source4/libcli/composite/sesssetup.c
@@ -29,7 +29,7 @@
 
 struct sesssetup_state {
 	union smb_sesssetup setup;
-	NTSTATUS session_key_err;
+	NTSTATUS gensec_status;
 	struct smb_composite_sesssetup *io;
 	struct smbcli_request *req;
 };
@@ -78,6 +78,7 @@ static void request_handler(struct smbcli_request *req)
 	struct smbcli_session *session = req->session;
 	DATA_BLOB session_key = data_blob(NULL, 0);
 	DATA_BLOB null_data_blob = data_blob(NULL, 0);
+	NTSTATUS session_key_err;
 
 	c->status = smb_raw_sesssetup_recv(req, state, &state->setup);
 
@@ -96,29 +97,41 @@ static void request_handler(struct smbcli_request *req)
 		    !NT_STATUS_IS_OK(c->status)) {
 			break;
 		}
-		c->status = gensec_update(session->gensec, state,
-					  state->setup.spnego.out.secblob,
-					  &state->setup.spnego.in.secblob);
-		if (!NT_STATUS_EQUAL(c->status, NT_STATUS_MORE_PROCESSING_REQUIRED) && 
-		    !NT_STATUS_IS_OK(c->status)) {
-			break;
-		}
-		if (state->setup.spnego.in.secblob.length == 0) {
-			break;
+		if (NT_STATUS_EQUAL(state->gensec_status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
+
+			/* The status value here, from the earlier pass at GENSEC is
+			 * vital to the security of the system.  Even if the other end
+			 * accepts, if GENSEC claims 'MORE_PROCESSING_REQUIRED' then
+			 * you must keep feeding it blobs, or else the remote
+			 * host/attacker might avoid mutal authentication
+			 * requirements */
+			
+			state->gensec_status = gensec_update(session->gensec, state,
+							 state->setup.spnego.out.secblob,
+							 &state->setup.spnego.in.secblob);
+			c->status = state->gensec_status;
+			if (!NT_STATUS_EQUAL(c->status, NT_STATUS_MORE_PROCESSING_REQUIRED) && 
+			    !NT_STATUS_IS_OK(c->status)) {
+				break;
+			}
+		} else {
+			state->setup.spnego.in.secblob = data_blob(NULL, 0);
 		}
-
+			
 		/* we need to do another round of session setup. We keep going until both sides
 		   are happy */
-		state->session_key_err = gensec_session_key(session->gensec, &session_key);
-		if (NT_STATUS_IS_OK(state->session_key_err)) {
+		session_key_err = gensec_session_key(session->gensec, &session_key);
+		if (NT_STATUS_IS_OK(session_key_err)) {
 			set_user_session_key(session, &session_key);
 			smbcli_transport_simple_set_signing(session->transport, session_key, null_data_blob);
 		}
 
-		state->req = smb_raw_sesssetup_send(session, &state->setup);
-		state->req->async.fn = request_handler;
-		state->req->async.private = c;
-		return;
+		if (state->setup.spnego.in.secblob.length) {
+			state->req = smb_raw_sesssetup_send(session, &state->setup);
+			state->req->async.fn = request_handler;
+			state->req->async.private = c;
+			return;
+		}
 	}
 
 	/* enforce the local signing required flag */
@@ -144,9 +157,10 @@ static void request_handler(struct smbcli_request *req)
 /*
   send a nt1 style session setup
 */
-static struct smbcli_request *session_setup_nt1(struct composite_context *c,
-						struct smbcli_session *session, 
-						struct smb_composite_sesssetup *io) 
+static NTSTATUS session_setup_nt1(struct composite_context *c,
+				  struct smbcli_session *session, 
+				  struct smb_composite_sesssetup *io,
+				  struct smbcli_request **req) 
 {
 	struct sesssetup_state *state = talloc_get_type(c->private, struct sesssetup_state);
 	const struct samr_Password *nt_hash = cli_credentials_get_nt_hash(io->in.credentials, state);
@@ -180,7 +194,7 @@ static struct smbcli_request *session_setup_nt1(struct composite_context *c,
 						   &lmv2_response, &ntlmv2_response, 
 						   &lmv2_session_key, &session_key)) {
 				data_blob_free(&names_blob);
-				return NULL;
+				return NT_STATUS_NO_MEMORY;
 			}
 			data_blob_free(&names_blob);
 			state->setup.nt1.in.password1 = lmv2_response;
@@ -218,19 +232,24 @@ static struct smbcli_request *session_setup_nt1(struct composite_context *c,
 		state->setup.nt1.in.password2 = data_blob(NULL, 0);
 	} else {
 		/* could match windows client and return 'cannot logon from this workstation', but it just confuses everybody */
-		return NULL;
+		return NT_STATUS_INVALID_PARAMETER;
 	}
 
-	return smb_raw_sesssetup_send(session, &state->setup);
+	*req = smb_raw_sesssetup_send(session, &state->setup);
+	if (!*req) {
+		return NT_STATUS_NO_MEMORY;
+	}
+	return (*req)->status;
 }
 
 
 /*
   old style session setup (pre NT1 protocol level)
 */
-static struct smbcli_request *session_setup_old(struct composite_context *c,
-						struct smbcli_session *session,
-						struct smb_composite_sesssetup *io)
+static NTSTATUS session_setup_old(struct composite_context *c,
+				  struct smbcli_session *session, 
+				  struct smb_composite_sesssetup *io,
+				  struct smbcli_request **req) 
 {
 	struct sesssetup_state *state = talloc_get_type(c->private, struct sesssetup_state);
 	const char *password = cli_credentials_get_password(io->in.credentials);
@@ -256,19 +275,24 @@ static struct smbcli_request *session_setup_old(struct composite_context *c,
 								strlen(password));
 	}
 	
-	return smb_raw_sesssetup_send(session, &state->setup);
+	*req = smb_raw_sesssetup_send(session, &state->setup);
+	if (!*req) {
+		return NT_STATUS_NO_MEMORY;
+	}
+	return (*req)->status;
 }
 
 
 /*
-  old style session setup (pre NT1 protocol level)
+  Modern, all singing, all dancing extended security (and possibly SPNEGO) request
 */
-static struct smbcli_request *session_setup_spnego(struct composite_context *c, 
-						   struct smbcli_session *session,
-						   struct smb_composite_sesssetup *io)
+static NTSTATUS session_setup_spnego(struct composite_context *c,
+				     struct smbcli_session *session, 
+				     struct smb_composite_sesssetup *io,
+				     struct smbcli_request **req) 
 {
 	struct sesssetup_state *state = talloc_get_type(c->private, struct sesssetup_state);
-	NTSTATUS status;
+	NTSTATUS status, session_key_err;
 	DATA_BLOB session_key = data_blob(NULL, 0);
 	DATA_BLOB null_data_blob = data_blob(NULL, 0);
 	const char *chosen_oid = NULL;
@@ -290,7 +314,7 @@ static struct smbcli_request *session_setup_spnego(struct composite_context *c,
 	status = gensec_client_start(session, &session->gensec, c->event_ctx);
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(1, ("Failed to start GENSEC client mode: %s\n", nt_errstr(status)));
-		return NULL;
+		return status;
 	}
 
 	gensec_want_feature(session->gensec, GENSEC_FEATURE_SESSION_KEY);
@@ -299,21 +323,21 @@ static struct smbcli_request *session_setup_spnego(struct composite_context *c,
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(1, ("Failed to start set GENSEC client credentails: %s\n", 
 			  nt_errstr(status)));
-		return NULL;
+		return status;
 	}
 
 	status = gensec_set_target_hostname(session->gensec, session->transport->socket->hostname);
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(1, ("Failed to start set GENSEC target hostname: %s\n", 
 			  nt_errstr(status)));
-		return NULL;
+		return status;
 	}
 
 	status = gensec_set_target_service(session->gensec, "cifs");
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(1, ("Failed to start set GENSEC target service: %s\n", 
 			  nt_errstr(status)));
-		return NULL;
+		return status;
 	}
 
 	if (session->transport->negotiate.secblob.length) {
@@ -327,24 +351,30 @@ static struct smbcli_request *session_setup_spnego(struct composite_context *c,
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(1, ("Failed to start set GENSEC client SPNEGO mechanism %s: %s\n",
 			  gensec_get_name_by_oid(chosen_oid), nt_errstr(status)));
-		return NULL;
+		return status;
 	}
 	
 	status = gensec_update(session->gensec, state,
 			       session->transport->negotiate.secblob,
 			       &state->setup.spnego.in.secblob);
-	if (!NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
+	if (!NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED) && 
+	    !NT_STATUS_IS_OK(status)) {
 		DEBUG(1, ("Failed initial gensec_update with mechanism %s: %s\n",
 			  gensec_get_name_by_oid(chosen_oid), nt_errstr(status)));
-		return NULL;
+		return status;
 	}
+	state->gensec_status = status;
 
-	state->session_key_err = gensec_session_key(session->gensec, &session_key);
-	if (NT_STATUS_IS_OK(state->session_key_err)) {
+	session_key_err = gensec_session_key(session->gensec, &session_key);
+	if (NT_STATUS_IS_OK(session_key_err)) {
 		smbcli_transport_simple_set_signing(session->transport, session_key, null_data_blob);
 	}
 
-	return smb_raw_sesssetup_send(session, &state->setup);
+	*req = smb_raw_sesssetup_send(session, &state->setup);
+	if (!*req) {
+		return NT_STATUS_NO_MEMORY;
+	}
+	return (*req)->status;
 }
 
 
@@ -358,12 +388,16 @@ struct composite_context *smb_composite_sesssetup_send(struct smbcli_session *se
 {
 	struct composite_context *c;
 	struct sesssetup_state *state;
+	NTSTATUS status;
 
 	c = talloc_zero(session, struct composite_context);
-	if (c == NULL) goto failed;
+	if (c == NULL) return NULL;
 
 	state = talloc(c, struct sesssetup_state);
-	if (state == NULL) goto failed;
+	if (state == NULL) {
+		c->state = SMBCLI_REQUEST_ERROR;
+		c->status = NT_STATUS_NO_MEMORY;
+	}
 
 	state->io = io;
 
@@ -380,24 +414,24 @@ struct composite_context *smb_composite_sesssetup_send(struct smbcli_session *se
 
 	/* see what session setup interface we will use */
 	if (session->transport->negotiate.protocol < PROTOCOL_NT1) {
-		state->req = session_setup_old(c, session, io);
+		status = session_setup_old(c, session, io, &state->req);
 	} else if (!session->transport->options.use_spnego ||
 		   !(io->in.capabilities & CAP_EXTENDED_SECURITY)) {
-		state->req = session_setup_nt1(c, session, io);
+		status = session_setup_nt1(c, session, io, &state->req);
 	} else {
-		state->req = session_setup_spnego(c, session, io);
+		status = session_setup_spnego(c, session, io, &state->req);
 	}
 
-	if (state->req == NULL) goto failed;
-
-	state->req->async.fn = request_handler;
-	state->req->async.private = c;
+	if (NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED) || 
+	    NT_STATUS_IS_OK(status)) {
+		state->req->async.fn = request_handler;
+		state->req->async.private = c;
+		return c;
+	}
 
+	c->state = SMBCLI_REQUEST_ERROR;
+	c->status = status;
 	return c;
-
-failed:
-	talloc_free(c);
-	return NULL;
 }
 
 
diff --git a/source4/libcli/ldap/ldap_bind.c b/source4/libcli/ldap/ldap_bind.c
index e70a56779b..738222da86 100644
--- a/source4/libcli/ldap/ldap_bind.c
+++ b/source4/libcli/ldap/ldap_bind.c
@@ -141,6 +141,7 @@ NTSTATUS ldap_bind_sasl(struct ldap_connection *conn, struct cli_credentials *cr
 {
 	NTSTATUS status;
 	TALLOC_CTX *tmp_ctx = NULL;
+
 	DATA_BLOB input = data_blob(NULL, 0);
 	DATA_BLOB output = data_blob(NULL, 0);
 
@@ -183,21 +184,35 @@ NTSTATUS ldap_bind_sasl(struct ldap_connection *conn, struct cli_credentials *cr
 	tmp_ctx = talloc_new(conn);
 	if (tmp_ctx == NULL) goto failed;
 
-	status = gensec_update(conn->gensec, tmp_ctx, input, &output);
-
 	while (1) {
+		NTSTATUS gensec_status;
 		struct ldap_message *response;
 		struct ldap_message *msg;
 		struct ldap_request *req;
 		int result = LDAP_OTHER;
 	
-		if (NT_STATUS_IS_OK(status) && output.length == 0) {
-			break;
-		}
+		status = gensec_update(conn->gensec, tmp_ctx,
+				       input,
+				       &output);
+		/* The status value here, from GENSEC is vital to the security
+		 * of the system.  Even if the other end accepts, if GENSEC
+		 * claims 'MORE_PROCESSING_REQUIRED' then you must keep
+		 * feeding it blobs, or else the remote host/attacker might
+		 * avoid mutal authentication requirements.
+		 *
+		 * Likewise, you must not feed GENSEC too much (after the OK),
+		 * it doesn't like that either
+		 */
+
+		gensec_status = status;
+
 		if (!NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED) && 
 		    !NT_STATUS_IS_OK(status)) {
 			break;
 		}
+		if (output.length == 0) {
+			break;
+		}
 
 		msg = new_ldap_sasl_bind_msg(tmp_ctx, "GSS-SPNEGO", &output);
 		if (msg == NULL) {
@@ -225,12 +240,15 @@ NTSTATUS ldap_bind_sasl(struct ldap_connection *conn, struct cli_credentials *cr
 		result = response->r.BindResponse.response.resultcode;
 
 		if (result != LDAP_SUCCESS && result != LDAP_SASL_BIND_IN_PROGRESS) {
+			status = NT_STATUS_UNEXPECTED_NETWORK_ERROR;
 			break;
 		}
 
-		status = gensec_update(conn->gensec, tmp_ctx,
-				       response->r.BindResponse.SASL.secblob,
-				       &output);
+		/* This is where we check if GENSEC wanted to be fed more data */
+		if (!NT_STATUS_EQUAL(gensec_status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
+			break;
+		}
+		input = response->r.BindResponse.SASL.secblob;
 	}
 
 	if (NT_STATUS_IS_OK(status) &&
diff --git a/source4/librpc/rpc/dcerpc_auth.c b/source4/librpc/rpc/dcerpc_auth.c
index 7aa563cb9d..8ad3be4ecd 100644
--- a/source4/librpc/rpc/dcerpc_auth.c
+++ b/source4/librpc/rpc/dcerpc_auth.c
@@ -50,6 +50,8 @@ NTSTATUS dcerpc_bind_auth(struct dcerpc_pipe *p, uint8_t auth_type, uint8_t auth
 	DATA_BLOB credentials;
 	DATA_BLOB null_data_blob = data_blob(NULL, 0);
 
+	int num_passes = 0;
+
 	if (!p->conn->security_state.generic_state) {
 		status = gensec_client_start(p, &p->conn->security_state.generic_state,
 					     p->conn->event_ctx);
@@ -73,33 +75,27 @@ NTSTATUS dcerpc_bind_auth(struct dcerpc_pipe *p, uint8_t auth_type, uint8_t auth
 	p->conn->security_state.auth_info->auth_context_id = random();
 	p->conn->security_state.auth_info->credentials = null_data_blob;
 
-	status = gensec_update(p->conn->security_state.generic_state, tmp_ctx,
-			       null_data_blob,
-			       &credentials);
-
-	p->conn->security_state.auth_info->credentials = credentials;
-
-	if (NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
-		/* We are demanding a reply, so use a request that will get us one */
-		status = dcerpc_bind_byuuid(p, tmp_ctx, uuid, version);
-		if (!NT_STATUS_IS_OK(status)) {
-			goto done;
-		}
-	} else if (NT_STATUS_IS_OK(status)) {
-		/* We don't care for the reply, so jump to the end */
-		status = dcerpc_bind_byuuid(p, tmp_ctx, uuid, version);
-		goto done;
-	} else {
-		/* Something broke in GENSEC - bail */
-		goto done;
-	}
-
 	while (1) {
+		num_passes++;
 		status = gensec_update(p->conn->security_state.generic_state, tmp_ctx,
 				       p->conn->security_state.auth_info->credentials,
 				       &credentials);
+
+		/* The status value here, from GENSEC is vital to the security
+		 * of the system.  Even if the other end accepts, if GENSEC
+		 * claims 'MORE_PROCESSING_REQUIRED' then you must keep
+		 * feeding it blobs, or else the remote host/attacker might
+		 * avoid mutal authentication requirements.
+		 *
+		 * Likewise, you must not feed GENSEC too much (after the OK),
+		 * it doesn't like that either
+		 */
+
 		if (!NT_STATUS_IS_OK(status) 
 		    && !NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
+			DEBUG(1, ("Failed DCERPC client gensec_update with mechanism %s: %s\n",
+				  gensec_get_name_by_authtype(auth_type), nt_errstr(status)));
+
 			break;
 		}
 
@@ -110,18 +106,25 @@ NTSTATUS dcerpc_bind_auth(struct dcerpc_pipe *p, uint8_t auth_type, uint8_t auth
 		p->conn->security_state.auth_info->credentials = credentials;
 		
 		if (NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
-			/* We are demanding a reply, so use a request that will get us one */
-			status = dcerpc_alter_context(p, tmp_ctx, &p->syntax, &p->transfer_syntax);
+			if (num_passes == 1) {
+				status = dcerpc_bind_byuuid(p, tmp_ctx, uuid, version);
+			} else {
+				/* We are demanding a reply, so use a request that will get us one */
+				status = dcerpc_alter_context(p, tmp_ctx, &p->syntax, &p->transfer_syntax);
+			}
 			if (!NT_STATUS_IS_OK(status)) {
 				break;
 			}
-		} else {
+		} else if (NT_STATUS_IS_OK(status)) {
 			/* NO reply expected, so just send it */
-			status = dcerpc_auth3(p->conn, tmp_ctx);
-			credentials = data_blob(NULL, 0);
-			if (!NT_STATUS_IS_OK(status)) {
-				break;
+			if (num_passes == 1) {
+				status = dcerpc_bind_byuuid(p, tmp_ctx, uuid, version);
+			} else {
+				status = dcerpc_auth3(p->conn, tmp_ctx);
 			}
+			break;
+		} else {
+			break;
 		}
 	};