libsepol: cil: only overwrite cil_typeattribute used when false.

PATCH In review Daniel Cashman 2 participants 3 messages Nov 15, 2017 – Nov 16, 2017 SELinux security module

lore

Share & tools

lore

Daniel Cashman

author

2017-11-15 02:27 UTC #0 lore raw

PATCH 1/1 libsepol: cil: only overwrite cil_typeattribute used when false.

From: Dan Cashman <[email protected]>

When using cil_db multiple_decls, the different cil_attribute nodes all point to the same underlying cil_attribute struct. This leads to problems, though, when modifying the used value in the struct. __cil_post_db_attr() changes the value of the field to based on the output of cil_typeattribute_used(), for use later in cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but due to the multiple declarations, cil_typeattribute_used() could be called again by a second node. In this second call, the value used is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually needed. This could result in the field being reset again, to an incorrect CIL_FALSE value. To avoid this, only change the value of the field if it is to be set to CIL_FALSE. Also update the checks that use this value to explicitly check for CIL_FALSE, rather than relying on its underlying value.

Alternatively, a different field could be used rather than overwriting the 'used' field, attrib structs could be un-shared, or duplicate declarations could just be skipped rather than sticking around in the tree.

Signed-off-by Daniel Cashman <[email protected]>

---
 libsepol/cil/src/cil_binary.c | 6 +++---
 libsepol/cil/src/cil_post.c   | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

libsepol/cil/src/cil_binary.c +3 -3

