linux-kernel - Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <73c80352-ded1-626b-0eb0-a9481165f25d@canonical.com>
Date:   Mon, 8 Apr 2019 09:58:21 -0700
From:   John Johansen <john.johansen@...onical.com>
To:     Kees Cook <keescook@...omium.org>, James Morris <jmorris@...ei.org>
Cc:     David Rheinsberg <david.rheinsberg@...il.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"

On 4/8/19 9:07 AM, Kees Cook wrote:
> Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
> state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
> since it was using the "bool" handler. After being changed to "int",
> this switched to "1" or "0", breaking the userspace AppArmor detection
> of dbus-broker. This restores the Y/N output while keeping the LSM
> infrastructure happy.
> 
> Before:
> 	$ cat /sys/module/apparmor/parameters/enabled
> 	1
> 
> After:
> 	$ cat /sys/module/apparmor/parameters/enabled
> 	Y
> 
> Reported-by: David Rheinsberg <david.rheinsberg@...il.com>
> Link: https://lkml.kernel.org/r/CADyDSO6k8vYb1eryT4g6+EHrLCvb68GAbHVWuULkYjcZcYNhhw@mail.gmail.com
> Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> This fix, if John is okay with it, is needed in v5.1 to correct the
> userspace regression reported by David.
> ---
>  security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 49d664ddff44..87500bde5a92 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1336,9 +1336,16 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
>  bool aa_g_paranoid_load = true;
>  module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
>  
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp);
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp);
> +#define param_check_aaintbool param_check_int
> +static const struct kernel_param_ops param_ops_aaintbool = {
> +	.set = param_set_aaintbool,
> +	.get = param_get_aaintbool
> +};
>  /* Boot time disable flag */
>  static int apparmor_enabled __lsm_ro_after_init = 1;
> -module_param_named(enabled, apparmor_enabled, int, 0444);
> +module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>  
>  static int __init apparmor_enabled_setup(char *str)
>  {
> @@ -1413,6 +1420,46 @@ static int param_get_aauint(char *buffer, const struct kernel_param *kp)
>  	return param_get_uint(buffer, kp);
>  }
>  
> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
> +{
> +	struct kernel_param kp_local;
> +	bool value;
> +	int error;
> +
> +	if (apparmor_initialized)
> +		return -EPERM;
> +
This isn't sufficient/correct. apparmor_initialized is only set after
apparmor has gone through and completed initialization. However if
apparmor is not selected as one of the LSMs to enable, then this check
won't stop apparmor_enabled from being set post boot.

However with the apparmor_enabled param being 0444 and the
apparmor_enabled_setup() fn handling boot cmdline do with even need
the set parameter fn?

> +	/* Create local copy, with arg pointing to bool type. */
> +	value = !!*((int *)kp->arg);
> +	memcpy(&kp_local, kp, sizeof(kp_local));
> +	kp_local.arg = &value;
> +
> +	error = param_set_bool(val, &kp_local);
> +	if (!error)
> +		*((int *)kp->arg) = *((bool *)kp_local.arg);
> +	return error;
> +}
> +
> +/*
> + * To avoid changing /sys/module/apparmor/parameters/enabled from Y/N to
> + * 1/0, this converts the "int that is actually bool" back to bool for
> + * display in the /sys filesystem, while keeping it "int" for the LSM
> + * infrastructure.
> + */
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp)
> +{
> +	struct kernel_param kp_local;
> +	bool value;
> +
> +	/* Create local copy, with arg pointing to bool type. */
> +	value = !!*((int *)kp->arg);
> +	memcpy(&kp_local, kp, sizeof(kp_local));
> +	kp_local.arg = &value;
> +
> +	return param_get_bool(buffer, &kp_local);
> +}
> +
>  static int param_get_audit(char *buffer, const struct kernel_param *kp)
>  {
>  	if (!apparmor_enabled)
>