Linux netfilter: Fix a Y2K38_SAFETY Coverity warning

[fxlb]

This concerns the 'Sequence number' in Netlink messages.

The Linux Documentation/userspace-api/netlink/intro.rst states:
:c:member:`nlmsghdr.nlmsg_seq` should be a set to a monotonically
increasing value. The value gets echoed back in responses and doesn't
matter in practice, but setting it to an increasing value for each
message sent is considered good hygiene. The purpose of the field is
matching responses to requests.

Thus seq_id, used to set it, has no need to be initialized to
time(NULL). It can start at 1.

The Coverity warning was:
CID 1508935: Use of 32-bit time_t (Y2K38_SAFETY)
A time_t value is stored in an integer with too few bits to accommodate
  it. The expression time(NULL) is cast to unsigned int.
  356                seq_id = time(NULL);

There was also an old clang warning on 64-bit build:
pcap-netfilter-linux.c:356:12: warning: implicit conversion loses
  integer precision: 'time_t' (aka 'long') to 'unsigned int'
  [-Wshorten-64-to-32]
  356 |                 seq_id = time(NULL);
      |                        ~ ^~~~~~~~~~

It was silenced by 16782c7e7811c2bc821c1f58ae948af7bccf81fe.
This commit is no longer useful.

The "To be look at before 2038..." in the previous commit message is no
longer relevant.

[guyharris]

It can start at 1

Which also means it takes longer to roll over (and ceases to be monotonically increasing), so that's an additional improvement. (Unless a lot of messages are sent per second, it'll be a while before it's likely to roll over, but....)

[fxlb]

When doing a test with a seq_idprint, I got only [seq_id=1] [seq_id=2] [seq_id=3] [seq_id=4] at the beginning of the capture.

[fxlb]

Note:
Only four for a capture on nflog:XXX.
four + one by packet captured on nfqueue:XXX.

[fxlb]

Because "The purpose of the field is matching responses to requests." a rool over should not be a problem.

[guyharris]

"Communicating between the kernel and user-space in Linux using Netlink Sockets: Source code reference" says, on page 10:

Finally, to generate the sequence numbering (line 20), we have selected the function time() which returns the seconds since 1970. The kernel uses the same sequence number in the acknowledgment and data replies, thus, the sequence number provides a way to identify that a given message comes as reply of certain request. Therefore, there is no need to use incremental sequence numbers.

I presume that "there is no need to use incremental sequence numbers" because, in that code, there's only one message in flight, so you would just use time() for each message, and that would suffice to uniquely identify the reply to the current request.

What libnl3 does, as of the current tip of the master branch, is:

1. have a per-netlink-socket data structure with a sequence number used with that socket;
2. initialize that sequence number value with the result of _badrandom_from_time(), which gets the value of time(NULL), assigns it to a uint64_t, assigns the lower 32 bits to a uint64_t, XORs the complement of the upper 32 bits with it (because "Otherwise, coverity warns about only considering 32 bit from time_t."), and returns the result;
3. whenever a sequence number is to be put in a message, returns that sequence number and increments it, wrapping around at UINT_MAX.

I guess there's some concern about collisions between sequence numbers in difference sockets, otherwise I'm not sure why you don't just start them all at 1.

[fxlb]

If "there's some concern about collisions between sequence numbers in difference sockets,", there is a design problem.

[fxlb]

In https://people.netfilter.org/pablo/netlink/netlink.pdf:

Sequence number (32 bits): message sequence number. This is useful together with
NLM F ACK if an user-space application wants to make sure that a request has been
correctly issued. Netlink uses the same sequence number in the messages that are sent as
reply to a given request §. For event-based notifications from kernel-space, this is always
zero.

With note:

§The sequence number is used as a tracking cookie since the kernel does not change the sequence number value
at all.

Nothing about time() or risk of collisions.

[guyharris]

So is there a reason why libnl3 bothers to randomize the sequence numbers?

Or did they do that because they mistakenly thought that they had to do that?

[fxlb]

It's unclear because there is also:
(different context/use?)

/**
 * Use next sequence number
 * @arg sk              Netlink socket.
 * 
 * Uses the next available sequence number and increases the counter
 * by one for subsequent calls.
 * 
 * @return Unique serial sequence number
 */     
