On Thu, Nov 9, 2023 at 8:51 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> Use the internal helper str_read() in more places while reading strings
> from a binary policy. This improves readability and helps adjusting
> future sanity checks on inputs in fewer places.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
For these four patches:
Acked-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> libsepol/src/policydb.c | 199 +++++++++-------------------------------
> 1 file changed, 41 insertions(+), 158 deletions(-)
>
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index f9537caa..f608aba4 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -2108,7 +2108,8 @@ static int common_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
>
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> + rc = str_read(&key, fp, len);
> + if (rc < 0)
> goto bad;
>
> comdatum->s.value = le32_to_cpu(buf[1]);
> @@ -2120,14 +2121,6 @@ static int common_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
> nel = le32_to_cpu(buf[3]);
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> - if (rc < 0)
> - goto bad;
> - key[len] = 0;
> -
> for (i = 0; i < nel; i++) {
> if (perm_read(p, comdatum->permissions.table, fp, comdatum->permissions.nprim))
> goto bad;
> @@ -2256,11 +2249,11 @@ static int class_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
>
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> + rc = str_read(&key, fp, len);
> + if (rc < 0)
> goto bad;
> +
> len2 = le32_to_cpu(buf[1]);
> - if (is_saturated(len2))
> - goto bad;
> cladatum->s.value = le32_to_cpu(buf[2]);
>
> if (symtab_init(&cladatum->permissions, PERM_SYMTAB_SIZE))
> @@ -2272,22 +2265,10 @@ static int class_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
>
> ncons = le32_to_cpu(buf[5]);
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> - if (rc < 0)
> - goto bad;
> - key[len] = 0;
> -
> if (len2) {
> - cladatum->comkey = malloc(len2 + 1);
> - if (!cladatum->comkey)
> - goto bad;
> - rc = next_entry(cladatum->comkey, fp, len2);
> + rc = str_read(&cladatum->comkey, fp, len2);
> if (rc < 0)
> goto bad;
> - cladatum->comkey[len2] = 0;
>
> cladatum->comdatum = hashtab_search(p->p_commons.table,
> cladatum->comkey);
> @@ -2369,21 +2350,14 @@ static int role_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
>
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> + rc = str_read(&key, fp, len);
> + if (rc < 0)
> goto bad;
>
> role->s.value = le32_to_cpu(buf[1]);
> if (policydb_has_boundary_feature(p))
> role->bounds = le32_to_cpu(buf[2]);
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> - if (rc < 0)
> - goto bad;
> - key[len] = 0;
> -
> if (ebitmap_read(&role->dominates, fp))
> goto bad;
>
> @@ -2460,9 +2434,6 @@ static int type_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
>
> len = le32_to_cpu(buf[pos]);
> - if (zero_or_saturated(len))
> - goto bad;
> -
> typdatum->s.value = le32_to_cpu(buf[++pos]);
> if (policydb_has_boundary_feature(p)) {
> uint32_t properties;
> @@ -2503,13 +2474,9 @@ static int type_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
> }
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> + rc = str_read(&key, fp, len);
> if (rc < 0)
> goto bad;
> - key[len] = 0;
>
> if (hashtab_insert(h, key, typdatum))
> goto bad;
> @@ -2681,14 +2648,8 @@ static int filename_trans_read_one_compat(policydb_t *p, struct policy_file *fp)
> if (rc < 0)
> return -1;
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - return -1;
> -
> - name = calloc(len + 1, sizeof(*name));
> - if (!name)
> - return -1;
>
> - rc = next_entry(name, fp, len);
> + rc = str_read(&name, fp, len);
> if (rc < 0)
> goto err;
>
> @@ -2766,14 +2727,8 @@ static int filename_trans_read_one(policydb_t *p, struct policy_file *fp)
> if (rc < 0)
> return -1;
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - return -1;
> -
> - name = calloc(len + 1, sizeof(*name));
> - if (!name)
> - return -1;
>
> - rc = next_entry(name, fp, len);
> + rc = str_read(&name, fp, len);
> if (rc < 0)
> goto err;
>
> @@ -2957,16 +2912,9 @@ static int ocontext_read_xen(const struct policydb_compat_info *info,
> if (rc < 0)
> return -1;
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - return -1;
> -
> - c->u.name = malloc(len + 1);
> - if (!c->u.name)
> - return -1;
> - rc = next_entry(c->u.name, fp, len);
> + rc = str_read(&c->u.name, fp, len);
> if (rc < 0)
> return -1;
> - c->u.name[len] = 0;
> if (context_read_and_validate
> (&c->context[0], p, fp))
> return -1;
> @@ -3024,15 +2972,13 @@ static int ocontext_read_selinux(const struct policydb_compat_info *info,
> if (rc < 0)
> return -1;
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len) || len > 63)
> + if (len > 63)
> return -1;
> - c->u.name = malloc(len + 1);
> - if (!c->u.name)
> - return -1;
> - rc = next_entry(c->u.name, fp, len);
> +
> + rc = str_read(&c->u.name, fp, len);
> if (rc < 0)
> return -1;
> - c->u.name[len] = 0;
> +
> if (context_read_and_validate
> (&c->context[0], p, fp))
> return -1;
> @@ -3080,13 +3026,10 @@ static int ocontext_read_selinux(const struct policydb_compat_info *info,
> if (port > UINT8_MAX || port == 0)
> return -1;
>
> - c->u.ibendport.dev_name = malloc(len + 1);
> - if (!c->u.ibendport.dev_name)
> - return -1;
> - rc = next_entry(c->u.ibendport.dev_name, fp, len);
> + rc = str_read(&c->u.ibendport.dev_name, fp, len);
> if (rc < 0)
> return -1;
> - c->u.ibendport.dev_name[len] = 0;
> +
> c->u.ibendport.port = port;
> if (context_read_and_validate
> (&c->context[0], p, fp))
> @@ -3120,15 +3063,11 @@ static int ocontext_read_selinux(const struct policydb_compat_info *info,
> return -1;
> c->v.behavior = le32_to_cpu(buf[0]);
> len = le32_to_cpu(buf[1]);
> - if (zero_or_saturated(len))
> - return -1;
> - c->u.name = malloc(len + 1);
> - if (!c->u.name)
> - return -1;
> - rc = next_entry(c->u.name, fp, len);
> +
> + rc = str_read(&c->u.name, fp, len);
> if (rc < 0)
> return -1;
> - c->u.name[len] = 0;
> +
> if (context_read_and_validate
> (&c->context[0], p, fp))
> return -1;
> @@ -3196,23 +3135,17 @@ static int genfs_read(policydb_t * p, struct policy_file *fp)
> if (rc < 0)
> goto bad;
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - goto bad;
> newgenfs = calloc(1, sizeof(genfs_t));
> if (!newgenfs)
> goto bad;
> - newgenfs->fstype = malloc(len + 1);
> - if (!newgenfs->fstype) {
> - free(newgenfs);
> - goto bad;
> - }
> - rc = next_entry(newgenfs->fstype, fp, len);
> +
> + rc = str_read(&newgenfs->fstype, fp, len);
> if (rc < 0) {
> free(newgenfs->fstype);
> free(newgenfs);
> goto bad;
> }
> - newgenfs->fstype[len] = 0;
> +
> for (genfs_p = NULL, genfs = p->genfs; genfs;
> genfs_p = genfs, genfs = genfs->next) {
> if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
> @@ -3243,16 +3176,10 @@ static int genfs_read(policydb_t * p, struct policy_file *fp)
> if (rc < 0)
> goto bad;
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - goto bad;
> - newc->u.name = malloc(len + 1);
> - if (!newc->u.name) {
> - goto bad;
> - }
> - rc = next_entry(newc->u.name, fp, len);
> + rc = str_read(&newc->u.name, fp, len);
> if (rc < 0)
> goto bad;
> - newc->u.name[len] = 0;
> +
> rc = next_entry(buf, fp, sizeof(uint32_t));
> if (rc < 0)
> goto bad;
> @@ -3344,21 +3271,14 @@ static int user_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> goto bad;
>
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> + rc = str_read(&key, fp, len);
> + if (rc < 0)
> goto bad;
>
> usrdatum->s.value = le32_to_cpu(buf[1]);
> if (policydb_has_boundary_feature(p))
> usrdatum->bounds = le32_to_cpu(buf[2]);
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> - if (rc < 0)
> - goto bad;
> - key[len] = 0;
> -
> if (p->policy_type == POLICY_KERN) {
> if (ebitmap_read(&usrdatum->roles.roles, fp))
> goto bad;
> @@ -3430,19 +3350,12 @@ static int sens_read(policydb_t * p
> goto bad;
>
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> + rc = str_read(&key, fp, len);
> + if (rc < 0)
> goto bad;
>
> levdatum->isalias = le32_to_cpu(buf[1]);
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> - if (rc < 0)
> - goto bad;
> - key[len] = 0;
> -
> levdatum->level = malloc(sizeof(mls_level_t));
> if (!levdatum->level || mls_read_level(levdatum->level, fp))
> goto bad;
> @@ -3476,20 +3389,13 @@ static int cat_read(policydb_t * p
> goto bad;
>
> len = le32_to_cpu(buf[0]);
> - if(zero_or_saturated(len))
> + rc = str_read(&key, fp, len);
> + if (rc < 0)
> goto bad;
>
> catdatum->s.value = le32_to_cpu(buf[1]);
> catdatum->isalias = le32_to_cpu(buf[2]);
>
> - key = malloc(len + 1);
> - if (!key)
> - goto bad;
> - rc = next_entry(key, fp, len);
> - if (rc < 0)
> - goto bad;
> - key[len] = 0;
> -
> if (hashtab_insert(h, key, catdatum))
> goto bad;
>
> @@ -3865,17 +3771,10 @@ static int filename_trans_rule_read(policydb_t *p, filename_trans_rule_t **r,
> return -1;
>
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - return -1;
>
> - ftr->name = malloc(len + 1);
> - if (!ftr->name)
> - return -1;
> -
> - rc = next_entry(ftr->name, fp, len);
> - if (rc)
> + rc = str_read(&ftr->name, fp, len);
> + if (rc < 0)
> return -1;
> - ftr->name[len] = 0;
>
> if (type_set_read(&ftr->stypes, fp))
> return -1;
> @@ -4119,15 +4018,10 @@ static int scope_read(policydb_t * p, int symnum, struct policy_file *fp)
> if (rc < 0)
> goto cleanup;
> key_len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(key_len))
> - goto cleanup;
> - key = malloc(key_len + 1);
> - if (!key)
> - goto cleanup;
> - rc = next_entry(key, fp, key_len);
> +
> + rc = str_read(&key, fp, key_len);
> if (rc < 0)
> goto cleanup;
> - key[key_len] = '\0';
>
> /* ensure that there already exists a symbol with this key */
> if (hashtab_search(p->symtab[symnum].table, key) == NULL) {
> @@ -4387,28 +4281,17 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
> goto bad;
> }
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - goto bad;
> - if ((p->name = malloc(len + 1)) == NULL) {
> - goto bad;
> - }
> - if ((rc = next_entry(p->name, fp, len)) < 0) {
> + rc = str_read(&p->name, fp, len);
> + if (rc < 0)
> goto bad;
> - }
> - p->name[len] = '\0';
> +
> if ((rc = next_entry(buf, fp, sizeof(uint32_t))) < 0) {
> goto bad;
> }
> len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> - goto bad;
> - if ((p->version = malloc(len + 1)) == NULL) {
> - goto bad;
> - }
> - if ((rc = next_entry(p->version, fp, len)) < 0) {
> + rc = str_read(&p->version, fp, len);
> + if (rc < 0)
> goto bad;
> - }
> - p->version[len] = '\0';
> }
>
> if ((p->policyvers >= POLICYDB_VERSION_POLCAP &&
> --
> 2.42.0
>
