dsdb: Always store and return the userParameters as a array of LE 16-bit values

This is not allowed to be odd length, as otherwise we can not send it over the SAMR transport correctly.

Allocating one byte less memory than required causes malloc() heap corruption
and then a crash or lockup of the SAMR server.

Andrew Bartlett

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10130
Change-Id: I5c0c531c1d660141e07f884a4789ebe11c1716f6
Pair-Programmed-With: Stefan Metzmacher <metze@samba.org>
Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Stefan Metzmacher <metze@samba.org>
(cherry picked from commit d7b4d10aba90f4a1acf01d1d5ab62161862f62f7)

diff --git a/source3/passdb/pdb_samba_dsdb.c b/source3/passdb/pdb_samba_dsdb.c
index cbeb3327686980f55e4515cf97cc9c7aba177a3d..4cd7a4b6cdcabf305e09dde852030e1e3a542e1e 100644 (file)
--- a/source3/passdb/pdb_samba_dsdb.c
+++ b/source3/passdb/pdb_samba_dsdb.c
@@ -259,9 +259,13 @@ static NTSTATUS pdb_samba_dsdb_init_sam_from_priv(struct pdb_methods *m,
                pdb_set_workstations(sam, str, PDB_SET);
        }
 
-       str = ldb_msg_find_attr_as_string(msg, "userParameters",
-                                           NULL);
-       if (str != NULL) {
+       blob = ldb_msg_find_ldb_val(msg, "userParameters");
+       if (blob != NULL) {
+               str = base64_encode_data_blob(frame, *blob);
+               if (str == NULL) {
+                       DEBUG(0, ("base64_encode_data_blob() failed\n"));
+                       goto fail;
+               }
                pdb_set_munged_dial(sam, str, PDB_SET);
        }
 
@@ -553,8 +557,25 @@ static int pdb_samba_dsdb_replace_by_sam(struct pdb_samba_dsdb_state *state,
 
        /* This will need work, it is actually a UTF8 'string' with internal NULLs, to handle TS parameters */
        if (need_update(sam, PDB_MUNGEDDIAL)) {
-               ret |= ldb_msg_add_string(msg, "userParameters",
-                                         pdb_get_munged_dial(sam));
+               const char *base64_munged_dial = NULL;
+
+               base64_munged_dial = pdb_get_munged_dial(sam);
+               if (base64_munged_dial != NULL && strlen(base64_munged_dial) > 0) {
+                       struct ldb_val blob;
+
+                       blob = base64_decode_data_blob_talloc(msg,
+                                                       base64_munged_dial);
+                       if (blob.data == NULL) {
+                               DEBUG(0, ("Failed to decode userParameters from "
+                                         "munged dialback string[%s] for %s\n",
+                                         base64_munged_dial,
+                                         ldb_dn_get_linearized(msg->dn)));
+                               talloc_free(frame);
+                               return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX;
+                       }
+                       ret |= ldb_msg_add_steal_value(msg, "userParameters",
+                                                      &blob);
+               }
        }
 
        if (need_update(sam, PDB_COUNTRY_CODE)) {

diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index 904ca1dcc9aac7bd886722ff6e4d67172d7aaf44..0807e899e61e3e650eab934abbd63de40dcf0575 100644 (file)
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -650,27 +650,42 @@ uint32_t samdb_result_acct_flags(struct ldb_context *sam_ctx, TALLOC_CTX *mem_ct
        return acct_flags;
 }
 
-struct lsa_BinaryString samdb_result_parameters(TALLOC_CTX *mem_ctx,
-                                               struct ldb_message *msg,
-                                               const char *attr)
+NTSTATUS samdb_result_parameters(TALLOC_CTX *mem_ctx,
+                                struct ldb_message *msg,
+                                const char *attr,
+                                struct lsa_BinaryString *s)
 {
-       struct lsa_BinaryString s;
+       int i;
        const struct ldb_val *val = ldb_msg_find_ldb_val(msg, attr);
 
-       ZERO_STRUCT(s);
+       ZERO_STRUCTP(s);
 
        if (!val) {
-               return s;
+               return NT_STATUS_OK;
+       }
+
+       if ((val->length % 2) != 0) {
+               /*
+                * If the on-disk data is not even in length, we know
+                * it is corrupt, and can not be safely pushed.  We
+                * would either truncate, send either a un-initilaised
+                * byte or send a forced zero byte
+                */
+               return NT_STATUS_INTERNAL_DB_CORRUPTION;
        }
 
-       s.array = talloc_array(mem_ctx, uint16_t, val->length/2);
-       if (!s.array) {
-               return s;
+       s->array = talloc_array(mem_ctx, uint16_t, val->length/2);
+       if (!s->array) {
+               return NT_STATUS_NO_MEMORY;
        }
-       s.length = s.size = val->length;
-       memcpy(s.array, val->data, val->length);
+       s->length = s->size = val->length;
 
-       return s;
+       /* The on-disk format is the 'network' format, being UTF16LE (sort of) */
+       for (i = 0; i < s->length / 2; i++) {
+               s->array[i] = SVAL(val->data, i * 2);
+       }
+
+       return NT_STATUS_OK;
 }
 
 /* Find an attribute, with a particular value */
@@ -978,10 +993,26 @@ int samdb_msg_add_logon_hours(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx,
 int samdb_msg_add_parameters(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
                             const char *attr_name, struct lsa_BinaryString *parameters)
 {
+       int i;
        struct ldb_val val;
+       if ((parameters->length % 2) != 0) {
+               return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX;
+       }
+
+       val.data = talloc_array(mem_ctx, uint8_t, parameters->length);
+       if (val.data == NULL) {
+               return LDB_ERR_OPERATIONS_ERROR;
+       }
        val.length = parameters->length;
-       val.data = (uint8_t *)parameters->array;
-       return ldb_msg_add_value(msg, attr_name, &val, NULL);
+       for (i = 0; i < parameters->length / 2; i++) {
+               /*
+                * The on-disk format needs to be in the 'network'
+                * format, parmeters->array is a uint16_t array of
+                * length parameters->length / 2
+                */
+               SSVAL(val.data, i * 2, parameters->array[i]);
+       }
+       return ldb_msg_add_steal_value(msg, attr_name, &val);
 }
 
 /*

diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c
index 7279fe02f724f1ff6defa3a749b3cbcbe67c260a..330e6fbccf6b3010e9c63287944633a33c80f566 100644 (file)
--- a/source4/rpc_server/samr/dcesrv_samr.c
+++ b/source4/rpc_server/samr/dcesrv_samr.c
@@ -61,8 +61,6 @@
        info->field = samdb_result_logon_hours(mem_ctx, msg, attr);
 #define QUERY_AFLAGS(msg, field, attr) \
        info->field = samdb_result_acct_flags(sam_ctx, mem_ctx, msg, a_state->domain_state->domain_dn);
-#define QUERY_PARAMETERS(msg, field, attr) \
-       info->field = samdb_result_parameters(mem_ctx, msg, attr);
 
 
 /* these are used to make the Set[User|Group]Info code easier to follow */
@@ -2703,6 +2701,8 @@ static NTSTATUS dcesrv_samr_QueryUserInfo(struct dcesrv_call_state *dce_call, TA
        const char * const *attrs = NULL;
        union samr_UserInfo *info;
 
+       NTSTATUS status;
+
        *r->out.info = NULL;
 
        DCESRV_PULL_HANDLE(h, r->in.user_handle, SAMR_HANDLE_USER);
@@ -3043,7 +3043,11 @@ static NTSTATUS dcesrv_samr_QueryUserInfo(struct dcesrv_call_state *dce_call, TA
                break;
 
        case 20:
-               QUERY_PARAMETERS(msg, info20.parameters,    "userParameters");
+               status = samdb_result_parameters(mem_ctx, msg, "userParameters", &info->info20.parameters);
+               if (!NT_STATUS_IS_OK(status)) {
+                       talloc_free(info);
+                       return status;
+               }
                break;
 
        case 21:
@@ -3062,7 +3066,12 @@ static NTSTATUS dcesrv_samr_QueryUserInfo(struct dcesrv_call_state *dce_call, TA
                QUERY_STRING(msg, info21.description,          "description");
                QUERY_STRING(msg, info21.workstations,         "userWorkstations");
                QUERY_STRING(msg, info21.comment,              "comment");
-               QUERY_PARAMETERS(msg, info21.parameters,       "userParameters");
+               status = samdb_result_parameters(mem_ctx, msg, "userParameters", &info->info21.parameters);
+               if (!NT_STATUS_IS_OK(status)) {
+                       talloc_free(info);
+                       return status;
+               }
+
                QUERY_RID   (msg, info21.rid,                  "objectSid");
                QUERY_UINT  (msg, info21.primary_gid,          "primaryGroupID");
                QUERY_AFLAGS(msg, info21.acct_flags,           "userAccountControl");