[PATCH 0/2] netlink related fixes

[PATCH 0/2] netlink related fixes

[Mathias Krause]

This series fixes two issues of the netlink interface -- one info leak
and a wrong size check.

Please apply!


Mathias Krause (2):
  audit: fix info leak in AUDIT_GET requests
  audit: use nlmsg_len() to get message payload length

 kernel/audit.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] audit: fix info leak in AUDIT_GET requests

[Mathias Krause]

We leak 4 bytes of kernel stack in response to an AUDIT_GET request as
we miss to initialize the mask member of status_set. Fix that.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Paris <eparis@redhat.com>
Cc: stable@vger.kernel.org  # v2.6.6+
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 kernel/audit.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..e237712 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -659,6 +659,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 	switch (msg_type) {
 	case AUDIT_GET:
+		status_set.mask		 = 0;
 		status_set.enabled	 = audit_enabled;
 		status_set.failure	 = audit_failure;
 		status_set.pid		 = audit_pid;
-- 
1.7.10.4

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

[Mathias Krause]

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.
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().

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

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

Re: [PATCH 1/2] audit: fix info leak in AUDIT_GET requests

[Richard Guy Briggs]

On Mon, Sep 30, 2013 at 10:04:24PM +0200, Mathias Krause wrote:
> We leak 4 bytes of kernel stack in response to an AUDIT_GET request as
> we miss to initialize the mask member of status_set. Fix that.

Thank you.

> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: stable@vger.kernel.org  # v2.6.6+
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> ---
>  kernel/audit.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..e237712 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -659,6 +659,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  
>  	switch (msg_type) {
>  	case AUDIT_GET:
> +		status_set.mask		 = 0;
>  		status_set.enabled	 = audit_enabled;
>  		status_set.failure	 = audit_failure;
>  		status_set.pid		 = audit_pid;
> -- 
> 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

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

[Richard Guy Briggs]

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

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

[Mathias Krause]

On 1 October 2013 05:51, Richard Guy Briggs <rgb@redhat.com> wrote:
> 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...)

Looks like there are two paths for sending netlink messages to
userland. One using audit_send_reply(), correctly filling the
nlmsg_len. One using audit_log_start()/audit_log_end() implementing
the protocol you're describing. Weird :/

> 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.

Looking at the current userland audit netlink code (v2.3.2,
lib/netlink.c:adjust_reply) it looks like the library doesn't care
much. It just ensures nlmsg_len doesn't exceed the length of the
received message and that the message is at least as big as a netlink
header. It even expects the regular netlink message rules do apply by
using NLMSG_OK() for the test -- so nlmsg_len must be at least as
large as a netlink header.

>> 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.


Regards,
Mathias