netfilter buglog - Jul 2023 - [Bug 1691] New: mnl_nlmsg_ok returns true on malformed/incomplete messages leading to potential runtime issues

https://bugzilla.netfilter.org/show_bug.cgi?id=1691

            Bug ID: 1691
           Summary: mnl_nlmsg_ok returns true on malformed/incomplete
                    messages leading to potential runtime issues
           Product: libmnl
           Version: unspecified
          Hardware: x86_64
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: libmnl
          Assignee: pablo at netfilter.org
          Reporter: eliogovea1993 at gmail.com

Consider the following example:

source:
```
#include <linux/netfilter.h>

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

#include <libmnl/libmnl.h>

int main(int argc, char** argv) {
    const struct nlmsghdr header = {
        .nlmsg_len = 0xFFFFFFFF,
        .nlmsg_type = 0x0000,
        .nlmsg_flags = 0x0000,
        .nlmsg_seq = 0x00000000,
        .nlmsg_pid = 0x00000000
    };

    assert(!mnl_nlmsg_ok(&header, sizeof(struct nlmsghdr)));

    return EXIT_SUCCESS;
}
```

output:
```
parse-header: parse-header.c:19: main: Assertion `!mnl_nlmsg_ok(&header,
sizeof(struct nlmsghdr))' failed.
Aborted (core dumped)
```

The previous example has a malformed message, the "buffer" used as
argument for
"mnl_nlmsg_ok" doesn't hold a complete message (length
"0xFFFFFFFF" is higher
than sizeof(header)), yet the function returns true. It seems unlikely that a
message will have that "nlmsg_len" in the header, but this report
assumes that
such bytes are possible while parsing, and a parser using libmnl must correctly
identify any incomplete messages.

Looking at the source code

```
EXPORT_SYMBOL bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
{
    return len >= (int)sizeof(struct nlmsghdr) &&
           nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
           (int)nlh->nlmsg_len <= len;
}
```

The last line casts "nlh->nlmsg_len" to "int", but that
value can't be stored
in an "int" and casting it makes its value negative. To fix it,
consider the
following patch:

```
diff --git a/src/nlmsg.c b/src/nlmsg.c
index c634501..8d0107f 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -154,7 +154,7 @@ EXPORT_SYMBOL bool mnl_nlmsg_ok(const struct nlmsghdr *nlh,
int len)
 {
        return len >= (int)sizeof(struct nlmsghdr) &&
               nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
-              (int)nlh->nlmsg_len <= len;
+              nlh->nlmsg_len <= (unsigned int)len;
 }

 /**

Casting "len" to "unsigned int" would be correct since the
first line already
checks that it is positive.

Since "mnl_nlmsg_ok" is used in a while loop in
"__mnl_cb_run", parsing
malformed messages like the previous one will try to dereference a "struct
nlmsghdr*" out of the limits of the message buffer.
```

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230703/aab736ac/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1691

Jeremy Sowden <jeremy at azazel.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |jeremy at azazel.net
           Assignee|pablo at netfilter.org         |jeremy at azazel.net

--- Comment #1 from Jeremy Sowden <jeremy at azazel.net> ---
My preference would be to get rid of the casts altogether:

EXPORT_SYMBOL bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
{
    size_t ulen = len;

    if (len < 0)
        return 0;

    return ulen           >= sizeof(struct nlmsghdr) &&
           nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
           nlh->nlmsg_len <= ulen;
}

I will send a patch.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230910/67446af2/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1691

Jeremy Sowden <jeremy at azazel.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #2 from Jeremy Sowden <jeremy at azazel.net> ---
http://git.netfilter.org/libmnl/commit/?id=754c9de5ea1bea821495523cf01989299552e524

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20240328/7fee251b/attachment.html>