CVE-2015-5370: s3:librpc/rpc: remove auth trailer and possible padding within dcerpc_check_auth()

This simplifies the callers a lot.

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>

4 files changed, 33 insertions, 53 deletions

diff --git a/source3/librpc/rpc/dcerpc.h b/source3/librpc/rpc/dcerpc.h
index e7d66b7252b..0a82e7eea8f 100644
--- a/source3/librpc/rpc/dcerpc.h
+++ b/source3/librpc/rpc/dcerpc.h
@@ -83,8 +83,7 @@ NTSTATUS dcerpc_add_auth_footer(struct pipe_auth_data *auth,
 NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 			   struct ncacn_packet *pkt,
 			   DATA_BLOB *pkt_trailer,
-			   size_t header_size,
-			   DATA_BLOB *raw_pkt,
-			   size_t *pad_len);
+			   uint8_t header_size,
+			   DATA_BLOB *raw_pkt);
 
 #endif /* __S3_DCERPC_H__ */
diff --git a/source3/librpc/rpc/dcerpc_helpers.c b/source3/librpc/rpc/dcerpc_helpers.c
index 96074a4705c..bb1da467ccc 100644
--- a/source3/librpc/rpc/dcerpc_helpers.c
+++ b/source3/librpc/rpc/dcerpc_helpers.c
@@ -481,19 +481,18 @@ NTSTATUS dcerpc_add_auth_footer(struct pipe_auth_data *auth,
 *
 * @param auth		The auth data for the connection
 * @param pkt		The actual ncacn_packet
-* @param pkt_trailer	The stub_and_verifier part of the packet
+* @param pkt_trailer [in][out]	The stub_and_verifier part of the packet,
+* 			the auth_trailer and padding will be removed.
 * @param header_size	The header size
 * @param raw_pkt	The whole raw packet data blob
-* @param pad_len	[out] The padding length used in the packet
 *
 * @return A NTSTATUS error code
 */
 NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 			   struct ncacn_packet *pkt,
 			   DATA_BLOB *pkt_trailer,
-			   size_t header_size,
-			   DATA_BLOB *raw_pkt,
-			   size_t *pad_len)
+			   uint8_t header_size,
+			   DATA_BLOB *raw_pkt)
 {
 	struct gensec_security *gensec_security;
 	NTSTATUS status;
@@ -502,6 +501,14 @@ NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 	DATA_BLOB full_pkt;
 	DATA_BLOB data;
 
+	/*
+	 * These check should be done in the caller.
+	 */
+	SMB_ASSERT(raw_pkt->length == pkt->frag_length);
+	SMB_ASSERT(header_size <= pkt->frag_length);
+	SMB_ASSERT(pkt_trailer->length < pkt->frag_length);
+	SMB_ASSERT((pkt_trailer->length + header_size) <= pkt->frag_length);
+
 	switch (auth->auth_level) {
 	case DCERPC_AUTH_LEVEL_PRIVACY:
 		DEBUG(10, ("Requested Privacy.\n"));
@@ -515,7 +522,6 @@ NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 		if (pkt->auth_length != 0) {
 			break;
 		}
-		*pad_len = 0;
 		return NT_STATUS_OK;
 
 	case DCERPC_AUTH_LEVEL_NONE:
@@ -524,7 +530,6 @@ NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 				  "authenticated connection!\n"));
 			return NT_STATUS_INVALID_PARAMETER;
 		}
-		*pad_len = 0;
 		return NT_STATUS_OK;
 
 	default:
@@ -543,10 +548,11 @@ NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 		return status;
 	}
 
