| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201202280229.q1S2TBVA080761@www262.sakura.ne.jp>
Date: Tue, 28 Feb 2012 11:29:11 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: john.johansen@...onical.com
Cc: linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] AppArmor: Add the ability to mediate mount
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...)
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists