lkml.org 
[lkml]   [2012]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 7/7] AppArmor: Add the ability to mediate mount
From
Date
Only checking "[PATCH 7/7] AppArmor: Add the ability to mediate mount".



John Johansen wrote:
> --- a/security/apparmor/include/apparmor.h
> +++ b/security/apparmor/include/apparmor.h
> @@ -29,8 +29,9 @@
> #define AA_CLASS_NET 4
> #define AA_CLASS_RLIMITS 5
> #define AA_CLASS_DOMAIN 6
> +#define AA_CLASS_MOUNT 7
>
> -#define AA_CLASS_LAST AA_CLASS_DOMAIN
> +#define AA_CLASS_LAST AA_CLASS_MOUNT

Maybe use of enum is easier to maintain?



> --- /dev/null
> +++ b/security/apparmor/mount.c
> +static void audit_mnt_flags(struct audit_buffer *ab, unsigned long flags)
> +{
(...snipped...)
> + if (flags & MS_UNBINDABLE)
> + audit_log_format(ab, flags & MS_REC ? ", runbindable" :
> + ", unbindable");
> + if (flags & MS_PRIVATE)
> + audit_log_format(ab, flags & MS_REC ? ", rprivate" :
> + ", private");
> + if (flags & MS_UNBINDABLE)
> + audit_log_format(ab, flags & MS_REC ? ", rslave" :
> + ", slave");

MS_UNBINDABLE -> MS_SLAVE

> + if (flags & MS_UNBINDABLE)
> + audit_log_format(ab, flags & MS_REC ? ", rshared" :
> + ", shared");

MS_UNBINDABLE -> MS_SHARED

(...snipped...)
> +}



> +/**
> + * mount_audit_cb - call back for mount specific audit fields
> + * @ab: audit_buffer (NOT NULL)
> + * @va: audit struct to audit values of (NOT NULL)
> + */
> +static void mount_audit_cb(struct audit_buffer *ab, void *va)
> +{
> + struct common_audit_data *sa = va;
(...snipped...)
> + if (sa->aad.mnt.data) {
> + audit_log_format(ab, " options=");
> + audit_log_untrustedstring(ab, sa->aad.mnt.data);
> + }

data might be binary. Also, there is no guarantee that
memchr(data, '\0', PAGE_SIZE) != NULL even if data is not binary.
I think auditing this argument should be avoided.

> +}



> +/**
> + * aa_audit_file - handle the auditing of file operations
> + * @profile: the profile being enforced (NOT NULL)
> + * @gfp: allocation flags
> + * @op: operation being mediated
> + * @name: name of object being mediated (MAYBE NULL)
> + * @src_name: src_name of object being mediated (MAYBE_NULL)
> + * @type: type of filesystem

aa_remount() passes @type == NULL.
Maybe use of gcc's __attribute__((nonnull(...))) helps catching this kind of
error.

> + * @trans: name of trans (MAYBE NULL)
> + * @flags: filesystem idependent mount flags
> + * @data: filesystem mount flags
> + * @request: permissions requested
> + * @perms: the permissions computed for the request (NOT NULL)
> + * @info: extra information message (MAYBE NULL)
> + * @error: 0 if operation allowed else failure error code
> + *
> + * Returns: %0 or error on failure
> + */



> +int aa_remount(struct aa_profile *profile, struct path *path,
> + unsigned long flags, void *data)
> +{
> + struct file_perms perms;

Please explicitly initialize perms, or

> + const char *name, *info = NULL;
> + char *buffer = NULL;
> + int binarydata, error;
> +
> + binarydata = path->dentry->d_sb->s_type->fs_flags & FS_BINARY_MOUNTDATA;
> +
> + error = aa_path_name(path, path_flags(profile, path), &buffer, &name,
> + &info);
> + if (error)
> + goto audit;
> +

this error path will call aa_audit_mount() with perms uninitialized.



> +int aa_mount_change_type(struct aa_profile *profile, struct path *path,
> + unsigned long flags)
> +{
> + struct file_perms perms = { };
> + char *buffer = NULL;
> + const char *name, *info = NULL;
> + int error;
> +
> + flags &= (MS_REC | MS_SILENT | MS_SHARED | MS_PRIVATE | MS_SLAVE |
> + MS_UNBINDABLE);

Do you really want to drop these flags from audit_mnt_flags()?



> +int aa_move_mount(struct aa_profile *profile, struct path *path,
> + const char *orig_name)
> +{
> + struct file_perms perms = { };
> + char *buffer = NULL, *old_buffer = NULL;
> + const char *name, *old_name, *info = NULL;

Please explicitly initialize old_name with NULL, or

> + struct path old_path;
> + int error;
> +
> + if (!orig_name || !*orig_name)
> + return -EINVAL;
> +
> + error = aa_path_name(path, path_flags(profile, path), &buffer, &name,
> + &info);
> + if (error)
> + goto audit;

this error path will call aa_audit_mount() with old_name uninitialized.



> +int aa_new_mount(struct aa_profile *profile, const char *orig_dev_name,
> + struct path *path, const char *type, unsigned long flags,
> + void *data)
> +{
> + struct file_system_type *fstype = NULL;
> + struct file_perms perms = { };
> + char *buffer = NULL, *dev_buffer = NULL;
> + const char *name, *dev_name, *info = NULL;

Please explicitly initialize dev_name with NULL, or

> + struct path dev_path;
> + int binary_data, error;
> +
> + fstype = get_fs_type(type);

get_fs_type() does not accept type == NULL but type != NULL is not checked.

> + if (!fstype) {
> + error = -ENODEV;
> + goto out;
> + }
> + binary_data = fstype->fs_flags & FS_BINARY_MOUNTDATA;
> +
> + if (fstype->fs_flags & FS_REQUIRES_DEV) {
> + if (!dev_name) {

dev_name -> orig_dev_name

kern_path() does not accept orig_dev_name == NULL.

> + error = -ENOENT;
> + goto out;
> + }
> +
> + error = kern_path(orig_dev_name, LOOKUP_FOLLOW, &dev_path);
> + if (error)
> + goto audit;
> +

this error path will call aa_audit_mount() with dev_name uninitialized.

> + error = aa_path_name(&dev_path, path_flags(profile, &dev_path),
> + &dev_buffer, &dev_name, &info);
> + path_put(&dev_path);
> + if (error)
> + goto audit;
> + } else
> + dev_name = orig_dev_name;
> +
(...snipped...)
> +}



> +int aa_pivotroot(struct aa_profile *profile, struct path *old_path,
> + struct path *new_path)
> +{
> + struct file_perms perms = { };
> + struct aa_profile *target = NULL;
> + char *old_buffer = NULL, *new_buffer = NULL;
> + const char *old_name, *new_name, *info = NULL;

Please explicitly initialize new_name with NULL, or

> + int error;
> +
> + error = aa_path_name(old_path, path_flags(profile, old_path),
> + &old_buffer, &old_name, &info);
> + if (error)
> + goto audit;

this error path will call aa_audit_mount() with new_name uninitialized.

(...snipped...)
> +}


\
 
 \ /
  Last update: 2012-02-28 03:31    [W:0.200 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site