On Fri, Feb 20, 2009 at 08:53:38PM +0530, Balbir Singh wrote:
> 
> Impact: Bug fix causes incorrect chown
> 
> From: Balbir Singh <[email protected]>
> 
> This patch fixes a potential security issue, we free path and add
> reallocate it using asprintf, but that breaks chown, since that relies on
> fts_path[0] and path to point to the same address location.
> 
> Please review, comment.
> 
> Signed-off-by: Balbir Singh <[email protected]>
> ---
> 
>  api.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/api.c b/api.c
> index 91b26e0..3799f45 100644
> --- a/api.c
> +++ b/api.c
> @@ -1211,17 +1211,20 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int 
> ignore_ownership)
>                       goto err;
>               }
> 
> -             if (!ignore_ownership)
> +             if (!ignore_ownership) {
> +                     dbg("Changing ownership of %s\n", fts_path[0]);
>                       error = cg_chown_recursive(fts_path,
>                               cgroup->control_uid, cgroup->control_gid);
> +             }
> 
>               if (error)
>                       goto err;
> 
>               for (j = 0; j < cgroup->controller[k]->index; j++) {
> -                     free(path);
> -                     ret = asprintf(&path, "%s%s", base,
> +                     ret = snprintf(path, FILENAME_MAX, "%s%s", base,
>                                       cgroup->controller[k]->values[j]->name);
> +                     dbg("setting %s to %s, error %d\n", path,
> +                             cgroup->controller[k]->values[j]->name, ret);
>                       if (ret < 0) {
>                               last_errno = errno;
>                               error = ECGOTHER;
> @@ -1245,8 +1248,7 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int 
> ignore_ownership)
>               }
> 
>               if (!ignore_ownership) {
> -                     free(path);
> -                     ret = asprintf(&path, "%s/tasks", base);
> +                     ret = snprintf(path, FILENAME_MAX, "%s/tasks", base);
>                       if (ret < 0) {

This also needs one more check. I will fix it up before merging.

Thanks balbir

>                               last_errno = errno;
>                               error = ECGOTHER;
> 
> -- 
>       Balbir

-- 
regards,
Dhaval

------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
_______________________________________________
Libcg-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libcg-devel

Reply via email to