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

[Mathias Krause]

On 1 October 2013 05:51, Richard Guy Briggs <rgb at 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