conditional_aces: Avoid manual parsing for ace_condition_unicode
authorAndrew Bartlett <abartlet@samba.org>
Thu, 21 Sep 2023 00:26:15 +0000 (12:26 +1200)
committerAndrew Bartlett <abartlet@samba.org>
Tue, 26 Sep 2023 23:45:36 +0000 (23:45 +0000)
A consequence of this is that we remove the confusing "length"
from the IDL, as it was the internal UTF8 length, not a wire
value.  We use null terminated strings internally now.

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
libcli/security/claims-conversions.c
libcli/security/conditional_ace.c
libcli/security/sddl_conditional_ace.c
librpc/idl/conditional_ace.idl

index d544187102e320312b1e1bf04d4aebd00afc0770..23f7c50524e17772b7a8ed3ef96e3ab4c7d32d8e 100644 (file)
@@ -56,28 +56,14 @@ static bool claim_v1_string_to_ace_string(
        size_t offset,
        struct ace_condition_token *result)
 {
-       /*
-        * A _v1 name string is NUL-terminated, while a conditional
-        * ACE is length-deliminated. We choose to copy the \0.
-        */
-       size_t len;
-       char *s = talloc_strndup(mem_ctx,
-                                claim->values[offset].string_value,
-                                CONDITIONAL_ACE_MAX_LENGTH);
+       char *s = talloc_strdup(mem_ctx,
+                               claim->values[offset].string_value);
        if (s == NULL) {
                return false;
        }
 
-       len = talloc_get_size(s) - 1;
-       if (len >= CONDITIONAL_ACE_MAX_LENGTH) {
-               DBG_WARNING("claim has string of unexpected length %zu or more\n",
-                           len);
-               TALLOC_FREE(s);
-               return false;
-       }
        result->type = CONDITIONAL_ACE_TOKEN_UNICODE;
        result->data.unicode.value = s;
-       result->data.unicode.length = len;
        return true;
 }
 
