From: Andrew Bartlett Date: Thu, 21 Sep 2023 00:26:15 +0000 (+1200) Subject: conditional_aces: Avoid manual parsing for ace_condition_unicode X-Git-Tag: tevent-0.16.0~408 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0ac979b2cc67d178327f2171bfac40186c40c70c;p=thirdparty%2Fsamba.git conditional_aces: Avoid manual parsing for ace_condition_unicode 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 Reviewed-by: Douglas Bagnall --- diff --git a/libcli/security/claims-conversions.c b/libcli/security/claims-conversions.c index d544187102e..23f7c50524e 100644 --- a/libcli/security/claims-conversions.c +++ b/libcli/security/claims-conversions.c @@ -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; } diff --git a/libcli/security/conditional_ace.c b/libcli/security/conditional_ace.c index 92b9c9454af..7e4590a1666 100644 --- a/libcli/security/conditional_ace.c +++ b/libcli/security/conditional_ace.c @@ -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; } diff --git a/libcli/security/sddl_conditional_ace.c b/libcli/security/sddl_conditional_ace.c index c32991a092b..5c6e02e37bd 100644 --- a/libcli/security/sddl_conditional_ace.c +++ b/libcli/security/sddl_conditional_ace.c @@ -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) { diff --git a/librpc/idl/conditional_ace.idl b/librpc/idl/conditional_ace.idl index 9b52e973cc9..1b9de9ec915 100644 --- a/librpc/idl/conditional_ace.idl +++ b/librpc/idl/conditional_ace.idl @@ -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 {