linux-kernel - Re: [PATCH] Smack: Move request_buffer from stack to smack_audit

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] [day] [month] [year] [list]

Message-ID: <abb71a39-b90f-5384-44e6-dcc01fa8f20b@schaufler-ca.com>
Date:   Tue, 3 Sep 2019 13:56:37 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Denis Efremov <efremov@...ux.com>
Cc:     "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, casey@...aufler-ca.com
Subject: Re: [PATCH] Smack: Move request_buffer from stack to smack_audit_data

On 9/3/2019 11:01 AM, Denis Efremov wrote:
> request_buffer is required to describe an access type in a string for
> the audit. The problem here is that the string is saved on the stack
> and then passed by reference to the next function in request field of
> the smack_audit_data structure. Referencing variables on a stack
> and saving them in external data structures is usually considered
> as bad and error-prone practice.

You're adding space to the smack_audit_data structure on the
off chance that the stack might disappear out from under something
this function is calling. If you trace the code path, you'll find
that doesn't happen. I can't say that I see any real value to this
change.

>  Thus, this commit simply moves
> the request_buffer from the stack to the stack_audit_data structure
> and removes the necessity of stack referencing. strcat calls are
> replaced with strlcat calls - a safer analog for strings concatenation
> with bounds checking.

Changing strcat to strlcat (or any variant, for that matter) when
the source is a string constant and the destination size is known
is completely pointless.

>
> Signed-off-by: Denis Efremov <efremov@...ux.com>

I appreciate the intention, but I don't see any real value here.
I won't be taking this.

> ---
>  security/smack/smack.h        |  6 +++++-
>  security/smack/smack_access.c | 12 +++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 62529f382942..9eeefb865dfd 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -278,7 +278,11 @@ struct smack_audit_data {
>  	const char *function;
>  	char *subject;
>  	char *object;
> -	char *request;
> +#ifdef CONFIG_SECURITY_SMACK_BRINGUP
> +	char request[SMK_NUM_ACCESS_TYPE + 5];
> +#else
> +	char request[SMK_NUM_ACCESS_TYPE + 1];
> +#endif
>  	int result;
>  };
>  
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index f1c93a7be9ec..99e58d4a9980 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -340,11 +340,6 @@ static void smack_log_callback(struct audit_buffer *ab, void *a)
>  void smack_log(char *subject_label, char *object_label, int request,
>  	       int result, struct smk_audit_info *ad)
>  {
> -#ifdef CONFIG_SECURITY_SMACK_BRINGUP
> -	char request_buffer[SMK_NUM_ACCESS_TYPE + 5];
> -#else
> -	char request_buffer[SMK_NUM_ACCESS_TYPE + 1];
> -#endif
>  	struct smack_audit_data *sad;
>  	struct common_audit_data *a = &ad->a;
>  
> @@ -360,7 +355,7 @@ void smack_log(char *subject_label, char *object_label, int request,
>  		sad->function = "unknown";
>  
>  	/* end preparing the audit data */
> -	smack_str_from_perm(request_buffer, request);
> +	smack_str_from_perm(sad->request, request);
>  	sad->subject = subject_label;
>  	sad->object  = object_label;
>  #ifdef CONFIG_SECURITY_SMACK_BRINGUP
> @@ -371,14 +366,13 @@ void smack_log(char *subject_label, char *object_label, int request,
>  	 * the logging policy says to do so.
>  	 */
>  	if (result == SMACK_UNCONFINED_SUBJECT)
> -		strcat(request_buffer, "(US)");
> +		strlcat(sad->request, "(US)", sizeof(sad->request));

Have you ever heard of a C compiler that would not correctly
terminate a constant string? I've been using them for over 40
years and have never seen a case where this was a problem.

>  	else if (result == SMACK_UNCONFINED_OBJECT)
> -		strcat(request_buffer, "(UO)");
> +		strlcat(sad->request, "(UO)", sizeof(sad->request));
>  
>  	if (result > 0)
>  		result = 0;
>  #endif
> -	sad->request = request_buffer;
>  	sad->result  = result;
>  
>  	common_lsm_audit(a, smack_log_callback, NULL);