@@ -346,9 +332,8 @@ static bool ace_string_to_claim_v1_string(TALLOC_CTX *mem_ctx,
                                          struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *claim,
                                          size_t offset)
 {
-       const char *s = talloc_strndup(mem_ctx,
-                                      tok->data.unicode.value,
-                                      tok->data.unicode.length);
+       const char *s = talloc_strdup(mem_ctx,
+                                     tok->data.unicode.value);
        if (s == NULL) {
                return false;
        }
index 92b9c9454afb1410df27e76be6239f112e2d4548..7e4590a1666619ff1506f71ed35ede4c59b3547d 100644 (file)
@@ -152,97 +152,45 @@ static ssize_t push_integer(uint8_t *data, size_t available,
 }
 
 
-static ssize_t pull_unicode(TALLOC_CTX *mem_ctx, uint8_t *data, size_t length,
-                           struct ace_condition_unicode *tok)
+static ssize_t pull_unicode(TALLOC_CTX *mem_ctx,
+                       uint8_t *data, size_t length,
+                       struct ace_condition_unicode *tok)
 {
-       char *utf8 = NULL;
-       uint8_t *utf16 = NULL;
-       size_t utf8_len;
-       uint32_t utf16_len;
-       uint32_t i;
-       bool ok;
-       if (length < 4) {
-               return -1;
-       }
-       utf16_len = PULL_LE_U32(data, 0);
-       if (utf16_len > length - 4) {
-               return -1;
-       }
-       if (utf16_len & 1) {
-               /* we need an even number of bytes */
+       ssize_t bytes_used;
+       enum ndr_err_code ndr_err;
+       DATA_BLOB v = data_blob_const(data, length);
+       struct ndr_pull *ndr = ndr_pull_init_blob(&v, mem_ctx);
+       if (ndr == NULL) {
                return -1;
        }
-       utf16 = data + 4;
-       /*
-        * The string in the ACE blob is utf-16, which we convert to
-        * utf-8 for further processing.
-        *
-        * There may be inefficencies here (FIXME, etc, if you dare),
-        * and we might prefer to keep it as utf-16 in the runtime.
-        * But maybe not.
-        */
-       for (i = 0; i < utf16_len; i += 2) {
-               /*
-                * A 0x0000 codepoint is illegal. The string is length-bound,
-                * not NUL-terminated. If we don't do this the string will be
-                * truncated at the first 0x0000, which is not terrible, but
-                * not expected, and it makes round-trip assertions
-                * impossible.
-                */
-               if (utf16[i] == 0 && utf16[i + 1] == 0) {
-                       return -1;
-               }
-       }
-
-       ok = convert_string_talloc(mem_ctx,
-                                  CH_UTF16LE, CH_UTF8,
-                                  utf16, utf16_len,
-                                  &utf8, &utf8_len);
-       if (!ok) {
+       ndr_err = ndr_pull_ace_condition_unicode(ndr, NDR_SCALARS|NDR_BUFFERS, tok);
+       if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
+               TALLOC_FREE(ndr);
                return -1;
        }
-       if (utf16_len == 0) {
-               /*
-                * This is a special case, because convert_string_talloc()
-                * will turn a length 0 string into a length 1 string
-                * containing a zero byte. This is not the same as returning
-                * the truly allocated size, counting the '\0' for all strings
-                * -- it only happens for the empty string.
-                */
-               utf8_len = 0;
-       }
-       tok->value = utf8;
-       tok->length = utf8_len;
-       return utf16_len + 4;
+       bytes_used = ndr->offset;
+       TALLOC_FREE(ndr);
+       return bytes_used;
 }
 
-static ssize_t push_unicode(uint8_t *data, size_t length,
-                           const struct ace_condition_unicode *tok)
+static ssize_t push_unicode(uint8_t *data, size_t available,
+                       const struct ace_condition_unicode *tok)
 {
-       /*
-        * The string stored in the token is utf-8, but must be
-        * converted to utf-16 in the compiled ACE.
-        */
-       bool ok;
-       size_t bytes_written;
-       uint8_t *length_goes_here = data;
-
-       if (length < 4) {
+       enum ndr_err_code ndr_err;
+       DATA_BLOB v;
+       ndr_err = ndr_push_struct_blob(&v, NULL,
+                                      tok,
+                                      (ndr_push_flags_fn_t)ndr_push_ace_condition_unicode);
+       if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
                return -1;
        }
-       length -= 4;
-       data += 4;
-
-       //XXX do we allow an empty string?
-       ok = convert_string_error(CH_UTF8, CH_UTF16LE,
-                                 tok->value, tok->length,
-                                 data, length,
-                                 &bytes_written);
-       if (! ok || bytes_written > length) {
+       if (available < v.length) {
+               talloc_free(v.data);
                return -1;
        }
-       PUSH_LE_U32(length_goes_here, 0, bytes_written);
-       return bytes_written + 4;
+       memcpy(data, v.data, v.length);
+       talloc_free(v.data);
+       return v.length;
 }
 
 
@@ -812,9 +760,9 @@ static bool resource_claim_lookup(
        name = op->data.resource_attr;
 
        if (sd->sacl == NULL) {
-               DBG_NOTICE("Resource attribute ACE '%*s' not found, "
+               DBG_NOTICE("Resource attribute ACE '%s' not found, "
                           "because there is no SACL\n",
-                          name.length, name.value);
+                          name.value);
                return true;
        }
 
@@ -825,9 +773,8 @@ static bool resource_claim_lookup(
                if (ace->type != SEC_ACE_TYPE_SYSTEM_RESOURCE_ATTRIBUTE) {
                        continue;
                }
-               if (strncasecmp_m(name.value,
-                                 ace->coda.claim.name,
-                                 name.length) != 0) {
+               if (strcasecmp_m(name.value,
+                                ace->coda.claim.name) != 0) {
                        continue;
                }
                /* this is the one */
@@ -836,8 +783,8 @@ static bool resource_claim_lookup(
                        return true;
                }
        }
-       DBG_NOTICE("Resource attribute ACE '%*s' not found.\n",
-                  name.length, name.value);
+       DBG_NOTICE("Resource attribute ACE '%s' not found.\n",
+                  name.value);
        return false;
 }
 
@@ -894,7 +841,7 @@ static bool token_claim_lookup(
                        DBG_ERR("claim %zu has no name!\n", i);
                        continue;
                }
-               if (strncasecmp_m(claims[i].name, name->value, name->length) == 0) {
+               if (strcasecmp_m(claims[i].name, name->value) == 0) {
                        /* this is the one */
                        ok = claim_lookup_internal(mem_ctx, &claims[i], result);
                        return ok;
@@ -1111,7 +1058,7 @@ static bool ternary_value(
        }
        if (arg->type == CONDITIONAL_ACE_TOKEN_UNICODE) {
                /* empty is false */
-               if (arg->data.unicode.length == 0) {
+               if (arg->data.unicode.value[0] == '\0') {
                        result->data.result.value = ACE_CONDITION_FALSE;
                } else {
                        result->data.result.value = ACE_CONDITION_TRUE;
@@ -1399,12 +1346,9 @@ static bool compare_unicode(const struct ace_condition_token *op,
         */
        uint8_t flags = lhs->flags | rhs->flags;
        if (flags & CLAIM_SECURITY_ATTRIBUTE_VALUE_CASE_SENSITIVE) {
-               *cmp = memcmp(a.value, b.value, MIN(a.length, b.length));
+               *cmp = strcmp(a.value, b.value);
        } else {
-               *cmp = strncasecmp_m(a.value, b.value, MIN(a.length, b.length));
-       }
-       if (*cmp == 0) {
-               *cmp = a.length - b.length;
+               *cmp = strcasecmp_m(a.value, b.value);
        }
        return true;
 }
index c32991a092bcb22cfbc9bb65f82b4f33458a3fc7..5c6e02e37bd28009dcdb365371837ba9c6b6da51 100644 (file)
@@ -530,20 +530,18 @@ char *debug_conditional_ace(TALLOC_CTX *mem_ctx,
                case CONDITIONAL_ACE_USER_ATTRIBUTE:
                case CONDITIONAL_ACE_RESOURCE_ATTRIBUTE:
                case CONDITIONAL_ACE_DEVICE_ATTRIBUTE:
-                       utf8_len = MIN(tok->data.unicode.length, 40);
                        snprintf(line, sizeof(line),
-                                "%s.%.*s  (any type)\n",
-                                nom, utf8_len,
+                                "%s.%s  (any type)\n",
+                                nom,
                                 tok->data.unicode.value
                                );
                        type = '?';
                        break;
 
                case CONDITIONAL_ACE_TOKEN_UNICODE:
-                       utf8_len = MIN(tok->data.unicode.length, 20);
                        snprintf(line, sizeof(line),
-                                "%s.%.*s  (any type)\n",
-                                nom, utf8_len,
+                                "%s.%s  (any type)\n",
+                                nom,
                                 tok->data.unicode.value
                                );
                        type = 'u';
@@ -720,7 +718,6 @@ static bool sddl_should_escape_utf16(uint16_t c)
 
 static bool sddl_encode_attr_name(TALLOC_CTX *mem_ctx,
                                  const char *src,
-                                 size_t src_len,
                                  char **dest,
                                  size_t *dest_len)
 {
@@ -730,6 +727,7 @@ static bool sddl_encode_attr_name(TALLOC_CTX *mem_ctx,
        char *escaped = NULL;
        size_t utf16_byte_len;
        size_t utf16_len;
+       size_t src_len = strlen(src);
        size_t escapees;
        size_t required;
        *dest = NULL;
@@ -809,7 +807,6 @@ static bool sddl_write_attr(struct sddl_write_context *ctx,
        size_t i;
        bool ok = sddl_encode_attr_name(ctx->mem_ctx,
                                        tok->data.local_attr.value,
-                                       tok->data.local_attr.length,
                                        &name, &name_len);
        if (!ok) {
                return false;
@@ -844,17 +841,14 @@ static bool sddl_write_unicode(struct sddl_write_context *ctx,
         * Apparently unicode strings have no mechanism for escapes, which is
         * nice at this point.
         *
-        * Probably we could rely on tok->data.unicode.value being
-        * nul-terminated and a talloc_asprintf().
+        * We rely on tok->data.unicode.value being
+        * nul-terminated.
         */
-       quoted = talloc_size(ctx->mem_ctx, tok->data.unicode.length + 3);
+       quoted = talloc_asprintf(ctx->mem_ctx, "\"%s\"",
+                                tok->data.unicode.value);
        if (quoted == NULL) {
                return false;
        }
-       quoted[0] = '"';
-       memcpy(quoted + 1, tok->data.unicode.value, tok->data.unicode.length);
-       quoted[tok->data.unicode.length + 1] = '"';
-       quoted[tok->data.unicode.length + 2] = '\0';
        ok = sddl_write(ctx, quoted);
        TALLOC_FREE(quoted);
        return ok;
@@ -1535,7 +1529,6 @@ static ssize_t read_attr2_string(
                return -1;
        }
 
-       dest->length = utf8_len;
        /* returning bytes consumed, not necessarily the length of token */
        return src_len;
 }
@@ -1878,7 +1871,6 @@ static bool parse_unicode(struct ace_condition_sddl_compiler_context *comp)
        comp->offset += len + 1;        /* +1 for the final quote */
        token.type = CONDITIONAL_ACE_TOKEN_UNICODE;
        token.data.unicode.value = s;
-       token.data.unicode.length = len;
 
        return write_sddl_token(comp, token);
 }
@@ -2302,7 +2294,6 @@ static bool parse_word(struct ace_condition_sddl_compiler_context *comp)
        }
        s[i] = 0;
        token.data.local_attr.value = s;
-       token.data.local_attr.length = i;
        comp->offset += i;
        return write_sddl_token(comp, token);
 }
@@ -3258,7 +3249,6 @@ char *sddl_resource_attr_from_claim(
        /* escape the claim name */
        ok = sddl_encode_attr_name(tmp_ctx,
                                   claim->name,
-                                  strlen(claim->name),
                                   &name, &name_len);
 
        if (!ok) {
index 9b52e973cc9f886d2d4ee3b3addb551e047db21c..1b9de9ec915bb2d935653f3fe21d4d4968e8adf7 100644 (file)
@@ -275,9 +275,8 @@ interface conditional_ace
                uint8 base;
        } ace_condition_int;
 
-       typedef struct {
-               [string, charset(UTF16)]uint8 *value;
-               uint32 length;
+       typedef [public] struct {
+               [flag(STR_SIZE4|STR_NOTERM|STR_BYTESIZE)] string value;
        } ace_condition_unicode;
 
        typedef [public] struct {