+	pkt_trailer->length -= auth_length;
 	data = data_blob_const(raw_pkt->data + header_size,
-				pkt_trailer->length - auth_length);
-	full_pkt = data_blob_const(raw_pkt->data,
-				raw_pkt->length - auth_info.credentials.length);
+			       pkt_trailer->length);
+	full_pkt = data_blob_const(raw_pkt->data, raw_pkt->length);
+	full_pkt.length -= auth_info.credentials.length;
 
 	switch (auth->auth_type) {
 	case DCERPC_AUTH_TYPE_NONE:
@@ -571,10 +577,13 @@ NTSTATUS dcerpc_check_auth(struct pipe_auth_data *auth,
 	 * pkt_trailer actually has a copy of the raw data, and they
 	 * are still both used in later calls */
 	if (auth->auth_level == DCERPC_AUTH_LEVEL_PRIVACY) {
+		if (pkt_trailer->length != data.length) {
+			return NT_STATUS_INVALID_PARAMETER;
+		}
 		memcpy(pkt_trailer->data, data.data, data.length);
 	}
 
-	*pad_len = auth_info.auth_pad_length;
+	pkt_trailer->length -= auth_info.auth_pad_length;
 	data_blob_free(&auth_info.credentials);
 	return NT_STATUS_OK;
 }
diff --git a/source3/rpc_client/cli_pipe.c b/source3/rpc_client/cli_pipe.c
index 6e0a7eada4c..0fb848f2b4c 100644
--- a/source3/rpc_client/cli_pipe.c
+++ b/source3/rpc_client/cli_pipe.c
@@ -391,9 +391,9 @@ static NTSTATUS cli_pipe_validate_current_pdu(TALLOC_CTX *mem_ctx,
 						DATA_BLOB *rdata,
 						DATA_BLOB *reply_pdu)
 {
-	struct dcerpc_response *r;
+	const struct dcerpc_response *r = NULL;
+	DATA_BLOB tmp_stub = data_blob_null;
 	NTSTATUS ret = NT_STATUS_OK;
-	size_t pad_len = 0;
 
 	/*
 	 * Point the return values at the real data including the RPC
@@ -427,37 +427,24 @@ static NTSTATUS cli_pipe_validate_current_pdu(TALLOC_CTX *mem_ctx,
 
 		r = &pkt->u.response;
 
+		tmp_stub.data = r->stub_and_verifier.data;
+		tmp_stub.length = r->stub_and_verifier.length;
+
 		/* Here's where we deal with incoming sign/seal. */
 		ret = dcerpc_check_auth(cli->auth, pkt,
-					&r->stub_and_verifier,
+					&tmp_stub,
 					DCERPC_RESPONSE_LENGTH,
-					pdu, &pad_len);
+					pdu);
 		if (!NT_STATUS_IS_OK(ret)) {
 			return ret;
 		}
 
-		if (pkt->frag_length < DCERPC_RESPONSE_LENGTH + pad_len) {
-			return NT_STATUS_BUFFER_TOO_SMALL;
-		}
-
 		/* Point the return values at the NDR data. */
-		rdata->data = r->stub_and_verifier.data;
-
-		if (pkt->auth_length) {
-			/* We've already done integer wrap tests in
-			 * dcerpc_check_auth(). */
-			rdata->length = r->stub_and_verifier.length
-					 - pad_len
-					 - DCERPC_AUTH_TRAILER_LENGTH
-					 - pkt->auth_length;
-		} else {
-			rdata->length = r->stub_and_verifier.length;
-		}
+		*rdata = tmp_stub;
 
-		DEBUG(10, ("Got pdu len %lu, data_len %lu, ss_len %u\n",
+		DEBUG(10, ("Got pdu len %lu, data_len %lu\n",
 			   (long unsigned int)pdu->length,
-			   (long unsigned int)rdata->length,
-			   (unsigned int)pad_len));
+			   (long unsigned int)rdata->length));
 
 		/*
 		 * If this is the first reply, and the allocation hint is
diff --git a/source3/rpc_server/srv_pipe.c b/source3/rpc_server/srv_pipe.c
index e6e39df3eb3..e5c7063a148 100644
--- a/source3/rpc_server/srv_pipe.c
+++ b/source3/rpc_server/srv_pipe.c
@@ -1437,7 +1437,6 @@ static NTSTATUS dcesrv_auth_request(struct pipe_auth_data *auth,
 {
 	NTSTATUS status;
 	size_t hdr_size = DCERPC_REQUEST_LENGTH;
-	size_t pad_len;
 
 	DEBUG(10, ("Checking request auth.\n"));
 
@@ -1448,25 +1447,11 @@ static NTSTATUS dcesrv_auth_request(struct pipe_auth_data *auth,
 	/* in case of sealing this function will unseal the data in place */
 	status = dcerpc_check_auth(auth, pkt,
 				   &pkt->u.request.stub_and_verifier,
-				   hdr_size, raw_pkt,
-				   &pad_len);
+				   hdr_size, raw_pkt);
 	if (!NT_STATUS_IS_OK(status)) {
 		return status;
 	}
 
-
-	/* remove padding and auth trailer,
-	 * this way the caller will get just the data */
-	if (pkt->auth_length) {
-		size_t trail_len = pad_len
-					+ DCERPC_AUTH_TRAILER_LENGTH
-					+ pkt->auth_length;
-		if (pkt->u.request.stub_and_verifier.length < trail_len) {
-			return NT_STATUS_INFO_LENGTH_MISMATCH;
-		}
-		pkt->u.request.stub_and_verifier.length -= trail_len;
-	}
-
 	return NT_STATUS_OK;
 }