Re: [PATCH 2/2] audit: use nlmsg_len() to get message payload length
On 1 October 2013 05:51, Richard Guy Briggs <rgb(a)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