[Bug 1691] New: mnl_nlmsg_ok returns true on malformed/incomplete messages leading to potential runtime issues

[bugzilla-daemon at netfilter.org]

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>