Calling g_hash_table_insert() with a key which is already hashed
will destroy the *new* key. Since @id is used below, that would
be use after free.
Fixes: d635caf940551f8f5b52683b8379a1f81c58f8fc
IWD's mechanism for connecting to EAP networks requires a network config
file to be present in IWD's storage. NM and its clients however won't
allow a connection to be attempted until a valid NMConnection is created
on the NM side for the network. To avoid duplicating the settings from
the IWD-side profiles in NM, automatically create NMSettingConnections
for EAP networks preconfigured on the IWD side, unless a matching
connection already exists. These connections will use the "external"
EAP method to mean their EAP settings can't be modified through NM, also
they won't be valid for devices configured to use the wpa_supplicant
backend unfortunately.
Those nm-generated connections can be modified by NM users (makes sense
for settings not related to the wifi authentication) in which case they
get saved as normal profiles and will not be recreated as nm-generated
connections on the next run.
I want to additionally handle deleting connections from NM clients so
that they're also forgotten by IWD, in a later patch.
To allow connections that mirror IWD's configured WPA-Enterprise
networks to be seen as valid by NM, add a new value for the eap key in
802-1x settings. 802-1x.eap stores EAP method names. In the IWD
connections we don't know what EAP method is configured and we don't
have any of the other 802-1x properties that would be required for the
settings to verify.
These connections can't be activated on devices managed by wpa_supplicant.
Make this function public. I'm not sure if at this point it makes
much sense to add a new file for iwd-specific utilities.
While there add a way for the function to return error if security
type can't be mapped to an IWD-supported security type.
The known networks hash table is indexed by the (ssid, security) tuple
for fast lookups both on DBus signals related to an IWD known network
and local NMConnection signals such as on removal.
There's no need anymore for NMIwdManager to know when a network has been
connected to, InterfaceAdded signals are now emitted when a network is
saved as a Known Network.
Before 0.5 IWD has changed the known networks API to expose separate
objects for each known network and dropped the KnownNetworks
manager-like interface so stop using that interface. Following
patches will add tracking of the known networks through
ObjectManager.
Instead of passing the return values to g_variant_get_string or
g_variant_boolean and then checking the return value of that call,
add wrappers that first check's whether the variant is non-NULL
and of the right type.
g_variant_get_string doesn't allow a NULL parameter and will also never
return NULL according to the docs.
For the State property we assume a state "unknown" and emit a warning
if the property can't be read, "unknown" is also a string in IWD itself
which would be returned if something went really wrong. In any case
this shouldn't happen.
[thaller@redhat.com: fix missing initialization of nm_auto() variable
interfaces.]
The certificate setter function like nm_setting_802_1x_set_ca_cert()
actually load the file from disk, and validate whether it is a valid
certificate. That is very wrong to do.
For one, the certificates are external files, which are not embedded
into the NMConnection. That means, strongly validating the files while
loading the ifcfg files, is wrong because:
- if validation fails, loading the file fails in its entirety with
a warning in the log. That is not helpful to the user, who now
can no longer use nmcli to fix the path of the certificate (because
the profile failed to load in the first place).
- even if the certificate is valid at load-time, there is no guarantee
that it is valid later on, when we actually try to use the file. What
good does such a validation do? nm_setting_802_1x_set_ca_cert() might
make sense during nmcli_connection_modify(). At the moment when we
create or update the profile, we do want to validate the input and
be helpful to the user. Validating the file later on, when reloading
the profile from disk seems undesirable.
- note how keyfile also does not perform such validations (for good
reasons, I presume).
Also, there is so much wrong with how ifcfg reader handles EAP files.
There is a lot of duplication, and trying to be too smart. I find it
wrong how the "eap_readers" are nested. E.g. both eap_peap_reader() and
"tls" method call to eap_tls_reader(), making it look like that
NMSetting8021x can handle multiple EAP profiles separately. But it cannot. The
802-1x profile is a flat set of properties like ca-cert and others. All
EAP methods share these properties, so having this complex parsing
is not only complicated, but also wrong. The reader should simply parse
the shell variables, and let NMSetting8021x::verify() handle validation
of the settings. Anyway, the patch does not address that.
Also, the setting of the likes of NM_SETTING_802_1X_CLIENT_CERT_PASSWORD was
awkwardly only done when
privkey_format != NM_SETTING_802_1X_CK_FORMAT_PKCS12
&& scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11
It is too smart. Just read it from file, if it contains invalid data, let
verify() reject it. That is only partly addressed.
Also note, how writer never actually writes the likes of
IEEE_8021X_CLIENT_CERT_PASSWORD. That is another bug and not fixed
either.
- rename secret related functions to have a "_secret" prefix.
Also, rename read_8021x_password() because it's not only useful
for 802-1x.
- In particular, this patch adds _secret_read_ifcfg() helper (formerly
read_8021x_password()), which is smart enough to obtain secrets from
the keys ifcfg file. We have other places where we don't get this
right.
- on a minor note, the patch also makes an effort to clear passwords
and certifcate data from memory. Yes, there are countless places
where we don't do that, but in this case, it's done and is as simple
as replacing gs_free with nm_auto_free_secret, etc.
The term "keys" is used ambigiously. Rename occurances which reference
the "keys" ifcfg-rh file.
While at it, rename the file "parsed" to "main_ifcfg". It follows the
same pattern as the "keys_ifcfg" name.
We only exposed wrappers around this function, but all of them have
different behavior, and none exposes all possible features. For example,
nm_utils_hexstr2bin() strips leading "0x", but it does not clear
the data on failure (nm_explicit_bzero()). Instead of adding more
wrappers, expose the underlying implementation, so that callers may
use the function the way they want it.
Of course, there are countless places where we get it wrong to clear
the memory. In particular, glib's GKeyFile implementation does
not care about that.
Anyway, the keyfile do contain private sensitive data. Adjust
a few places to zero out such data from memory.
NMSetting8021x has various utility functions to set
the certificate:
- nm_setting_802_1x_set_ca_cert()
- nm_setting_802_1x_set_client_cert()
- nm_setting_802_1x_set_private_key()
- nm_setting_802_1x_set_phase2_ca_cert()
- nm_setting_802_1x_set_phase2_client_cert()
- nm_setting_802_1x_set_phase2_private_key()
They support:
- accepting a plain PKCS11 URI, with scheme set to
NM_SETTING_802_1X_CK_SCHEME_PKCS11.
- accepting a filename, with scheme set to
NM_SETTING_802_1X_CK_SCHEME_BLOB or
NM_SETTING_802_1X_CK_SCHEME_PATH.
In the latter case, the function tries to load the file and verify it.
In case of the private-key setters, this also involves accepting a
password. Depending on whether the scheme is BLOB or PATH, the function
will either set the certificate to a PATH blob, or take the blob that
was read from file.
The functions seem misdesigned to me, because their behavior is
rather obscure. E.g. they behave fundamentally different, depending
on whether scheme is PKCS11 or BLOB/PATH.
Anyway, improve them:
- refactor the common code into a function _cert_impl_set(). Previously,
their non-trivial implementations were copy+pasted several times,
now they all use the same implementation.
- if the function is going to fail, don't touch the setting. Previously,
the functions would first clear the certificate before trying to
validate the input. It's more logical, that if a functions is going
to fail to check for failure first and don't modify the settings.
- not every blob can be represented. For example, if we have a blob
which starts with "file://", then there is no way to set it, simply
because we don't support a prefix for blobs (like "data:;base64,").
This means, if we try to set the certificate to a particular binary,
we must check that the binary is interpreted with the expected scheme.
Add this check.
First of all, g_warning() is not a suitable error handling. In
particular, note how this code is reached when obtaining a setting
from D-Bus, that is, the user is not at fault.
The proper way to handle this, is allowing the setter to set the invalid
value. Only later, during verify() we will fail. This way,
NetworkManager can extend the format and older libnm clients don't
break. This is how forward-compatibility (with older libnm vs. newer
daemon) is supposed to work.
- all this code duplication. Add functions and macros to simplify
the implementation of certificate properties.
Overall, pretty trival. Replace code with a macro.
At other places, we already use __BYTE_ORDER define to detect endianness.
We don't need multiple mechanisms.
Also note that meson did not do the correct thing as AC_C_BIGENDIAN,
so meson + nss + big-endian was possibly broken.
We need to (and already did) define our own identifier for ciphers,
because the gnutls/nss identifiers must be abstracted.
Don't use a string for that. The number of supported ciphers
is not generic but fixed and known at compiler time. An enum
is better suited.
@key is directly passed to nm_crypto_md5_hash(), which cannot (by API design)
fail. No need to initialize it.
Also, no need to allocate an additional trailing NUL byte. The key is
binary, every attempt to use it as a string will horribly fail.
- avoid "const gsize" as type for function arguments.
- consistently use "guint8 *" type for binary data, instead
of "char *", which indicates a NUL terminated C string.
- drop nm_crypto_encrypt(). It's not actually used outside of
"nm-crypto.c".
- rename internal _nm_crypto_*() functions that are only used
in tests. It's so much nicer to visually recognize functions
that are used for testing only.
Do not first load the file during nm_crypto_verify_private_key(),
and later re-load it, in case we are setting a blob. Instead,
ensure we only load the file once.
This fixes a race, and also the very wrong assertion:
priv->phase2_private_key = nm_crypto_read_file (value, NULL);
nm_assert (priv->phase2_private_key);
We should never assert that an IO operation succeeds.
Also, we encode blobs, paths, and pkcs11 URIs all inside a binary
field. Unfortunately, there is no defined prefix for blobs (TODO).
That means, if you have a blob that happens to start with "file://"
it cannot be expressed. At least, check that the binary field that
we are setting gets detected as correct scheme type.
file_to_secure_bytes() tried to load the file from disk and ensure that
the data will be cleared. It did so poorely, because g_file_get_contents()
cannot be used for that.
Add a helper function nm_crypto_read_file() to get this right.
g_file_get_contents() may use re-alloc to load the file. Each time
it re-allocated the buffer, it does not bother clearing the loaded
buffer from memory.
Alternatively, g_file_get_contents() may use stat() and only allocate
one buffer. But also in this mode, without realloc(), it does not
clear the buffer if reading the file fails with IO error later.
Use nm_utils_file_get_contents() which does that.
While at it, don't load files larger that 100 MB.
It's only used for testing, so this change is not very relevant.
Anyway, I think our crypto code should succeed in not leaving
key material in memory. Refactor the code to do that, though,
how the pem file gets composed is quite a hack (for tests good
enough though).
nm_utils_rsa_key_encrypt() is internal API which is only uesd for testing.
Move it to nm-crypto.h (where it fits better) and rename it to make the
testing-aspect obvious.
In nm-crypto.c we have functions that are only called from tests.
Maybe these functions should move away from libnm-core to the
test.
Leave it, but at least rename them to make it clear that these
functions are not relevant for libnm's actual usage. For a
reviewer that makes a big difference as crypto functions in libnm
have a significantly higher requirement for quality.
There is nothing new here. We already have other *nmtst* functions
beside our regular code. The concention is, that functions that
are only for testing are named explicitly ("nmtst"), and that they
can only be called by test functions themselves.
The GBytes has a suitable cleanup function, which zeros the certificate
from memory.
Also, all callers that require the certificate, actually later converted
it into a GBytes anyway. This way, they can re-used the same instance
(avoiding an additionaly copying of the data), and they will properly
clear the memory when freed.
