CVE-2015-5370: s4:librpc/rpc: avoid using dcecli_security->auth_info and use per request values

We now avoid reusing the same auth_info structure for incoming and outgoing
values. We need to make sure that the remote server doesn't overwrite our own
values.

This will trigger some failures with our currently broken server,
which will be fixed in the next commits.

The broken server requires an dcerpc_auth structure with no credentials
in order to do an alter_context request that just creates a presentation
context without doing authentication.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11344

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Günther Deschner <gd@samba.org>

3 files changed, 62 insertions, 44 deletions

diff --git a/source4/librpc/rpc/dcerpc.c b/source4/librpc/rpc/dcerpc.c
index 5113f63a22a..908fed20113 100644
--- a/source4/librpc/rpc/dcerpc.c
+++ b/source4/librpc/rpc/dcerpc.c
@@ -144,7 +144,6 @@ static struct dcecli_connection *dcerpc_connection_init(TALLOC_CTX *mem_ctx,
 	c->security_state.auth_type = DCERPC_AUTH_TYPE_NONE;
 	c->security_state.auth_level = DCERPC_AUTH_LEVEL_NONE;
 	c->security_state.auth_context_id = 0;
-	c->security_state.auth_info = NULL;
 	c->security_state.session_key = dcerpc_generic_session_key;
 	c->security_state.generic_state = NULL;
 	c->flags = 0;
@@ -1226,7 +1225,7 @@ struct tevent_req *dcerpc_bind_send(TALLOC_CTX *mem_ctx,
 
 	/* construct the NDR form of the packet */
 	status = ncacn_push_auth(&blob, state, &pkt,
-				 p->conn->security_state.auth_info);
+				 p->conn->security_state.tmp_auth_info.out);
 	if (tevent_req_nterror(req, status)) {
 		return tevent_req_post(req, ev);
 	}
@@ -1299,6 +1298,7 @@ static void dcerpc_bind_recv_handler(struct rpc_request *subreq,
 		tevent_req_data(req,
 		struct dcerpc_bind_state);
 	struct dcecli_connection *conn = state->p->conn;
+	struct dcecli_security *sec = &conn->security_state;
 	struct dcerpc_binding *b = NULL;
 	NTSTATUS status;
 	uint32_t flags;
@@ -1372,11 +1372,13 @@ static void dcerpc_bind_recv_handler(struct rpc_request *subreq,
 	}
 
 	/* the bind_ack might contain a reply set of credentials */
-	if (conn->security_state.auth_info && pkt->u.bind_ack.auth_info.length) {
+	if (pkt->auth_length != 0 && sec->tmp_auth_info.in != NULL) {
 		uint32_t auth_length;
 
-		status = dcerpc_pull_auth_trailer(pkt, conn, &pkt->u.bind_ack.auth_info,
-						  conn->security_state.auth_info, &auth_length, true);
+		status = dcerpc_pull_auth_trailer(pkt, sec->tmp_auth_info.mem,
+						  &pkt->u.bind_ack.auth_info,
+						  sec->tmp_auth_info.in,
+						  &auth_length, true);
 		if (tevent_req_nterror(req, status)) {
 			return;
 		}
@@ -1426,9 +1428,8 @@ NTSTATUS dcerpc_auth3(struct dcerpc_pipe *p,
 	}
 
 	/* construct the NDR form of the packet */
-	status = ncacn_push_auth(&blob, mem_ctx,
-				 &pkt,
-				 p->conn->security_state.auth_info);
+	status = ncacn_push_auth(&blob, mem_ctx, &pkt,
+				 p->conn->security_state.tmp_auth_info.out);
 	if (!NT_STATUS_IS_OK(status)) {
 		return status;
 	}
@@ -2234,7 +2235,7 @@ struct tevent_req *dcerpc_alter_context_send(TALLOC_CTX *mem_ctx,
 
 	/* construct the NDR form of the packet */
 	status = ncacn_push_auth(&blob, state, &pkt,
-				 p->conn->security_state.auth_info);
+				 p->conn->security_state.tmp_auth_info.out);
 	if (tevent_req_nterror(req, status)) {
 		return tevent_req_post(req, ev);
 	}
@@ -2307,6 +2308,7 @@ static void dcerpc_alter_context_recv_handler(struct rpc_request *subreq,
 		tevent_req_data(req,
 		struct dcerpc_alter_context_state);
 	struct dcecli_connection *conn = state->p->conn;
+	struct dcecli_security *sec = &conn->security_state;
 	NTSTATUS status;
 
 	/*
@@ -2365,12 +2367,13 @@ static void dcerpc_alter_context_recv_handler(struct rpc_request *subreq,
 	}
 
 	/* the alter_resp might contain a reply set of credentials */
-	if (conn->security_state.auth_info &&
-	    pkt->u.alter_resp.auth_info.length) {
+	if (pkt->auth_length != 0 && sec->tmp_auth_info.in != NULL) {
 		uint32_t auth_length;
 
-		status = dcerpc_pull_auth_trailer(pkt, conn, &pkt->u.alter_resp.auth_info,
-						  conn->security_state.auth_info, &auth_length, true);
+		status = dcerpc_pull_auth_trailer(pkt, sec->tmp_auth_info.mem,
+						  &pkt->u.alter_resp.auth_info,
+						  sec->tmp_auth_info.in,
+						  &auth_length, true);
 		if (tevent_req_nterror(req, status)) {
 			return;
 		}
diff --git a/source4/librpc/rpc/dcerpc.h b/source4/librpc/rpc/dcerpc.h
index c7299fec5e6..39d28a6e5d4 100644
--- a/source4/librpc/rpc/dcerpc.h
+++ b/source4/librpc/rpc/dcerpc.h
@@ -49,7 +49,11 @@ struct dcecli_security {
 	enum dcerpc_AuthType auth_type;
 	enum dcerpc_AuthLevel auth_level;
 	uint32_t auth_context_id;
-	struct dcerpc_auth *auth_info;
+	struct {
+		struct dcerpc_auth *out;
+		struct dcerpc_auth *in;
+		TALLOC_CTX *mem;
+	} tmp_auth_info;
 	struct gensec_security *generic_state;
 
 	/* get the session key */
diff --git a/source4/librpc/rpc/dcerpc_auth.c b/source4/librpc/rpc/dcerpc_auth.c
index 443c7587e72..15a843b4ef5 100644
--- a/source4/librpc/rpc/dcerpc_auth.c
+++ b/source4/librpc/rpc/dcerpc_auth.c
@@ -124,7 +124,8 @@ _PUBLIC_ NTSTATUS dcerpc_bind_auth_none(struct dcerpc_pipe *p,
 
 struct bind_auth_state {
 	struct dcerpc_pipe *pipe;
-	DATA_BLOB credentials;
+	struct dcerpc_auth out_auth_info;
+	struct dcerpc_auth in_auth_info;
 	bool more_processing;	/* Is there anything more to do after the
 				 * first bind itself received? */
 };
@@ -141,6 +142,12 @@ static void bind_auth_next_step(struct composite_context *c)
 	state = talloc_get_type(c->private_data, struct bind_auth_state);
 	sec = &state->pipe->conn->security_state;
 
+	state->out_auth_info = (struct dcerpc_auth) {
+		.auth_type = sec->auth_type,
+		.auth_level = sec->auth_level,
+		.auth_context_id = sec->auth_context_id,
+	};
+
 	/* 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
@@ -156,16 +163,14 @@ static void bind_auth_next_step(struct composite_context *c)
 
 	c->status = gensec_update_ev(sec->generic_state, state,
 				  state->pipe->conn->event_ctx,
-				  sec->auth_info->credentials,
-				  &state->credentials);
+				  state->in_auth_info.credentials,
+				  &state->out_auth_info.credentials);
 	if (state->pipe->timed_out) {
 		composite_error(c, NT_STATUS_IO_TIMEOUT);
 		return;
 	}
 	state->pipe->inhibit_timeout_processing = false;
 
-	data_blob_free(&sec->auth_info->credentials);
-
 	if (NT_STATUS_EQUAL(c->status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
 		more_processing = true;
 		c->status = NT_STATUS_OK;
@@ -173,18 +178,21 @@ static void bind_auth_next_step(struct composite_context *c)
 
 	if (!composite_is_ok(c)) return;
 
-	if (state->credentials.length == 0) {
+	if (state->out_auth_info.credentials.length == 0) {
 		composite_done(c);
 		return;
 	}
 
-	sec->auth_info->credentials = state->credentials;
+	state->in_auth_info = (struct dcerpc_auth) {
+		.auth_type = DCERPC_AUTH_TYPE_NONE,
+	};
+	sec->tmp_auth_info.in = &state->in_auth_info;
+	sec->tmp_auth_info.mem = state;
+	sec->tmp_auth_info.out = &state->out_auth_info;
 
 	if (!more_processing) {
 		/* NO reply expected, so just send it */
 		c->status = dcerpc_auth3(state->pipe, state);
-		data_blob_free(&state->credentials);
-		sec->auth_info->credentials = data_blob(NULL, 0);
 		if (!composite_is_ok(c)) return;
 
 		composite_done(c);
@@ -197,8 +205,6 @@ static void bind_auth_next_step(struct composite_context *c)
 					   state->pipe,
 					   &state->pipe->syntax,
 					   &state->pipe->transfer_syntax);
-	data_blob_free(&state->credentials);
-	sec->auth_info->credentials = data_blob(NULL, 0);
 	if (composite_nomem(subreq, c)) return;
 	tevent_req_set_callback(subreq, bind_auth_recv_alter, c);
 }
@@ -209,6 +215,11 @@ static void bind_auth_recv_alter(struct tevent_req *subreq)
 	struct composite_context *c =
 		tevent_req_callback_data(subreq,
 		struct composite_context);
+	struct bind_auth_state *state =	talloc_get_type(c->private_data,
+							struct bind_auth_state);
+	struct dcecli_security *sec = &state->pipe->conn->security_state;
+
+	ZERO_STRUCT(sec->tmp_auth_info);
 
 	c->status = dcerpc_alter_context_recv(subreq);
 	TALLOC_FREE(subreq);
@@ -225,14 +236,15 @@ static void bind_auth_recv_bindreply(struct tevent_req *subreq)
 		struct composite_context);
 	struct bind_auth_state *state =	talloc_get_type(c->private_data,
 							struct bind_auth_state);
+	struct dcecli_security *sec = &state->pipe->conn->security_state;
+
+	ZERO_STRUCT(sec->tmp_auth_info);
 
 	c->status = dcerpc_bind_recv(subreq);
 	TALLOC_FREE(subreq);
 	if (!composite_is_ok(c)) return;
 
 	if (state->pipe->conn->flags & DCERPC_HEADER_SIGNING) {
-		struct dcecli_security *sec = &state->pipe->conn->security_state;
-
 		gensec_want_feature(sec->generic_state, GENSEC_FEATURE_SIGN_PKT_HEADER);
 	}
 
@@ -362,15 +374,11 @@ struct composite_context *dcerpc_bind_auth_send(TALLOC_CTX *mem_ctx,
 	 */
 	sec->auth_context_id = 1;
 
-	sec->auth_info = talloc(p, struct dcerpc_auth);
-	if (composite_nomem(sec->auth_info, c)) return c;
-
-	sec->auth_info->auth_type = sec->auth_type;
-	sec->auth_info->auth_level = sec->auth_level,
-	sec->auth_info->auth_pad_length = 0;
-	sec->auth_info->auth_reserved = 0;
-	sec->auth_info->auth_context_id = sec->auth_context_id;
-	sec->auth_info->credentials = data_blob(NULL, 0);
+	state->out_auth_info = (struct dcerpc_auth) {
+		.auth_type = sec->auth_type,
+		.auth_level = sec->auth_level,
+		.auth_context_id = sec->auth_context_id,
+	};
 
 	/* The status value here, from GENSEC is vital to the security
 	 * of the system.  Even if the other end accepts, if GENSEC
@@ -386,8 +394,8 @@ struct composite_context *dcerpc_bind_auth_send(TALLOC_CTX *mem_ctx,
 	state->pipe->timed_out = false;
 	c->status = gensec_update_ev(sec->generic_state, state,
 				  p->conn->event_ctx,
-				  sec->auth_info->credentials,
-				  &state->credentials);
+				  data_blob_null,
+				  &state->out_auth_info.credentials);
 	if (state->pipe->timed_out) {
 		composite_error(c, NT_STATUS_IO_TIMEOUT);
 		return c;
@@ -403,25 +411,28 @@ struct composite_context *dcerpc_bind_auth_send(TALLOC_CTX *mem_ctx,
 	state->more_processing = NT_STATUS_EQUAL(c->status,
 						 NT_STATUS_MORE_PROCESSING_REQUIRED);
 
-	if (state->credentials.length == 0) {
+	if (state->out_auth_info.credentials.length == 0) {
 		composite_done(c);
 		return c;
 	}
 
-	sec->auth_info->credentials = state->credentials;
-
 	if (gensec_have_feature(sec->generic_state, GENSEC_FEATURE_SIGN_PKT_HEADER)) {
-		if (auth_level >= DCERPC_AUTH_LEVEL_INTEGRITY) {
+		if (sec->auth_level >= DCERPC_AUTH_LEVEL_INTEGRITY) {
 			state->pipe->conn->flags |= DCERPC_PROPOSE_HEADER_SIGNING;
 		}
 	}
 
+	state->in_auth_info = (struct dcerpc_auth) {
+		.auth_type = DCERPC_AUTH_TYPE_NONE,
+	};
+	sec->tmp_auth_info.in = &state->in_auth_info;
+	sec->tmp_auth_info.mem = state;
+	sec->tmp_auth_info.out = &state->out_auth_info;
+
 	/* The first request always is a dcerpc_bind. The subsequent ones
 	 * depend on gensec results */
 	subreq = dcerpc_bind_send(state, p->conn->event_ctx, p,
 				  &syntax, &transfer_syntax);
-	data_blob_free(&state->credentials);
-	sec->auth_info->credentials = data_blob(NULL, 0);
 	if (composite_nomem(subreq, c)) return c;
 	tevent_req_set_callback(subreq, bind_auth_recv_bindreply, c);