unsigned int nl_socket_use_seq(struct nl_sock *sk)
{
        if (sk->s_seq_next == UINT_MAX) {
                sk->s_seq_next = 0;
                return UINT_MAX;
        }
        return sk->s_seq_next++;
}       

[fxlb]

In libnl, doc/core.txt:

[[core_seq_num]]
=== Sequence Numbers

Netlink allows the use of sequence numbers to help relate replies to
requests. It should be noted that unlike in protocols such as TCP
there is no strict enforcement of the sequence number. The sole purpose
of sequence numbers is to assist a sender in relating replies to the
corresponding requests. See <<core_msg_types>> for more information.

Sequence numbers are managed on a per socket basis, see
<<core_sk_seq_num>> for more information on how to use sequence numbers.

[fxlb]

Same file:

Sequence Number (32bit)::
The sequence number is optional and may be used to allow referring to
a previous message, e.g. an error message can refer to the original
request causing the error.

[fxlb]

So is there a reason why libnl3 bothers to randomize the sequence numbers?

Or did they do that because they mistakenly thought that they had to do that?

Good questions...

[fxlb]

No mention to time() use in https://www.rfc-editor.org/rfc/rfc3549.
(Linux Netlink as an IP Services Protocol)

[fxlb]

https://docs.kernel.org/6.1/userspace-api/netlink/intro.html:

nlmsghdr.nlmsg_seq should be a set to a monotonically increasing value. The value gets echoed back in
responses and doesn’t matter in practice, but setting it to an increasing value for each message sent is
considered good hygiene. The purpose of the field is matching responses to requests. Asynchronous
notifications will have nlmsghdr.nlmsg_seq of 0.

Same as in Documentation/userspace-api/netlink/intro.rst.

[fxlb]

Their "randomize" is not randomize and is decreasing.

#include <stdint.h>
#include <time.h>
#include <stdio.h>
#include <unistd.h>

int main ()
{
    uint32_t result;
    uint64_t v64;
    time_t t;
    int i;

    for (i = 0; i < 5; i++) {
        t = time(NULL);
        v64 = (uint64_t)t;
        result = (uint32_t)v64;

        /* XOR with the upper bits. Otherwise, coverity warns about only
         * considering 32 bit from time_t.  Use the inverse, so that for the
         * most part the bits don't change.  */
        result ^= (~(v64 >> 32));
        printf("%u\n", result);

        sleep(1);
    }
}

Result example:
2533809684
2533809683
2533809682
2533809681
2533809680

[guyharris]

There are two parts to libnl3's sequence number handling:

1. the initial sequence number for a socket comes from the randomizer in _badrandom_from_time();
2. all later sequence numbers come from the incrementing in nl_socket_use_seq().

The random initial sequence number sounds similar to what's done for TCP. That's done to make it harder for an attacker to guess sequence numbers, not to prevent sequence number collisions from different TCP connections.

If replies to netlink packets always arrive on the same socket as the one on which the request was sent, that's similar to TCP, so there's no need to prevent collisions from different sockets. It may be different from TCP, however, in that I'm not sure there's a way to attack a netlink socket user by injecting traffic on the socket. If there's a way to do that, randomized sequence numbers might be used as a defense against that.

The only way I see avoiding collisions between sockets being useful would be...

...if you're running a program that captures on an nlmon device to monitor netlink traffic, and the program wants to match request and responses. I don't see anything in LINKTYPE_NETLINK to associate a captured netlink packet with a specific socket.

[fxlb]

There are two parts to libnl3's sequence number handling:

1. the _initial_ sequence number for a socket comes from the randomizer in `_badrandom_from_time()`;

2. all _later_ sequence numbers come from the incrementing in `nl_socket_use_seq()`.

Thank you for digging deeper.

The random initial sequence number sounds similar to what's done for TCP. That's done to make it harder for an attacker to guess sequence numbers, not to prevent sequence number collisions from different TCP connections.

If replies to netlink packets always arrive on the same socket as the one on which the request was sent, that's similar to TCP, so there's no need to prevent collisions from different sockets. It may be different from TCP, however, in that I'm not sure there's a way to attack a netlink socket user by injecting traffic on the socket. If there's a way to do that, randomized sequence numbers might be used as a defense against that.

I think there is a big difference about randomization:

On Linux, e.g., the TCP sequence number, (via secure_tcp_seq()) based on adresses, ports, secret, hashes is "unpredictable" (or at least very difficult to predict).

The sequence number based on time() seems totally predictable.