r14891: fix a bug found by the ibm checker

the problem was that we shift with <<= (privilege-1)

and we called the function with privilege=0

add some checks to catch invalid privilege values
and hide the mask representation in privilege.c

metze
(This used to be commit a69f000324764bcd4cf420f2ecba1aca788258e4)

3 files changed, 52 insertions, 34 deletions

diff --git a/source4/dsdb/samdb/samdb_privilege.c b/source4/dsdb/samdb/samdb_privilege.c
index 6ca8aa7ae2..f2fc43967f 100644
--- a/source4/dsdb/samdb/samdb_privilege.c
+++ b/source4/dsdb/samdb/samdb_privilege.c
@@ -31,16 +31,14 @@
   add privilege bits for one sid to a security_token
 */
 static NTSTATUS samdb_privilege_setup_sid(void *samctx, TALLOC_CTX *mem_ctx,
-					  const struct dom_sid *sid, 
-					  uint64_t *mask)
+					  struct security_token *token,
+					  const struct dom_sid *sid)
 {
 	const char * const attrs[] = { "privilege", NULL };
 	struct ldb_message **res = NULL;
 	struct ldb_message_element *el;
 	int ret, i;
 	char *sidstr;
-	
-	*mask = 0;
 
 	sidstr = ldap_encode_ndr_dom_sid(mem_ctx, sid);
 	NT_STATUS_HAVE_NO_MEMORY(sidstr);
@@ -59,13 +57,13 @@ static NTSTATUS samdb_privilege_setup_sid(void *samctx, TALLOC_CTX *mem_ctx,
 
 	for (i=0;i<el->num_values;i++) {
 		const char *priv_str = (const char *)el->values[i].data;
-		int privilege = sec_privilege_id(priv_str);
+		enum sec_privilege privilege = sec_privilege_id(priv_str);
 		if (privilege == -1) {
 			DEBUG(1,("Unknown privilege '%s' in samdb\n",
 				 priv_str));
 			continue;
 		}
-		*mask |= sec_privilege_mask(privilege);
+		sec_privilege_set(token, privilege);
 	}
 
 	return NT_STATUS_OK;
@@ -103,14 +101,12 @@ _PUBLIC_ NTSTATUS samdb_privilege_setup(struct security_token *token)
 	token->privilege_mask = 0;
 	
 	for (i=0;i<token->num_sids;i++) {
-		uint64_t mask;
-		status = samdb_privilege_setup_sid(samctx, mem_ctx, 
-						   token->sids[i], &mask);
+		status = samdb_privilege_setup_sid(samctx, mem_ctx,
+						   token, token->sids[i]);
 		if (!NT_STATUS_IS_OK(status)) {
 			talloc_free(mem_ctx);
 			return status;
 		}
-		token->privilege_mask |= mask;
 	}
 
 	talloc_free(mem_ctx);
diff --git a/source4/libcli/security/privilege.c b/source4/libcli/security/privilege.c
index 202a418f6b..f81ff6dccc 100644
--- a/source4/libcli/security/privilege.c
+++ b/source4/libcli/security/privilege.c
@@ -130,7 +130,7 @@ static const struct {
 /*
   map a privilege id to the wire string constant
 */
-const char *sec_privilege_name(unsigned int privilege)
+const char *sec_privilege_name(enum sec_privilege privilege)
 {
 	int i;
 	for (i=0;i<ARRAY_SIZE(privilege_names);i++) {
@@ -146,9 +146,12 @@ const char *sec_privilege_name(unsigned int privilege)
   
   TODO: this should use language mappings
 */
-const char *sec_privilege_display_name(int privilege, uint16_t *language)
+const char *sec_privilege_display_name(enum sec_privilege privilege, uint16_t *language)
 {
 	int i;
+	if (privilege < 1 || privilege > 64) {
+		return NULL;
+	}
 	for (i=0;i<ARRAY_SIZE(privilege_names);i++) {
 		if (privilege_names[i].privilege == privilege) {
 			return privilege_names[i].display_name;
@@ -160,12 +163,12 @@ const char *sec_privilege_display_name(int privilege, uint16_t *language)
 /*
   map a privilege name to a privilege id. Return -1 if not found
 */
-int sec_privilege_id(const char *name)
+enum sec_privilege sec_privilege_id(const char *name)
 {
 	int i;
 	for (i=0;i<ARRAY_SIZE(privilege_names);i++) {
 		if (strcasecmp(privilege_names[i].name, name) == 0) {
-			return (int)privilege_names[i].privilege;
+			return privilege_names[i].privilege;
 		}
 	}
 	return -1;
@@ -175,9 +178,14 @@ int sec_privilege_id(const char *name)
 /*
   return a privilege mask given a privilege id
 */
-uint64_t sec_privilege_mask(unsigned int privilege)
+static uint64_t sec_privilege_mask(enum sec_privilege privilege)
 {
 	uint64_t mask = 1;
+
+	if (privilege < 1 || privilege > 64) {
+		return 0;
+	}
+
 	mask <<= (privilege-1);
 	return mask;
 }
@@ -186,9 +194,15 @@ uint64_t sec_privilege_mask(unsigned int privilege)
 /*
   return True if a security_token has a particular privilege bit set
 */
-BOOL sec_privilege_check(const struct security_token *token, unsigned int privilege)
+BOOL sec_privilege_check(const struct security_token *token, enum sec_privilege privilege)
 {
-	uint64_t mask = sec_privilege_mask(privilege);
+	uint64_t mask;
+
+	if (privilege < 1 || privilege > 64) {
+		return False;
+	}
+
+	mask = sec_privilege_mask(privilege);
 	if (token->privilege_mask & mask) {
 		return True;
 	}
@@ -198,7 +212,30 @@ BOOL sec_privilege_check(const struct security_token *token, unsigned int privil
 /*
   set a bit in the privilege mask
 */
-void sec_privilege_set(struct security_token *token, unsigned int privilege)
+void sec_privilege_set(struct security_token *token, enum sec_privilege privilege)
 {
+	if (privilege < 1 || privilege > 64) {
+		return;
+	}
 	token->privilege_mask |= sec_privilege_mask(privilege);
 }
+
+void sec_privilege_debug(int dbg_lev, const struct security_token *token)
+{
+	DEBUGADD(dbg_lev, (" Privileges (0x%16llX):\n",
+			    (unsigned long long) token->privilege_mask));
+
+	if (token->privilege_mask) {
+		int i = 0;
+		uint_t privilege;
+
+		for (privilege = 1; privilege <= 64; privilege++) {
+			uint64_t mask = sec_privilege_mask(privilege);
+
+			if (token->privilege_mask & mask) {
+				DEBUGADD(dbg_lev, ("  Privilege[%3lu]: %s\n", (unsigned long)i++, 
+					sec_privilege_name(privilege)));
+			}
+		}
+	}
+}
diff --git a/source4/libcli/security/security_token.c b/source4/libcli/security/security_token.c
index 6e380a62ad..80644e1f2d 100644
--- a/source4/libcli/security/security_token.c
+++ b/source4/libcli/security/security_token.c
@@ -128,7 +128,6 @@ void security_token_debug(int dbg_lev, const struct security_token *token)
 {
 	TALLOC_CTX *mem_ctx;
 	int i;
-	uint_t privilege;
 
 	if (!token) {
 		DEBUG(dbg_lev, ("Security token: (NULL)\n"));
@@ -149,21 +148,7 @@ void security_token_debug(int dbg_lev, const struct security_token *token)
 			   dom_sid_string(mem_ctx, token->sids[i])));
 	}
 
-	DEBUGADD(dbg_lev, (" Privileges (0x%08X%08X):\n",
-			    (uint32_t)((token->privilege_mask & 0xFFFFFFFF00000000LL) >> 32),
-			    (uint32_t)(token->privilege_mask & 0x00000000FFFFFFFFLL)));
-
-	if (token->privilege_mask) {
-		i = 0;
-		for (privilege = 0; privilege < 64; privilege++) {
-			uint64_t mask = sec_privilege_mask(privilege);
-
-			if (token->privilege_mask & mask) {
-				DEBUGADD(dbg_lev, ("  Privilege[%3lu]: %s\n", (unsigned long)i++, 
-					sec_privilege_name(privilege)));
-			}
-		}
-	}
+	sec_privilege_debug(dbg_lev, token);
 
 	talloc_free(mem_ctx);
 }