[systemd-devel] [PATCH 10/12] policy: kdbus_policy_set() fix a use after free bug

[Djalal Harouni]

If we try to register the same name twice then kdbus_policy_add_one()
will fail with -EEXIST

In kdbus_policy_set() we have two calls to kdbus_policy_add_one() if
they fail we clean things up with kdbus_policy_entry_free(), but since
we failed ret == -EEXIST ,we hit the error path where we have another:

if (e)
kdbus_policy_entry_free(e);

We have a use after free bug here, Since 'e' is freed but never set to
NULL.

To fix this we can set that 'e' to NULL after each
kdbus_policy_entry_free() call, but it is better to just clean things up
in a one place, in the error path and remove the other
kdbus_policy_entry_free() calls.

Thix fixes the bug triggered by test-kdbus-policy when we try to
register the same name twice.

Signed-off-by: Djalal Harouni tix...@opendz.org
---
 policy.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/policy.c b/policy.c
index 601d2a8..d75c2ef 100644
--- a/policy.c
+++ b/policy.c
@@ -512,10 +512,8 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
 
if (e) {
ret = kdbus_policy_add_one(db, e);
-   if (ret  0) {
-   kdbus_policy_entry_free(e);
+   if (ret  0)
goto exit;
-   }
}
 
if (max_policies  ++count  max_policies) {
@@ -584,11 +582,8 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
goto exit;
}
 
-   if (e) {
+   if (e)
ret = kdbus_policy_add_one(db, e);
-   if (ret  0)
-   kdbus_policy_entry_free(e);
-   }
 
 exit:
if (ret  0) {
-- 
1.9.0

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 10/12] policy: kdbus_policy_set() fix a use after free bug

[Daniel Mack]

On 06/20/2014 06:50 PM, Djalal Harouni wrote:
 If we try to register the same name twice then kdbus_policy_add_one()
 will fail with -EEXIST
 
 In kdbus_policy_set() we have two calls to kdbus_policy_add_one() if
 they fail we clean things up with kdbus_policy_entry_free(), but since
 we failed ret == -EEXIST ,we hit the error path where we have another:
 
 if (e)
   kdbus_policy_entry_free(e);
 
 We have a use after free bug here, Since 'e' is freed but never set to
 NULL.

Applied, thanks.


Daniel


 
 To fix this we can set that 'e' to NULL after each
 kdbus_policy_entry_free() call, but it is better to just clean things up
 in a one place, in the error path and remove the other
 kdbus_policy_entry_free() calls.
 
 Thix fixes the bug triggered by test-kdbus-policy when we try to
 register the same name twice.
 
 Signed-off-by: Djalal Harouni tix...@opendz.org
 ---
  policy.c | 9 ++---
  1 file changed, 2 insertions(+), 7 deletions(-)
 
 diff --git a/policy.c b/policy.c
 index 601d2a8..d75c2ef 100644
 --- a/policy.c
 +++ b/policy.c
 @@ -512,10 +512,8 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
  
   if (e) {
   ret = kdbus_policy_add_one(db, e);
 - if (ret  0) {
 - kdbus_policy_entry_free(e);
 + if (ret  0)
   goto exit;
 - }
   }
  
   if (max_policies  ++count  max_policies) {
 @@ -584,11 +582,8 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
   goto exit;
   }
  
 - if (e) {
 + if (e)
   ret = kdbus_policy_add_one(db, e);
 - if (ret  0)
 - kdbus_policy_entry_free(e);
 - }
  
  exit:
   if (ret  0) {
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel