Since the [main].iwd-config-path functionality, where NM watches for
NMSettingsConnection changes and update IWD network config files with
new settings, has proven to work without issues so far, enable it by
default. Instead of hardcoding /var/lib/iwd as the value, and since the
value can't be probed at NM compile time, query it from IWD's recently-
added D-Bus interface for settings when [main].iwd-config-path is either
missing or set to the new value "auto".
Coverity has issues with functions that handle ownership like
g_strstrip(). Thus the scan is full of false positives like:
Error: RESOURCE_LEAK (CWE-772): [#def45] [important]
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: noescape: Resource "g_strdup(attribute_values[i])" is not freed or pointed-to in "g_strchug".
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: leaked_storage: Failing to save or free storage allocated by "g_strdup(attribute_values[i])" leaks it.
# 132| if (strcmp(attribute_names[i], "value") == 0) {
# 133| parse_context->state = PARSER_METHOD_GSM_APN;
# 134|-> parse_context->apn = g_strstrip(g_strdup(attribute_values[i]));
# 135| break;
# 136| }
Add a workaround for that.
There are other functions that have the same problem, but the usage
g_strstrip(g_strdup(...)) is common to warrant a special workaround.
It's a false positive. Still avoid it.
Error: FORWARD_NULL (CWE-476): [#def479]
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: var_deref_model: Passing "strbuf" to "nm_str_buf_maybe_expand", which dereferences null "strbuf->_priv_str".
# 5573| l2 = ((gsize) l) + 1u;
# 5574|
# 5575|-> nm_str_buf_maybe_expand(strbuf, l2, FALSE);
# 5576|
# 5577| va_start(args, format);
Error: FORWARD_NULL (CWE-476): [#def480]
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: no_write_call: Although "nm_str_buf_maybe_expand" does overwrite "strbuf->_priv_str" on some paths, it also contains at least one feasible path which does not overwrite it.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5578: var_deref_op: Dereferencing null pointer "strbuf->_priv_str".
# 5576|
# 5577| va_start(args, format);
# 5578|-> l = g_vsnprintf(&strbuf->_priv_str[strbuf->_priv_len], l2, format, args);
# 5579| va_end(args);
# 5580|
Also Coverity found that something is wrong here:
Error: FORWARD_NULL (CWE-476): [#def361]
NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:441: var_compare_op: Comparing "connection" to null implies that "connection" might be null.
NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:489: var_deref_model: Passing null pointer "connection" to "g_object_unref", which dereferences it.
# 487| }
# 488|
# 489|-> g_object_unref(connection);
# 490| }
# 491|
Fixes: 6793a32a8c ('libnm: port to GDBus')
Previously, we would allocate a buffer of the worst case, that is,
4 times the number of bytes, in case all of them require octal escaping.
Coverity doesn't like _escape_ansic() for another reason:
Error: NULL_RETURNS (CWE-476): [#def298]
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: returned_null: "g_malloc" returns "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: alias: Assigning: "q" = "dest = g_malloc(strlen(source) * 4UL + 1UL + 3UL)". Both pointers are now "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:163: dereference: Incrementing a pointer which might be null: "q".
# 161| q = dest = g_malloc(strlen(source) * 4 + 1 + 3);
# 162|
# 163|-> *q++ = '$';
# 164| *q++ = '\'';
# 165|
It doesn't recognize that g_malloc() shouldn't return NULL (because
we never request zero bytes).
I am not sure how to avoid that, but let's rework the code to first count
how many characters we exactly need. It think that should also help with
the coverity warning.
Doing exact allocation requires first to count the number of required
bytes. It still might be worth it, because we might keep the allocated
strings a bit longer around.
It seems very ugly to read one byte at a time. Use a naive buffered
reader, so that we can read multiple bytes at a time, and return them
one by one.
Also, this now keeps state of any error/EOF. Once we reach EOF, we won't
read again. The previous code did that too, but I think this code is
easier to read.
g_warning() and printing to stdout/stderr are not suitable actions
for a library. If there is something important, find a way to report the
condition to the caller. If it's not important, stay quiet.
"val" and "key" are now marked as nm_auto. They are freed at the end,
and we should not free them before breaking the loop (at least not,
without also clearing the variables).
Fixes: 02dbba49d6 ('libnm: fix leak in nm_vpn_service_plugin_read_vpn_details()')
Since g_string_free() takes an additional argument,
it's not direclty usable with nm_clear_pointer(ptr, g_string_free);
As workaround, add nm_clear_g_string() helper.
Coverity doesn't like the previous code:
Error: RESOURCE_LEAK (CWE-772): [#def34] [important]
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: noescape: Resource "g_strdup(config)" is not freed or pointed-to in "g_strdelimit".
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: leaked_storage: Failing to save or free storage allocated by "g_strdup(config)" leaks it.
# 833| char *sanitized_config;
# 834|
# 835|-> sanitized_config = g_strdelimit(g_strdup(config), "\r\n", ' ');
# 836| err = teamdctl_port_config_update_raw(priv->tdc, slave_iface, sanitized_config);
# 837| g_free(sanitized_config);
Maybe this works better.
When the ACCEPT_ALL_MAC_ADDRESSES key is found by the wired reader, the
wired setting was not being created.
Fixes: d946aa0c50 ('wired-setting: add support to accept-all-mac-addresses')
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Commit 33b9fa3a3c ("manager: Keep volatile/external connections
while referenced by async_op_lst") changed active_connection_find() to
also return active connections that are not yet activating but are
waiting authorization.
This has side effect for other callers of the function. In particular,
_get_activatable_connections_filter() should exclude only ACs that are
really active, not those waiting for authorization.
Otherwise, in ensure_master_active_connection() all the ACs waiting
authorization are missed and we might fail to find the right master
AC.
Add an argument to active_connection_find to select whether include
ACs waiting authorization.
Fixes: 33b9fa3a3c ('manager: Keep volatile/external connections while referenced by async_op_lst')
https://bugzilla.redhat.com/show_bug.cgi?id=1955101
A user might configure /etc/dhcpcd.conf to contain static fallback addresses.
In that case, the dhcpcd plugin reports the state as "static". Let's treat
that the same way as bound.
Note that this is not an officially supported or endorsed way of
configuring fallback addresses in NetworkManager. Rather, when using
DHCP plugins, the user can hack the system and make unsupported
modifications in /etc/dhcpcd.conf or /etc/dhcp. This change only makes
it a bit easier to do it.
See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/579#note_922758https://bugzilla.gnome.org/show_bug.cgi?id=768362
Based-on-patch-by: gordonb3 <gordon@bosvangennip.nl>
We use the merge function to initialize the cloned instance.
Previously, merge did not always copy all properties, so the
cloned instance might not have been identical. Fix that.
gboolean is a typedef for int, so there is no difference in behavior.
However, we use IS_IPv4 as index into arrays of length two. Making
it "int" seems more approriate. Also, this is what all the other
(similar) code does.
dhcp-anycast-address is only set by OLPC mesh device. It's ugly to have
this in form of a nm_device_set_dhcp_anycast_address() method, because
that means to cache the address in NMDevice. Meaning, we have more state
in NMDevice, where it's not clear where it comes from.
Instead, whenever we need to DHCP anycast address, as the subclass to
provide it (if any). This way, it gets extracted from the currently
applied connection at the moment when it is needed. Beyond that, the
setting is not duplicated/cached in NMDevice anymore.
Instead of passing the setting on during ip4_start()/ip6_start(), make
it a property of NMDhcpClient.
This property is currently only set by OLPC devices, and is only
implemented by NMDhcpDhclient. As such, it also does not need to change
or get reset. Hence, and immutable, construct-only property is clearer,
because we don't have to pass parameters to ip[46]_start().
Arguably, the parameter is still there, but being immutable and always
set, make it easier to reason about it.
Kernel will coerce values like
ethtool -A eth0 autoneg on rx off
to have autonet still on.
Also, if autoneg on the interface is enabled, then `ethtool -A eth0 tx off`
has no effect.
In NetworkManager, the user cannot configure "autoneg on" together with
any rx/tx settings. That would render the profile invalid. However, we
also need to take care that a profile
nmcli connection add ... ethtool.pause-autoneg ignore ethtool.pause-tx off
really means off. That means, we must coerce an unspecified autoneg
setting to "off".
Setting pause-rx/pause-tx to an explicit value, implies that the user
does not want to enable autoneg. Reject that as invalid value in the
connection profile.
Currently we commit the MTU to the device when updating the IP
configuration, or when a port device is added to the controller. This
means that for a connection with DHCP, the MTU is set only after DHCP
has completed. In particular, if DHCP doesn't complete and the
connection has an infinite timeout, the MTU is never set.
_commit_mtu() tracks different sources for the MTU of a device, and
each source has a different priority. Among these sources there are
the parent link (for VLANs), a dynamic IP configuration (DHCP, PPP)
and the connection profile.
A MTU from the connection always has the highest priority and
overrides other sources.
Therefore, if the connection specifies an MTU it can be applied at
stage2, even before configuring IP addressing.
https://bugzilla.redhat.com/show_bug.cgi?id=1890234https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/859
If the device is still unmanaged by platform-init (which means that
udev didn't emit the event for the interface) when the device gets
realized, we currently clear the assume state. Later, when the device
becomes managed, NM is not able to properly assume the device using
the UUID.
This situation arises, for example, when NM already configured the
device in initrd; after NM is restarted in the real root, udev events
can be delayed causing this race condition.
Among all unamanaged flags, platform-init is the only one that can be
delayed externally. We should not clear the assume state if the device
has only platform-init in the unmanaged flags.
ntables backend is not yet well tested. Don't flip the default yet
but for now always use iptables.
Once nftables is shown to work well, revert this patch.
Add support for nftables, as a second backend beside iptables (firewalld
still missing).
Like iptables, choose to call the `nft` tool. The alternative would be
to use libnftables or talk netlink.
It's ugly to blocking wait for a process to complete. We already do that
for iptables, but we better should not because we should not treat other
processes as trusted and not allow untrusted code to block NetworkManager.
Fixing that would require a central manager that serializes all requests.
Especially with firewalld support, this will be interesting again,
because we don't want to synchronously talk D-Bus either.
For now, `nft` is still called synchronously. However, the internal
implementation uses an asynchronous function. That currently
serves no purpose except supporting a timeout. Otherwise, the only
reason why this is asynchronous is that I implemented this first, and
I think in the future we want this code to be non-blocking. So, instead
of dropping the asynchronous code, I wrap it in a synchronous function
for now.
The configured nft table is:
table inet nm-shared-eth0 {
chain nat_postrouting {
type nat hook postrouting priority srcnat; policy accept;
ip saddr 192.168.42.0/24 ip daddr != 192.168.42.0/24 masquerade
}
chain filter_forward {
type filter hook forward priority filter; policy accept;
ip daddr 192.168.42.0/24 oifname "eth0" ct state { established, related } accept
ip saddr 192.168.42.0/24 iifname "eth0" accept
iifname "eth0" oifname "eth0" accept
iifname "eth0" reject
oifname "eth0" reject
}
}
The new name makes it more generic, because the limitation exists
for iptables chains. Everything else (iptables comments, nftables
tables) has no such length limit.
