* [PATCH 0/2] netlink related fixes @ 2013-09-30 20:04 Mathias Krause 2013-09-30 20:04 ` [PATCH 1/2] audit: fix info leak in AUDIT_GET requests Mathias Krause 2013-09-30 20:04 ` [PATCH 2/2] audit: use nlmsg_len() to get message payload length Mathias Krause 0 siblings, 2 replies; 6+ messages in thread From: Mathias Krause @ 2013-09-30 20:04 UTC (permalink / raw) To: linux-audit; +Cc: Al Viro 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] audit: fix info leak in AUDIT_GET requests 2013-09-30 20:04 [PATCH 0/2] netlink related fixes Mathias Krause @ 2013-09-30 20:04 ` Mathias Krause 2013-10-01 2:20 ` Richard Guy Briggs 2013-09-30 20:04 ` [PATCH 2/2] audit: use nlmsg_len() to get message payload length Mathias Krause 1 sibling, 1 reply; 6+ messages in thread From: Mathias Krause @ 2013-09-30 20:04 UTC (permalink / raw) To: linux-audit; +Cc: Al Viro 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] audit: fix info leak in AUDIT_GET requests 2013-09-30 20:04 ` [PATCH 1/2] audit: fix info leak in AUDIT_GET requests Mathias Krause @ 2013-10-01 2:20 ` Richard Guy Briggs 0 siblings, 0 replies; 6+ messages in thread From: Richard Guy Briggs @ 2013-10-01 2:20 UTC (permalink / raw) To: Mathias Krause; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] audit: use nlmsg_len() to get message payload length 2013-09-30 20:04 [PATCH 0/2] netlink related fixes Mathias Krause 2013-09-30 20:04 ` [PATCH 1/2] audit: fix info leak in AUDIT_GET requests Mathias Krause @ 2013-09-30 20:04 ` Mathias Krause 2013-10-01 3:51 ` Richard Guy Briggs 1 sibling, 1 reply; 6+ messages in thread From: Mathias Krause @ 2013-09-30 20:04 UTC (permalink / raw) To: linux-audit; +Cc: Al Viro 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] audit: use nlmsg_len() to get message payload length 2013-09-30 20:04 ` [PATCH 2/2] audit: use nlmsg_len() to get message payload length Mathias Krause @ 2013-10-01 3:51 ` Richard Guy Briggs 2013-10-01 6:33 ` Mathias Krause 0 siblings, 1 reply; 6+ messages in thread From: Richard Guy Briggs @ 2013-10-01 3:51 UTC (permalink / raw) To: Mathias Krause; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] audit: use nlmsg_len() to get message payload length 2013-10-01 3:51 ` Richard Guy Briggs @ 2013-10-01 6:33 ` Mathias Krause 0 siblings, 0 replies; 6+ messages in thread From: Mathias Krause @ 2013-10-01 6:33 UTC (permalink / raw) To: Richard Guy Briggs; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-01 6:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-30 20:04 [PATCH 0/2] netlink related fixes Mathias Krause 2013-09-30 20:04 ` [PATCH 1/2] audit: fix info leak in AUDIT_GET requests Mathias Krause 2013-10-01 2:20 ` Richard Guy Briggs 2013-09-30 20:04 ` [PATCH 2/2] audit: use nlmsg_len() to get message payload length Mathias Krause 2013-10-01 3:51 ` Richard Guy Briggs 2013-10-01 6:33 ` Mathias Krause
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox