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.
Rename _nm_crypto_verify_cert() to _nm_crypto_verify_x509().
Also, don't let it return a NMCryptoFileFormat result. This
function only checks for a particular format, hence it
should only return true/false.
Also, fix setting error output argument when the function fails.
We already do matrix-builds with autotools|meson and gcc|clang.
Make the selected crypto backend depending on the compiler, so
that we get more coverage.
If the library is available, let's at least compile both
crypto backends.
That is helpful when developing on crypto backends, so that
one does not have to configure the build twice.
With autotools, the build is only run during `make check`.
Not for meson, but that is generally the case with our meson
setup, that it also builds tests during the regular build step.
Follow our convention, that items in headers are all named with
an "NM" prefix.
Also, "nm-crypto-impl.h" contains internal functions that are to be implemented
by the corresponding crypto backends. Distinguish their names as well.
There are two aspects: the public crypto API that is provided by
"nm-crypto.h" header, and the internal header which crypto backends
need to implement. Split them.
Data that we load from crypto files should be cleared once it's
no longer used.
Just a small step. There are many other places where we copy the data
and leave it around.
crypto_make_des_aes_key() asserts that iv-lenght is at least
8 characters. Whatever the reason. That means, decrypt_key()
must check for that condition first, and gracefully fail.
Also, don't use strtol() to convert a pair of hex digits to
integer.
Also, don't keep the IV in memory. Yes, it's not very critical,
but this is crypto code, we should not leave data behind.