index c0ca60f2..e4003c8b 100644
@@ -567,8 +567,8 @@ int cil_typeattribute_to_policydb(policydb_t *pdb, struct cil_typeattribute *cil
 	char *key = NULL;
 	type_datum_t *sepol_attr = NULL;
 
-	if (!cil_attr->used) {
-		return SEPOL_OK;		
+	if (cil_attr->used == CIL_FALSE) {
+		return SEPOL_OK;
 	}
 
 	sepol_attr = cil_malloc(sizeof(*sepol_attr));

@@ -632,7 +632,7 @@ int cil_typeattribute_to_bitmap(policydb_t *pdb, const struct cil_db *db, struct
 	ebitmap_node_t *tnode;
 	unsigned int i;
 
-	if (!cil_attr->used) {
+	if (cil_attr->used == CIL_FALSE) {
 		return SEPOL_OK;
 	}

libsepol/cil/src/cil_post.c +2 -1

index 3e013c97..823ecf0f 100644
@@ -1369,7 +1369,8 @@ static int __cil_post_db_attr_helper(struct cil_tree_node *node, uint32_t *finis
 			rc = __evaluate_type_expression(attr, db);
 			if (rc != SEPOL_OK) goto exit;
 		}
-		attr->used = cil_typeattribute_used(attr, db);
+		if (cil_typeattribute_used(attr, db) == CIL_FALSE)
+			attr->used = CIL_FALSE;
 		break;
 	}
 	case CIL_ROLEATTRIBUTE: {

Signature · 1 line

2.15.0.448.gf294e3d99a-goog

jwcart2

2017-11-16 15:13 UTC #1 lore raw

Quoted 17 lines · Daniel Cashman

From: Dan Cashman <[email protected]>
When using cil_db multiple_decls, the different cil_attribute nodes all point to the same underlying cil_attribute struct. This leads to problems, though, when modifying the used value in the struct. __cil_post_db_attr() changes the value of the field to based on the output of cil_typeattribute_used(), for use later in cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but due to the multiple declarations, cil_typeattribute_used() could be called again by a second node. In this second call, the value used is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually needed. This could result in the field being reset again, to an incorrect CIL_FALSE value. To avoid this, only change the value of the field if it is to be set to CIL_FALSE. Also update the checks that use this value to explicitly check for CIL_FALSE, rather than relying on its underlying value.

Quoted 4 lines

Alternatively, a different field could be used rather than overwriting the 'used' field, attrib structs could be un-shared, or duplicate declarations could just be skipped rather than sticking around in the tree.

You patch does work. I am wondering if it might not be better to have a new field. I am going to experiment with this today and see how that would work.

Quoted 22 lines

Signed-off-by Daniel Cashman <[email protected]>

---
  libsepol/cil/src/cil_binary.c | 6 +++---
  libsepol/cil/src/cil_post.c   | 3 ++-
  2 files changed, 5 insertions(+), 4 deletions(-)

libsepol/cil/src/cil_binary.c +2 -2

index c0ca60f2..e4003c8b 100644
@@ -567,8 +567,8 @@ int cil_typeattribute_to_policydb(policydb_t *pdb, struct cil_typeattribute *cil
  	char *key = NULL;
  	type_datum_t *sepol_attr = NULL;
 
-	if (!cil_attr->used) {
-		return SEPOL_OK;		
+	if (cil_attr->used == CIL_FALSE) {
+		return SEPOL_OK;
  	}
 
  	sepol_attr = cil_malloc(sizeof(*sepol_attr));

libsepol/cil/src/cil_binary.c int cil_typeattribute_to_bitmap(policydb_t *pdb, const struct cil_db *db, struct · line 638

Quoted 8 lines

libsepol/cil/src/cil_binary.c +1 -1

@@ -632,7 +632,7 @@ int cil_typeattribute_to_bitmap(policydb_t *pdb, const struct cil_db *db, struct
  	ebitmap_node_t *tnode;
  	unsigned int i;
 
-	if (!cil_attr->used) {
+	if (cil_attr->used == CIL_FALSE) {
  		return SEPOL_OK;
  	}

I don't think these changes are necessary. It would be a problem if we were checking specifically for CIL_TRUE because with your patch the used field can contain other non-false values.

Jim

Quoted 15 lines

libsepol/cil/src/cil_post.c +2 -1

index 3e013c97..823ecf0f 100644
@@ -1369,7 +1369,8 @@ static int __cil_post_db_attr_helper(struct cil_tree_node *node, uint32_t *finis
  			rc = __evaluate_type_expression(attr, db);
  			if (rc != SEPOL_OK) goto exit;
  		}
-		attr->used = cil_typeattribute_used(attr, db);
+		if (cil_typeattribute_used(attr, db) == CIL_FALSE)
+			attr->used = CIL_FALSE;
  		break;
  	}
  	case CIL_ROLEATTRIBUTE: {

Signature · 2 lines

James Carter <[email protected]>
National Security Agency

Dan Cashman

author

2017-11-16 16:59 UTC #2 lore raw

Quoted 48 lines · On 11/16/2017 07:13 AM, jwcart2 wrote:

Quoted 17 lines · Daniel Cashman
From: Dan Cashman <[email protected]>
When using cil_db multiple_decls, the different cil_attribute nodes all point to the same underlying cil_attribute struct. This leads to problems, though, when modifying the used value in the struct. __cil_post_db_attr() changes the value of the field to based on the output of cil_typeattribute_used(), for use later in cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but due to the multiple declarations, cil_typeattribute_used() could be called again by a second node. In this second call, the value used is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually needed. This could result in the field being reset again, to an incorrect CIL_FALSE value. To avoid this, only change the value of the field if it is to be set to CIL_FALSE. Also update the checks that use this value to explicitly check for CIL_FALSE, rather than relying on its underlying value.
Quoted 4 lines
Alternatively, a different field could be used rather than overwriting the 'used' field, attrib structs could be un-shared, or duplicate declarations could just be skipped rather than sticking around in the tree.
You patch does work. I am wondering if it might not be better to have a new field. I am going to experiment with this today and see how that would work.
Quoted 23 lines
Signed-off-by Daniel Cashman <[email protected]>
---
libsepol/cil/src/cil_binary.c | 6 +++---
libsepol/cil/src/cil_post.c | 3 ++-
  2 files changed, 5 insertions(+), 4 deletions(-)
libsepol/cil/src/cil_binary.c +0 -0
index c0ca60f2..e4003c8b 100644
@@ -567,8 +567,8 @@ int cil_typeattribute_to_policydb(policydb_t *pdb, 
struct cil_typeattribute *cil
      char *key = NULL;
      type_datum_t *sepol_attr = NULL;
-    if (!cil_attr->used) {
-        return SEPOL_OK;
+    if (cil_attr->used == CIL_FALSE) {
+        return SEPOL_OK;
      }
      sepol_attr = cil_malloc(sizeof(*sepol_attr));
libsepol/cil/src/cil_binary.c +0 -0
@@ -632,7 +632,7 @@ int cil_typeattribute_to_bitmap(policydb_t *pdb, 
const struct cil_db *db, struct
ebitmap_node_t *tnode;

Quoted 8 lines

Quoted 6 lines
      unsigned int i;
-    if (!cil_attr->used) {
+    if (cil_attr->used == CIL_FALSE) {
          return SEPOL_OK;
      }
I don't think these changes are necessary. It would be a problem if we were checking specifically for CIL_TRUE because with your patch the used field can contain other non-false values.

Yes, definitely not necessary. Just a proposed change to not always assume that CIL_FALSE is zero, but my guess is that:
1) it always will be
2) we actually want it to be
3) changing that would require huge numbers of changes scattered throughout the code base.

Perfectly happy to not include them.

Quoted 22 lines

Jim
Quoted 16 lines
libsepol/cil/src/cil_post.c +0 -0
index 3e013c97..823ecf0f 100644
@@ -1369,7 +1369,8 @@ static int __cil_post_db_attr_helper(struct 
cil_tree_node *node, uint32_t *finis
              rc = __evaluate_type_expression(attr, db); if (rc != SEPOL_OK) goto exit;
          }
-        attr->used = cil_typeattribute_used(attr, db); +        if (cil_typeattribute_used(attr, db) == CIL_FALSE) +            attr->used = CIL_FALSE;
          break;
      }
      case CIL_ROLEATTRIBUTE: {