fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2025-12-29 12:40:11 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

platform/netlink: don't reallocate ancillary data for recvmsg() on truncation

Browse source 
Coverity thinks there is a problem here:

    Error: TAINTED_SCALAR (CWE-20): [#def233]
    NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1437: tainted_argument: Calling function "recvmsg" taints argument "msg".
    NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: tainted_data: Passing tainted expression "msg.msg_controllen" to "g_realloc", which uses it as an allocation size.
    NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
    # 1456|
    # 1457|           msg.msg_controllen *= 2;
    # 1458|->         msg.msg_control = g_realloc(msg.msg_control, msg.msg_controllen);
    # 1459|           goto retry;
    # 1460|       }

but the problem is not the tainted data. The problem is how should
we handle MSG_CTRUNC? If we reach MSG_CTRUNC we already lost a message.
Retrying to receive the next message is not going to fix that and is
wrong.

Also, there really is no reason why any truncation should happen. The only
ancillary data that should be present is the sender information, and for
that our buffer is supposed to be large enough.

So, simply ignore truncation. It shouldn't happen, if it happened we
cannot recover from it (aside failing an assertion), and all we really
care are the retrieved credentials. If truncation happened, we might
not have retrieved the credentials, but then that is for the caller
to handle (by rejecting the message as untrusted).

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/872
This commit is contained in:
Thomas Haller 2021-05-27 14:04:55 +02:00
parent ff922f89f4
commit 5740ed67cb
No known key found for this signature in database
GPG key ID: 29C2366E4DFC5728
1 changed files with 24 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

42
src/libnm-platform/nm-netlink.c
View file

@ -1403,14 +1403,27 @@ nl_recv(struct nl_sock * sk,
struct ucred * out_creds,
gboolean * out_creds_has)
{
/* We really expect msg_contol_buf to be large enough and MSG_CTRUNC not
* happening. We nm_assert() against that. However, in release builds
* we don't assert, so add some extra safety space for the unexpected
* case where we might need more than CMSG_SPACE(sizeof(struct ucred)).
* It should not hurt and should not be necessary. It's just some
* extra defensive space. */
#define _MSG_CONTROL_BUF_EXTRA_SPACE (NM_MORE_ASSERTS ? 512u : 0u)
union {
struct cmsghdr cmsghdr;
char buf[CMSG_SPACE(sizeof(struct ucred)) + _MSG_CONTROL_BUF_EXTRA_SPACE];
} msg_contol_buf;
ssize_t n;
int flags = 0;
struct iovec iov;
struct msghdr msg = {
.msg_name = (void *) nla,
.msg_namelen = sizeof(struct sockaddr_nl),
.msg_iov = &iov,
.msg_iovlen = 1,
.msg_name = (void *) nla,
.msg_namelen = sizeof(struct sockaddr_nl),
.msg_iov = &iov,
.msg_iovlen = 1,
.msg_controllen = 0,
.msg_control = NULL,
};
struct ucred tmpcreds;
gboolean tmpcreds_has = FALSE;
@ -1429,8 +1442,8 @@ nl_recv(struct nl_sock * sk,
iov.iov_base = g_malloc(iov.iov_len);
if (out_creds && (sk->s_flags & NL_SOCK_PASSCRED)) {
msg.msg_controllen = CMSG_SPACE(sizeof(struct ucred));
msg.msg_control = g_malloc(msg.msg_controllen);
msg.msg_controllen = sizeof(msg_contol_buf);
msg.msg_control = msg_contol_buf.buf;
}
retry:
@ -1448,16 +1461,11 @@ retry:
goto abort;
}
if (msg.msg_flags & MSG_CTRUNC) {
if (msg.msg_controllen == 0) {
retval = -NME_NL_MSG_TRUNC;
goto abort;
}
msg.msg_controllen *= 2;
msg.msg_control = g_realloc(msg.msg_control, msg.msg_controllen);
goto retry;
}
/* We really don't expect truncation of ancillary data. We provided a large
* enough buffer, so this is likely a bug. In the worst case, we might lack
* the requested credentials and the caller likely will reject the message
* later. */
nm_assert(!(msg.msg_flags & MSG_CTRUNC));
if (iov.iov_len < n || (msg.msg_flags & MSG_TRUNC)) {
/* respond with error to an incomplete message */
@ -1503,8 +1511,6 @@ retry:
retval = n;
abort:
g_free(msg.msg_control);
if (retval <= 0) {
g_free(iov.iov_base);
return retval;