[PATCH] selinux: fix error handling bugs in security_load_policy()

[PATCH] selinux: fix error handling bugs in security_load_policy()

[Dan Carpenter]

There are a few bugs in the error handling for security_load_policy().

1) If the newpolicy->sidtab allocation fails then it leads to a NULL
   dereference.  Also the error code was not set to -ENOMEM on that
   path.
2) If policydb_read() failed then we call policydb_destroy() twice
   which meands we call kvfree(p->sym_val_to_name[i]) twice.
3) If policydb_load_isids() failed then we call sidtab_destroy() twice
   and that results in a double free in the sidtab_destroy_tree()
   function because entry.ptr_inner and entry.ptr_leaf are not set to
   NULL.

One thing that makes this code nice to deal with is that none of the
functions return partially allocated data.  In other words, the
policydb_read() either allocates everything successfully or it frees
all the data it allocates.  It never returns a mix of allocated and
not allocated data.

I re-wrote this to only free the successfully allocated data which
avoids the double frees.  I also re-ordered selinux_policy_free() so
it's in the reverse order of the allocation function.

Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I was wrong about context_cpy().  There is no double free in the error
handling there.  Sorry about that.

 security/selinux/ss/services.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index a48fc1b337ba..645e436cdb85 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2127,10 +2127,10 @@ static void selinux_policy_free(struct selinux_policy *policy)
 	if (!policy)
 		return;
 
-	policydb_destroy(&policy->policydb);
 	sidtab_destroy(policy->sidtab);
-	kfree(policy->sidtab);
 	kfree(policy->map.mapping);
+	policydb_destroy(&policy->policydb);
+	kfree(policy->sidtab);
 	kfree(policy);
 }
 
@@ -2224,23 +2224,25 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 		return -ENOMEM;
 
 	newpolicy->sidtab = kzalloc(sizeof(*newpolicy->sidtab), GFP_KERNEL);
-	if (!newpolicy->sidtab)
-		goto err;
+	if (!newpolicy->sidtab) {
+		rc = -ENOMEM;
+		goto err_policy;
+	}
 
 	rc = policydb_read(&newpolicy->policydb, fp);
 	if (rc)
-		goto err;
+		goto err_sidtab;
 
 	newpolicy->policydb.len = len;
 	rc = selinux_set_mapping(&newpolicy->policydb, secclass_map,
 				&newpolicy->map);
 	if (rc)
-		goto err;
+		goto err_policydb;
 
 	rc = policydb_load_isids(&newpolicy->policydb, newpolicy->sidtab);
 	if (rc) {
 		pr_err("SELinux:  unable to load the initial SIDs\n");
-		goto err;
+		goto err_mapping;
 	}
 
 
@@ -2254,7 +2256,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 	rc = security_preserve_bools(state, &newpolicy->policydb);
 	if (rc) {
 		pr_err("SELinux:  unable to preserve booleans\n");
-		goto err;
+		goto err_free_isids;
 	}
 
 	/*
@@ -2279,13 +2281,23 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 		pr_err("SELinux:  unable to convert the internal"
 			" representation of contexts in the new SID"
 			" table\n");
-		goto err;
+		goto err_free_isids;
 	}
 
 	*newpolicyp = newpolicy;
 	return 0;
-err:
-	selinux_policy_free(newpolicy);
+
+err_free_isids:
+	sidtab_destroy(newpolicy->sidtab);
+err_mapping:
+	kfree(newpolicy->map.mapping);
+err_policydb:
+	policydb_destroy(&newpolicy->policydb);
+err_sidtab:
+	kfree(newpolicy->sidtab);
+err_policy:
+	kfree(newpolicy);
+
 	return rc;
 }
 
-- 
2.28.0

Re: [PATCH] selinux: fix error handling bugs in security_load_policy()

[Stephen Smalley]

On Wed, Aug 26, 2020 at 7:32 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> There are a few bugs in the error handling for security_load_policy().
>
> 1) If the newpolicy->sidtab allocation fails then it leads to a NULL
>    dereference.  Also the error code was not set to -ENOMEM on that
>    path.
> 2) If policydb_read() failed then we call policydb_destroy() twice
>    which meands we call kvfree(p->sym_val_to_name[i]) twice.
> 3) If policydb_load_isids() failed then we call sidtab_destroy() twice
>    and that results in a double free in the sidtab_destroy_tree()
>    function because entry.ptr_inner and entry.ptr_leaf are not set to
>    NULL.
>
> One thing that makes this code nice to deal with is that none of the
> functions return partially allocated data.  In other words, the
> policydb_read() either allocates everything successfully or it frees
> all the data it allocates.  It never returns a mix of allocated and
> not allocated data.
>
> I re-wrote this to only free the successfully allocated data which
> avoids the double frees.  I also re-ordered selinux_policy_free() so
> it's in the reverse order of the allocation function.
>
> Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

I guess this wasn't against current selinux next branch?

patching file security/selinux/ss/services.c
Hunk #1 succeeded at 2145 (offset 18 lines).
Hunk #2 succeeded at 2263 (offset 39 lines).
Hunk #3 succeeded at 2303 with fuzz 1 (offset 47 lines).
Hunk #4 succeeded at 2323 (offset 42 lines).

But otherwise it looked good to me.

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Re: [PATCH] selinux: fix error handling bugs in security_load_policy()

[Paul Moore]

On Wed, Aug 26, 2020 at 8:49 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Aug 26, 2020 at 7:32 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > There are a few bugs in the error handling for security_load_policy().
> >
> > 1) If the newpolicy->sidtab allocation fails then it leads to a NULL
> >    dereference.  Also the error code was not set to -ENOMEM on that
> >    path.
> > 2) If policydb_read() failed then we call policydb_destroy() twice
> >    which meands we call kvfree(p->sym_val_to_name[i]) twice.
> > 3) If policydb_load_isids() failed then we call sidtab_destroy() twice
> >    and that results in a double free in the sidtab_destroy_tree()
> >    function because entry.ptr_inner and entry.ptr_leaf are not set to
> >    NULL.
> >
> > One thing that makes this code nice to deal with is that none of the
> > functions return partially allocated data.  In other words, the
> > policydb_read() either allocates everything successfully or it frees
> > all the data it allocates.  It never returns a mix of allocated and
> > not allocated data.
> >
> > I re-wrote this to only free the successfully allocated data which
> > avoids the double frees.  I also re-ordered selinux_policy_free() so
> > it's in the reverse order of the allocation function.
> >
> > Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> I guess this wasn't against current selinux next branch?
>
> patching file security/selinux/ss/services.c
> Hunk #1 succeeded at 2145 (offset 18 lines).
> Hunk #2 succeeded at 2263 (offset 39 lines).
> Hunk #3 succeeded at 2303 with fuzz 1 (offset 47 lines).
> Hunk #4 succeeded at 2323 (offset 42 lines).
>
> But otherwise it looked good to me.
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

My guess is that Dan was using selinux/next, just not the latest.

Anyway, the patch is now merged into selinux/next but I had to do some
manual fixes so please double check that it looks okay to you.  Thanks
everyone.

-- 
paul moore
www.paul-moore.com