On architectures where "char" is signed, the check "ch < ' '" is
also TRUE for non-ASCII characters greater than 127. This is an
easy mistake to make. Fix it by using nm_ascii_is_control() which
gets this right.
It's a bug, but possibly not too bad because unnecesarily escaping
a UTF-8 characters is not a severe problem, because the user anyway must
be prepared to unescape the string.
(cherry picked from commit 83f888054b)
These functions have overlap with g_ascii_is*() functions.
However g_ascii_is*() (and the is* functions from <ctype.h>) are
always confusing to me, in the sense that it's not clearly stated
which characters qualify for a certain category. And review is not
easy either, because they are implemented via a table lookup.
E.g. were you aware that 127 is considered g_ascii_iscntrl()? Probably
you were, but it's not clear to see that anywhere.
The main point of our own functions is to have is easier to see how
characters get categorized, by using comparison instead of table lookup.
Also, several existing code did in fact not use the g_ascii_is*()
macros, possibly because of the (perceived) difficulty to understand
their exact meaning. As a consequence, several checks got wrong.
For example, (ch < ' ') is not a valid check for testing whether
the character is a ASCII control character, for two reasons:
- if char is a signed type (as likely it is), then this also evaluates
to TRUE for all non-ASCII, UTF-8 characters that are greater than
127.
- it does not consider DEL character (127) a control character.
(cherry picked from commit 5b6005d06e)
Problems of this patch:
- the code does not differentiate between an ifcfg file and an alias
file. Different shell variables are honored however depending on the
context and the warning should reflect that.
- there are no warnings about /etc/sysconfig/network. The main problem
is that we read this file for every ifcfg file we parse, and we would
need to ratelimit the number of warnings. Another problem is that
the file likely contains keys that we intentionally don't support.
We would need a new way to omit warnings about those lines.
Example:
TYPE=Ethernet
PROXY_METHOD=none
BROWSER_ONLY=no
BOOTPROTO=dhcp
DEFROUTE=yes
STABLE_ID=$'xxx\xF4yy'
IPV4_FAILURE_FATAL=no
IPV6INIT=yes
XX=foo
XX1=foo'
'
IPV6_AUTOCONF=yes xxxx
IPV6_DEFROUTE=yesx
IPV6_DEFROUTE=yes
IPV6_FAILURE_FATAL=no
IPV6_ADDR_GEN_MODE=stable-privacy
NAME=xxx
UUID=9d8ed7ff-3cdd-4336-9e26-3e978dc87102
ONBOOT=no
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:6: key STABLE_ID does not contain valid UTF-8 and is treated as ""
<debug> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:9: key XX is unknown and ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:10: key XX1 is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:11: invalid line ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:12: key IPV6_AUTOCONF is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:13: key IPV6_DEFROUTE is duplicated and the early occurrence ignored
https://bugzilla.redhat.com/show_bug.cgi?id=1959656
(cherry picked from commit 41be0c8fde)
ifcfg files are a text format. It makes no sense to ever accept
non-UTF-8 blobs. If binary data is to be encoded in a ifcfg file, then
the upper layers must escape/encode it in valid UTF-8.
Let svUnescape() silently reject any binary "text". This will lead to treat such
strings as empty strings "". This is no different than some invalid
quoting: the string is not parsable as (UTF-8) text and will be treated
as such.
This is potentially a breaking change. But the benefit is that all the
upper layers can rely on only getting valid UTF-8 strings. For example,
a non-UTF-8 string cannot be converted to a "s" GVariant (of course not,
it's not a string). But our nm_connection_verify() commonly does not
check that all strings are in fact valid UTF-8. So a user who edits
an ifcfg file could inject non-valid strings, and cause assertion
failures later on.
It's actually easy to provoke a crash (or at least an assertion failure)
by writing an ifcfg file with certain keys as binary.
Note that you can either reproduce the binary files by writing non-UTF-8
"strings" dirctly, or by using \x, \u, or \U escape sequences.
Note that also '\0' gets rejected and renders the string as invalid
(i.e. as empty). Before the returned string would have been simply
truncated and the rest ignored. Such NUL bytes can only be produced
using the escape sequences, because the ifcfg reader already (silently)
truncates the file on the first binary NUL.
(cherry picked from commit 7c9b0d68e4)
Note that previously the check
if (s[slen] < ' ') {
...
return (*to_free = _escape_ansic(s));
}
would be TRUE for all UTF-8 characters if `char` is signed. That means,
depending on the compiler, we would always ANSI escape all UTF-8
characters. With this patch, we no longer do that!
Instead, valid unicode gets now preserved (albeit quoted).
On the other hand, always ANSIC escape invalid UTF-8 (regardless of the
compiler). ifcfg-rh is really a text based format. If a caller wants to store
binary data, they need to escape it first, for example with some own escaping
scheme, base64 or bin2hexstr.
A caller passing a non-text to svEscape() is likely a bug already and
they should have not done that.
Still, let svEscape() handle that by using ANSIC escaping. That works
as far as escaping is concerned, but likely later will be a problem
during unescaping, when the reader expects a valid UTF-8 string.
svEscape() is in no place to signal a sensible error, so proceed the
best it can, by escaping.
(cherry picked from commit 5877928b46)
src/libnm-glib-aux/nm-random-utils.c:112:12: error: ignoring return value of 'getrandom' declared with attribute 'warn_unused_result' [-Werror=unused-result]
Fixes: 18597e33cb ('glib-aux: also use getrandom() for seeding pseudo random generator')
(cherry picked from commit 11d59f81cf)
During nm_device_unrealize(), we first clear the device's ifindex. Then
we call _set_state_full(NM_DEVICE_STATE_UNMANAGED).
NMVpnConnection are subclasses of NMActiveConnection, it is that way
connected to NM_DEVICE_STATE_CHANGED signal. And this leads to a call
to _set_vpn_state(), which then calls nm_device_replace_vpn6_config()
to unregister the config. Thereby an assertion fails because the
ifindex no longer matches.
Fix that by relaxing the assertion. Also, don't apply the IP
configuration in unexpected device states.
https://bugzilla.redhat.com/show_bug.cgi?id=1912423https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/927
(cherry picked from commit 63a248ecb2)
NMDeviceModem has priv->modem set from construction to dispose, and
the NM_MODEM_IP4_CONFIG_RESULT/NM_MODEM_IP6_CONFIG_RESULT signals
connected all the time.
On the other hand, NMModem may hook up to NMPPPManager's
NM_PPP_MANAGER_SIGNAL_IP{4,6}_CONFIG signals, which emit the
config-results signals. And PPP manager emits the config signals
from impl_ppp_manager_set_ip{4,6}_config().
That means, at any moment can be a D-Bus calls, which leads to emitting
those signals and calling into modem_ip4_config_result() and
modem_ip6_config_result().
At least, it's not clear from review what would prevent that from
happening. If you cannot easily verify that certain conditions are
satisfied, then this is not the place to assert, but to handle the case
as something that can happen regularly.
Handle signals in the unexpected state by ignoring them.
https://bugzilla.redhat.com/show_bug.cgi?id=1916192https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/926
(cherry picked from commit fa786e1f46)
- the return value of getrandom() is ssize_t.
- handle EAGAIN to indicate low entropy.
- treat a return value of zero the same as any other
low "n", by falling back to bad random bytes.
(cherry picked from commit 416f97c5a9)
We make an effort to get a better fallback case with
_bad_random_bytes().
Also make an effort to get good randomness in the first place. Even if
we compile against libc headers that don't provide getrandom(). Also,
this isn't really ugly, because for a long time glibc was reluctant to
add getrandom() wrapper and using syscall() was the way to go.
(cherry picked from commit 05a6936bef)
nm_utils_random_bytes() tries to get good randomness. If it fails, we still
try our own approach, but also signal that the returned numbers are bad.
In practice, none of the callers cares about the return value, because they
wouldn't know what to do in case of bad randomness (abort() is not an
option and retry is not expected to help and sending an email to the
admin isn't gonna help either). So the fallback case really should try
its best.
The fallback case depends on a good random seed and a good pseudorandom
number generator.
Getting a good seed is in reality impossible, after kernel let us down.
That is part of the problem, but we try our best.
The other part is to use a cryptographic pseudorandom number generator.
GRand uses a Mersenne Twister, so that is not good enough. In this low
level code we also cannot call gnutls/nss, because usually we don't have
that dependency. Maybe we could copy&paste the chacha20 implementation,
it's simple enough and a compatible license. That might be good, but
instead cock our own by adding some sha256 into the mix. This is
fallback code after all, and we want to try hard, but not *that* hard to
add chacha20 to NetworkManager.
So, what we do is to use a well seeded GRand instance, and XOR that
output with a sha256 digest of the state. It's probably slow, but
performance is not the issue in this code path.
(cherry picked from commit c22c3ce9f9)
If a prefix delegation is needed, currently NM restarts DHCPv6 on the
device with default route, but only if DHCPv6 was already running.
Allow the device to start DHCPv6 for a PD even if it was running
without DHCPv6.
See also: https://github.com/coreos/fedora-coreos-tracker/issues/888
(cherry picked from commit 62869621bd)
Previously we sent announcements immediately for non-controllers, or
after the first port was attached for controllers.
This has two problems:
- announcements can be sent when there is no carrier and they would
be lost;
- if a controller has a port, the port could be itself a controller;
in that case we start sending ARPs with the fake address of the
port. Later, when a leaf port is added to the second-level
controller, the correct port MAC will be propagated by kernel up to
both controllers.
To solve both problems, send ARP announcements only when the interface
has carrier. This also solves the second issue because controllers
created by NM have carrier only when there is a port with carrier.
Fixes: de1022285a ('device: do ARP announcements only after masters have a slave')
https://bugzilla.redhat.com/show_bug.cgi?id=1956793
(cherry picked from commit 1377f160ed)
I missed that we already have a gettid() wrapper. Drop the duplicated
again and use nm_utils_gettid().
Fixes: e874c5bf6b ('random: Provide missing gettid() declaration')
(cherry picked from commit 0c4a65929c)
If the kernel command-line doesn't contain an explict ip=$method,
currently the generator creates connections with both IPv4 and IPv6
set to 'auto', and both allowed to fail.
Since NM is run in configure-and-quit mode in the initrd, NM can get
an IPv4 address or an IPv6 one (or both) depending on which address
family is quicker to complete. This unpredictable behavior is not
present in the legacy module, which always does IPv4 only by default.
Set a required-timeout of 20 seconds for IPv4, so that NM will
preferably get an IPv4, or will fall back to IPv6.
See also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/729
(cherry picked from commit 0a18e97345)
Change the logic in check_ip_state() to delay the connection ACTIVATED
state if an address family is pending and its required-timeout has not
expired.
(cherry picked from commit 35cccc41cb)
Add a new property to specify the minimum time interval in
milliseconds for which dynamic IP configuration should be tried before
the connection succeeds.
This property is useful for example if both IPv4 and IPv6 are enabled
and are allowed to fail. Normally the connection succeeds as soon as
one of the two address families completes; by setting a required
timeout for e.g. IPv4, one can ensure that even if IP6 succeeds
earlier than IPv4, NetworkManager waits some time for IPv4 before the
connection becomes active.
(cherry picked from commit cb5960cef7)
g_rand_new() reads /dev/urandom and falls back to timestamp and pid.
At this point, we already unsuccessfully tried getrandom()/urandom,
so that doesn't seem promising to try.
Try harder to get good random seeds for our GRand instance.
Have one global instance, that gets seeded with various things that come
to mind. The random sequence of that instance is then used to initialize
the thread-local GRand instances.
Maybe this is all snake oil. If we fail to get good randomness by using
kernel API, what can we do? But really, callers also don't know how they
should handle a failure to get random data (short of abort() or
logging), so there is value in nm_utils_random_bytes() trying really
the best it can, and callers pretending that it doesn't fail.
This aims to improve the fallback case.
(cherry picked from commit 3649efe2b5)
The previous implementation was just wrong.
Fixes: e1ca3bf7ed ('shared: add nm_strerror_native() to replace strerror() and g_strerror()')
(cherry picked from commit 5bc39d9783)
_nm_thread_local is very neat, but when we allocate resources
we need to make sure that they are destroyed when the thread
exits.
We can use pthread_setspecific() for that, but using it is cumbersome.
Add a helper function to make that simpler.
Also, the number of possible pthread_key_t keys is limited. With this
way, we only need one key in total.
(cherry picked from commit bec4a40437)
We require these, otherwise we can get a linker error about
_nm_utils_monotonic_timestamp_initialized symbol being undefined.
(cherry picked from commit 09fb7877a9)
We have two GKeyfile files (timestamps and seen-bssids).
When a profile was deleted while NetworkManager was running, then
entries were removed from these keyfiles. But if a profile disappeared
while NetworkManger was stopped, then those UUIDs piled up.
This also happens if you have temporary connections in /run and reboot.
We need a way to garbage collect entries that are no longer relevant.
As the keyfile databases only get loaded once from disk, we will prune
all UUIDs for which we have no more connection loaded, on the first time
we write out the files again.
Note what this means: if you "temporarily" remove a connection profile
(without NetworkManager noticing) and restore it later, then the additional
information might have been pruned. There is no way how NetworkManager
could know that this UUID is coming back. The alternative is what we did
before: pile them up indefinitely. That seems more problematic.
(cherry picked from commit 2e720a1dc8)
Previously, there was no limit how many seen-bssids are tracked.
That seems problematic, also because there is no API how to get
rid of an excessive list of entries.
We should limit the number of entries. Add an (arbitrary) limit
of 30.
But this means that we drop the surplus of entries, and for that it
seems important to keep the newest, most recently seen entries.
Previously, entries were merely sorted ASCIIbetically. Now, honor
their order (with most recently seen first).
Also, normalize the BSSIDs. From internal code, we should only get
normalize strings, but when we load them from disk, they might be bogus.
As we might cut of the list, we don't want that invalid entries
cut of valid ones. And of course, invalid entries make no sense at
all.
(cherry picked from commit 8278719840)
ifcfg-rh plugin never stored the seen bssid list to file, and
keyfile no longer does, and it's no longer parsed from GVariant.
So there is actually no way how anything could be set here.
The seen-bssids should only be populate from
"/var/lib/NetworkManager/seen-bssids". Nowhere else.
(cherry picked from commit 15a0271781)
