Messages in this thread | | | Date | Mon, 27 Feb 2012 23:17:16 -0800 | From | John Johansen <> | Subject | Re: [PATCH 7/7] AppArmor: Add the ability to mediate mount |
| |
On 02/27/2012 06:29 PM, Tetsuo Handa wrote: > 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? > meh, I'm indifferent either works I don't really see enums as easier, but certainly no worse
> > >> --- /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...)
gah, color me embarrassed and a little puzzled I know I fixed that error once already. Not sure if it did get saved, was not git added or maybe something else ...
>> +} > > > >> +/** >> + * 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. > Hrmm we should only be setting it/checking it if its not binary data. But of course while I got the checking bit in there, I missed the conditional on setting.
You are right about the no null termination not being guarenteed though we should be using audit_log_n_untrustedstring, and capping at the first of null byte or data page.
Its size can be an issue and that is why its deliberately the last element that gets added. I think that we do want to audit it though because it can be quite useful.
>> +} > > > >> +/** >> + * 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. right, I did grab the type in remount from the fs, though perhaps I should have, NULL should be valid here as it just assigns it to mnt.type which is conditionally logged in mount_audit_cb, so the comment needs to updated
Really the whole audit setup for these routines needs to be reworked, to clean them up. I have been toying with a couple of ways to make this cleaner and avoid calling function with a huge list of parameters like, is being done here.
> Maybe use of gcc's __attribute__((nonnull(...))) helps catching this kind of > error. > yeah, actually that isn't a bad idea
>> + * @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 > yikes! yes. I missed this one when reworking how the permissions where handled
>> + 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()? > hrmm, it was intentional, though something I was on the fence about. Basically this is currently the set of valid flags for do_change_type() and the question was whether to have apparmor rejecting on invalid flags or do_change_type()
Of course this also means that apparmor needs to be kept in sync with what is valid flags in do_change_type(). So perhaps it is better to just have apparmor issue rejects here even if do_change_type would fail it later.
> > >> +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 > yep, thanks
>> + 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 > sigh, yep.
>> + 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. > indeed, thanks >> + 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. > right
>> + 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. > yep
>> + 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 > yep
>> + 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...) >> +}
thanks for the review Tetsuo
| |