CVE-2015-5370: s3:librpc/rpc: remove auth trailer and possible padding within dcerpc_...

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>

diff --git a/source3/librpc/rpc/dcerpc.h b/source3/librpc/rpc/dcerpc.h
index e7d66b7252b5754477e075a6615f6503ca40f030..0a82e7eea8fe110e611c3c65e8580f1a76520b99 100644 (file)
--- 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 96074a4705c9b13ca7cbb76b193fb3ec955c2ede..bb1da467ccc55766b47c52c25563363b557c4fdb 100644 (file)
--- 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 6e0a7eada4c93c8aa6364b8a0a5c6f43b98bf054..0fb848f2b4cf5f62d5971e5b783b32dff3782af8 100644 (file)
--- 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 e6e39df3eb33809e1f210e9100c1d626f478e3db..e5c7063a14863f0e6864dda6bf9ff972324a6c12 100644 (file)
--- 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;
 }