Re: [PATCH 2/2] audit: use nlmsg_len() to get message payload length

[Richard Guy Briggs <rgb@redhat.com>]

On Mon, Sep 30, 2013 at 10:04:25PM +0200, Mathias Krause wrote:
> Using the nlmsg_len member of the netlink header to test if the message
> is valid is wrong as it includes the size of the netlink header itself.

Normally, yes.

The original kaudit unicast socket sends up messages with nlmsg_len set
to the payload length rather than the entire message length.  This
breaks the convention used by netlink.  This is set in audit_log_end().
(audit_make_reply looks like it needs a revisit...)
The existing auditd daemon assumes this breakage.  Fixing this would
require co-ordinating a change in the established protocol between
kaudit kernel code and auditd userspace code.

> Thereby allowing to send short netlink messages that pass those checks.
> 
> Use nlmsg_len() instead to test for the right message length. The result
> of nlmsg_len() is guaranteed to be non-negative as the netlink message
> already passed the checks of nlmsg_ok().

Since both of these are downward-headed messages, you are correct.

> Also switch to min_t() to please checkpatch.pl.

Thank you for the catch.

> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: stable@vger.kernel.org  # v2.6.6+ for the 1st hunk, v2.6.23+ for the 2nd
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> ---
>  kernel/audit.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e237712..10c7263 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -671,7 +671,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  				 &status_set, sizeof(status_set));
>  		break;
>  	case AUDIT_SET:
> -		if (nlh->nlmsg_len < sizeof(struct audit_status))
> +		if (nlmsg_len(nlh) < sizeof(struct audit_status))
>  			return -EINVAL;
>  		status_get   = (struct audit_status *)data;
>  		if (status_get->mask & AUDIT_STATUS_ENABLED) {
> @@ -833,7 +833,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  
>  		memset(&s, 0, sizeof(s));
>  		/* guard against past and future API changes */
> -		memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
> +		memcpy(&s, data, min_t(size_t, sizeof(s), nlmsg_len(nlh)));
>  		if ((s.enabled != 0 && s.enabled != 1) ||
>  		    (s.log_passwd != 0 && s.log_passwd != 1))
>  			return -EINVAL;
> -- 
> 1.7.10.4
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545