libcli/security: Optionally disallow device‐specific attributes and operators where they are not applicable

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

lib/fuzzing/fuzz_conditional_ace_blob.c

@@ -95,6 +95,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *input, size_t len)
 	}
 
 	s2 = ace_conditions_compile_sddl(mem_ctx,
+					 ACE_CONDITION_FLAG_ALLOW_DEVICE,
 					 sddl,
 					 &message,
 					 &message_offset,

lib/fuzzing/fuzz_sddl_conditional_ace.c

@@ -57,6 +57,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *input, size_t len)
 	mem_ctx = talloc_new(NULL);
 
 	s1 = ace_conditions_compile_sddl(mem_ctx,
+					 ACE_CONDITION_FLAG_ALLOW_DEVICE,
 					 sddl_string,
 					 &message,
 					 &message_offset,
@@ -98,6 +99,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *input, size_t len)
 	}
 
 	s2 = ace_conditions_compile_sddl(mem_ctx,
+					 ACE_CONDITION_FLAG_ALLOW_DEVICE,
 					 resddl,
 					 &message,
 					 &message_offset,

libcli/security/conditional_ace.h

@@ -46,6 +46,7 @@ bool conditional_ace_encode_binary(TALLOC_CTX *mem_ctx,
 				   DATA_BLOB *dest);
 
 struct ace_condition_script * ace_conditions_compile_sddl(TALLOC_CTX *mem_ctx,
+							  const enum ace_condition_flags ace_condition_flags,
 							  const char *sddl,
 							  const char **message,
 							  size_t *message_offset,

libcli/security/sddl.c

@@ -487,6 +487,7 @@ static bool sddl_decode_guid(const char *str, struct GUID *guid)
 
 
 static DATA_BLOB sddl_decode_conditions(TALLOC_CTX *mem_ctx,
+					const enum ace_condition_flags ace_condition_flags,
 					const char *conditions,
 					size_t *length,
 					const char **msg,
@@ -495,6 +496,7 @@ static DATA_BLOB sddl_decode_conditions(TALLOC_CTX *mem_ctx,
 	DATA_BLOB blob = {0};
 	struct ace_condition_script *script = NULL;
 	script = ace_conditions_compile_sddl(mem_ctx,
+					     ace_condition_flags,
 					     conditions,
 					     msg,
 					     msg_offset,
@@ -518,6 +520,7 @@ static DATA_BLOB sddl_decode_conditions(TALLOC_CTX *mem_ctx,
 */
 static bool sddl_decode_ace(TALLOC_CTX *mem_ctx,
 			    struct security_ace *ace,
+			    const enum ace_condition_flags ace_condition_flags,
 			    char **sddl_copy,
 			    struct sddl_transition_state *state,
 			    const char **msg, size_t *msg_offset)
@@ -671,7 +674,12 @@ static bool sddl_decode_ace(TALLOC_CTX *mem_ctx,
 		DATA_BLOB conditions = {0};
 		s = tok[6];
 
-		conditions = sddl_decode_conditions(mem_ctx, s, &length, msg, msg_offset);
+		conditions = sddl_decode_conditions(mem_ctx,
+						    ace_condition_flags,
+						    s,
+						    &length,
+						    msg,
+						    msg_offset);
 		if (conditions.data == NULL) {
 			DBG_WARNING("Conditional ACE compilation failure at %zu: %s\n",
 				    *msg_offset, *msg);
@@ -733,6 +741,7 @@ static const struct flag_map acl_flags[] = {
   decode an ACL
 */
 static struct security_acl *sddl_decode_acl(struct security_descriptor *sd,
+					    const enum ace_condition_flags ace_condition_flags,
 					    const char **sddlp, uint32_t *flags,
 					    struct sddl_transition_state *state,
 					    const char **msg, size_t *msg_offset)
@@ -795,6 +804,7 @@ static struct security_acl *sddl_decode_acl(struct security_descriptor *sd,
 			return NULL;
 		}
 		ok = sddl_decode_ace(acl->aces, &acl->aces[acl->num_aces],
+				     ace_condition_flags,
 				     &sddl_copy, state, msg, msg_offset);
 		if (!ok) {
 			*msg_offset += sddl_copy - aces_start;
@@ -818,6 +828,7 @@ static struct security_acl *sddl_decode_acl(struct security_descriptor *sd,
  */
 struct security_descriptor *sddl_decode_err_msg(TALLOC_CTX *mem_ctx, const char *sddl,
 						const struct dom_sid *domain_sid,
+						const enum ace_condition_flags ace_condition_flags,
 						const char **msg, size_t *msg_offset)
 {
 	struct sddl_transition_state state = {
@@ -857,13 +868,13 @@ struct security_descriptor *sddl_decode_err_msg(TALLOC_CTX *mem_ctx, const char
 		switch (c) {
 		case 'D':
 			if (sd->dacl != NULL) goto failed;
-			sd->dacl = sddl_decode_acl(sd, &sddl, &flags, &state, msg, msg_offset);
+			sd->dacl = sddl_decode_acl(sd, ace_condition_flags, &sddl, &flags, &state, msg, msg_offset);
 			if (sd->dacl == NULL) goto failed;
 			sd->type |= flags | SEC_DESC_DACL_PRESENT;
 			break;
 		case 'S':
 			if (sd->sacl != NULL) goto failed;
-			sd->sacl = sddl_decode_acl(sd, &sddl, &flags, &state, msg, msg_offset);
+			sd->sacl = sddl_decode_acl(sd, ace_condition_flags, &sddl, &flags, &state, msg, msg_offset);
 			if (sd->sacl == NULL) goto failed;
 			/* this relies on the SEC_DESC_SACL_* flags being
 			   1 bit shifted from the SEC_DESC_DACL_* flags */
@@ -909,8 +920,12 @@ struct security_descriptor *sddl_decode(TALLOC_CTX *mem_ctx, const char *sddl,
 {
 	const char *msg = NULL;
 	size_t msg_offset = 0;
-	struct security_descriptor *sd = sddl_decode_err_msg(mem_ctx, sddl, domain_sid,
+	struct security_descriptor *sd = sddl_decode_err_msg(mem_ctx,
-							     &msg, &msg_offset);
+							     sddl,
+							     domain_sid,
+							     ACE_CONDITION_FLAG_ALLOW_DEVICE,
+							     &msg,
+							     &msg_offset);
 	DBG_NOTICE("could not decode '%s'\n", sddl);
 	if (msg != NULL) {
 		DBG_NOTICE("                  %*c\n", (int)msg_offset, '^');

libcli/security/sddl.h

@@ -24,12 +24,14 @@
 #include <talloc.h>
 #include "lib/util/data_blob.h"
 
+#include "librpc/gen_ndr/conditional_ace.h"
 #include "librpc/gen_ndr/security.h"
 
 struct security_descriptor *sddl_decode(TALLOC_CTX *mem_ctx, const char *sddl,
 					const struct dom_sid *domain_sid);
 struct security_descriptor *sddl_decode_err_msg(TALLOC_CTX *mem_ctx, const char *sddl,
 						const struct dom_sid *domain_sid,
+						const enum ace_condition_flags ace_condition_flags,
 						const char **msg, size_t *msg_offset);
 char *sddl_encode(TALLOC_CTX *mem_ctx, const struct security_descriptor *sd,
 		  const struct dom_sid *domain_sid);

libcli/security/sddl_conditional_ace.c

@@ -40,6 +40,8 @@
 #define SDDL_FLAG_EXPECTING_PAREN_LITERAL   128
 #define SDDL_FLAG_NOT_EXPECTING_END_PAREN   256
 
+#define SDDL_FLAG_DEVICE                    512
+
 #define SDDL_FLAG_IS_UNARY_OP               (1 << 20)
 #define SDDL_FLAG_IS_BINARY_OP              (1 << 21)
 
@@ -114,6 +116,7 @@ struct ace_condition_sddl_compiler_context {
 	struct dom_sid *domain_sid;
 	uint32_t state;
 	uint8_t last_token_type;
+	bool allow_device;
 };
 
 struct sddl_data {
@@ -133,7 +136,7 @@ static const struct sddl_data sddl_strings[256] = {
 	},
 	[CONDITIONAL_ACE_TOKEN_DEVICE_MEMBER_OF] = {
 		"Device_Member_of",
-		SDDL_FLAGS_MEMBER_OP,
+		SDDL_FLAGS_MEMBER_OP|SDDL_FLAG_DEVICE,
 		SDDL_PRECEDENCE_COMMON,
 		1
 	},
@@ -146,7 +149,7 @@ static const struct sddl_data sddl_strings[256] = {
 	},
 	[CONDITIONAL_ACE_TOKEN_DEVICE_MEMBER_OF_ANY] = {
 		"Device_Member_of_Any",
-		SDDL_FLAGS_MEMBER_OP,
+		SDDL_FLAGS_MEMBER_OP|SDDL_FLAG_DEVICE,
 		SDDL_PRECEDENCE_COMMON,
 		1
 	},
@@ -158,7 +161,7 @@ static const struct sddl_data sddl_strings[256] = {
 	},
 	[CONDITIONAL_ACE_TOKEN_NOT_DEVICE_MEMBER_OF] = {
 		"Not_Device_Member_of",
-		SDDL_FLAGS_MEMBER_OP,
+		SDDL_FLAGS_MEMBER_OP|SDDL_FLAG_DEVICE,
 		SDDL_PRECEDENCE_COMMON,
 		1
 	},
@@ -170,7 +173,7 @@ static const struct sddl_data sddl_strings[256] = {
 	},
 	[CONDITIONAL_ACE_TOKEN_NOT_DEVICE_MEMBER_OF_ANY] = {
 		"Not_Device_Member_of_Any",
-		SDDL_FLAGS_MEMBER_OP,
+		SDDL_FLAGS_MEMBER_OP|SDDL_FLAG_DEVICE,
 		SDDL_PRECEDENCE_COMMON,
 		1
 	},
@@ -356,7 +359,7 @@ static const struct sddl_data sddl_strings[256] = {
 	},
 	[CONDITIONAL_ACE_DEVICE_ATTRIBUTE] = {
 		"device attribute",
-		SDDL_FLAGS_ATTRIBUTE,
+		SDDL_FLAGS_ATTRIBUTE|SDDL_FLAG_DEVICE,
 		SDDL_NOT_AN_OP,
 		0
 	},
@@ -2256,6 +2259,20 @@ static bool parse_word(struct ace_condition_sddl_compiler_context *comp)
 			size_t o = candidates[j];
 			if (sddl_strings[o].name[i] == '\0') {
 				/* it is this one */
+
+				if (!comp->allow_device &&
+				    (sddl_strings[o].flags & SDDL_FLAG_DEVICE))
+				{
+					comp_error(
+						comp,
+						"a device‐relative expression "
+						"will never evaluate to true "
+						"in this context (did you "
+						"intend a user‐relative "
+						"expression?)");
+					return false;
+				}
+
 				token.type = o;
 				token.data.sddl_op.start = comp->offset;
 				comp->offset += i;
@@ -2327,7 +2344,19 @@ static bool parse_attr2(struct ace_condition_sddl_compiler_context *comp)
 				  (const char *) (comp->sddl + comp->offset),
 				  attr_len);
 		if (ret == 0) {
-			token.type = sddl_attr_types[i].code;
+			const uint8_t code = sddl_attr_types[i].code;
+
+			if (!comp->allow_device &&
+			    (sddl_strings[code].flags & SDDL_FLAG_DEVICE))
+			{
+				comp_error(comp,
+					   "a device attribute is not "
+					   "applicable in this context (did "
+					   "you intend a user attribute?)");
+				return false;
+			}
+
+			token.type = code;
 			comp->offset += attr_len;
 			break;
 		}
@@ -2676,6 +2705,7 @@ static bool parse_expression(struct ace_condition_sddl_compiler_context *comp)
 static bool init_compiler_context(
 	TALLOC_CTX *mem_ctx,
 	struct ace_condition_sddl_compiler_context *comp,
+	const enum ace_condition_flags ace_condition_flags,
 	const char *sddl,
 	size_t max_length,
 	size_t max_stack)
@@ -2713,6 +2743,7 @@ static bool init_compiler_context(
 	comp->target_len = &program->length;
 	comp->length = strlen(sddl);
 	comp->state =  SDDL_FLAG_EXPECTING_PAREN;
+	comp->allow_device = ace_condition_flags & ACE_CONDITION_FLAG_ALLOW_DEVICE;
 	return true;
 }
 
@@ -2721,6 +2752,7 @@ static bool init_compiler_context(
  *
  * @param mem_ctx
  * @param sddl - the string to be parsed
+ * @param ace_condition_flags - flags controlling compiler behaviour
  * @param message - on error, a pointer to a compiler message
  * @param message_offset - where the error occurred
  * @param consumed_length - how much of the SDDL was used
@@ -2728,6 +2760,7 @@ static bool init_compiler_context(
  */
 struct ace_condition_script * ace_conditions_compile_sddl(
 	TALLOC_CTX *mem_ctx,
+	const enum ace_condition_flags ace_condition_flags,
 	const char *sddl,
 	const char **message,
 	size_t *message_offset,
@@ -2741,6 +2774,7 @@ struct ace_condition_script * ace_conditions_compile_sddl(
 
 	ok = init_compiler_context(mem_ctx,
 				   &comp,
+				   ace_condition_flags,
 				   sddl,
 				   CONDITIONAL_ACE_MAX_LENGTH,
 				   CONDITIONAL_ACE_MAX_TOKENS);
@@ -3026,7 +3060,12 @@ struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *sddl_decode_resource_attr (
 	size_t len;
 	struct ace_condition_unicode attr_name = {};
 
-	ok = init_compiler_context(mem_ctx, &comp, str, 3, 3);
+	ok = init_compiler_context(mem_ctx,
+				   &comp,
+				   ACE_CONDITION_FLAG_ALLOW_DEVICE,
+				   str,
+				   3,
+				   3);
 	if (!ok) {
 		return NULL;
 	}
@@ -3302,7 +3341,12 @@ struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *parse_sddl_literal_as_claim(
 	struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *claim = NULL;
 	struct ace_condition_sddl_compiler_context comp = {};
 
-	ok = init_compiler_context(mem_ctx, &comp, str, 2, 2);
+	ok = init_compiler_context(mem_ctx,
+				   &comp,
+				   ACE_CONDITION_FLAG_ALLOW_DEVICE,
+				   str,
+				   2,
+				   2);
 	if (!ok) {
 		return NULL;
 	}

libcli/security/tests/test_sddl_conditional_ace.c

@@ -94,8 +94,12 @@ static void test_sddl_compile(void **state)
 	DATA_BLOB compiled;
 	size_t length;
 
-	s = ace_conditions_compile_sddl(mem_ctx, sddl, &message,
+	s = ace_conditions_compile_sddl(mem_ctx,
-					&message_offset, &length);
+					ACE_CONDITION_FLAG_ALLOW_DEVICE,
+					sddl,
+					&message,
+					&message_offset,
+					&length);
 	if (message != NULL) {
 		print_error_message(sddl, message, message_offset);
 	}
@@ -130,8 +134,12 @@ static void test_sddl_compile2(void **state)
 	DATA_BLOB compiled;
 	size_t length;
 
-	s = ace_conditions_compile_sddl(mem_ctx, sddl, &message,
+	s = ace_conditions_compile_sddl(mem_ctx,
-					&message_offset, &length);
+					ACE_CONDITION_FLAG_ALLOW_DEVICE,
+					sddl,
+					&message,
+					&message_offset,
+					&length);
 	if (message != NULL) {
 		print_error_message(sddl, message, message_offset);
 	}
@@ -624,6 +632,7 @@ static void test_round_trips(void **state)
 		DATA_BLOB e1, e2, e3;
 		fputs("=======================\n", stderr);
 		s1 = ace_conditions_compile_sddl(mem_ctx,
+						 ACE_CONDITION_FLAG_ALLOW_DEVICE,
 						 sddl[i],
 						 &message,
 						 &message_offset,
@@ -679,6 +688,7 @@ static void test_round_trips(void **state)
 		}
 		print_message("SDDL: %s\n", resddl1);
 		s3 = ace_conditions_compile_sddl(mem_ctx,
+						 ACE_CONDITION_FLAG_ALLOW_DEVICE,
 						 resddl1,
 						 &message,
 						 &message_offset,
@@ -728,6 +738,7 @@ static void test_a_number_of_valid_strings(void **state)
 		size_t message_offset;
 
 		s = ace_conditions_compile_sddl(mem_ctx,
+						ACE_CONDITION_FLAG_ALLOW_DEVICE,
 						sddl[i],
 						&message,
 						&message_offset,
@@ -803,6 +814,7 @@ static void test_a_number_of_invalid_strings(void **state)
 		const char *message = NULL;
 		size_t message_offset;
 		s = ace_conditions_compile_sddl(mem_ctx,
+						ACE_CONDITION_FLAG_ALLOW_DEVICE,
 						sddl[i],
 						&message,
 						&message_offset,
@@ -847,6 +859,7 @@ static void test_valid_strings_with_trailing_crap(void **state)
 		const char *message = NULL;
 		size_t message_offset;
 		s = ace_conditions_compile_sddl(mem_ctx,
+						ACE_CONDITION_FLAG_ALLOW_DEVICE,
 						pairs[i].sddl,
 						&message,
 						&message_offset,

librpc/idl/conditional_ace.idl

@@ -389,6 +389,10 @@ interface conditional_ace
 		uint32 length;
 	} ace_condition_script;
 
+	typedef enum {
+		ACE_CONDITION_FLAG_ALLOW_DEVICE = 0x01
+	} ace_condition_flags;
+
 	/*
 	 * Flags for ace_condition_token.flags field.
 	 *

source4/librpc/ndr/py_security.c

@@ -299,6 +299,7 @@ static PyObject *py_descriptor_from_sddl(PyObject *self, PyObject *args)
 	}
 
 	secdesc = sddl_decode_err_msg(tmp_ctx, sddl, sid,
+				      ACE_CONDITION_FLAG_ALLOW_DEVICE,
 				      &err_msg, &err_msg_offset);
 	if (secdesc == NULL) {
 		PyObject *exc = NULL;