summaryrefslogtreecommitdiff
Commit message (Collapse)AuthorAgeFilesLines
* ifcfg-rh: don't use 802-1x certifcate setter functionsth/crypto-secretsThomas Haller2018-09-034-210/+207
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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.
* ifcfg-rh: rework parsing secretsThomas Haller2018-09-031-79/+130
| | | | | | | | | | | | | | | | - 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.
* ifcfg-rh/trivial: rename variable for ifcfg keys fileThomas Haller2018-09-031-59/+59
| | | | | | | | 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.
* libnm-core: expose _nm_utils_str2bin_full() as internal APIThomas Haller2018-09-032-10/+17
| | | | | | | | | 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.
* libnm/keyfile: clear memory when reading certificates from keyfileThomas Haller2018-09-031-20/+33
| | | | | | | | | 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.
* libnm/802-1x: refactor getting private-key formatThomas Haller2018-09-031-67/+63
| | | | Move similar code to a helper function/macro.
* libnm/802-1x: refactor setting certificate from pathThomas Haller2018-09-033-506/+243
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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.
* libnm/802-1x: cleanup NMSetting8021x:verify()Thomas Haller2018-09-031-25/+62
|
* libnm/802-1x: don't verify certificates in GObject property setterThomas Haller2018-09-031-49/+6
| | | | | | | | | | | | 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.
* libnm/802-1x: refactor certificate handling in settingsThomas Haller2018-09-031-190/+94
| | | | | | | - all this code duplication. Add functions and macros to simplify the implementation of certificate properties. Overall, pretty trival. Replace code with a macro.
* libnm/802-1x: call g_bytes_unref() directly without checking for NULLThomas Haller2018-09-031-29/+14
| | | | Since its inception, g_bytes_unref() is fine with NULL argument.
* libnm/802-1x/trivial: reorder codeThomas Haller2018-09-031-505/+514
|
* libnm/802-1x: refactor GObject properties of NMSetting8021xThomas Haller2018-09-031-323/+280
|
* libnm/crypto: mark nm_crypto_make_des_aes_key() as test-only functionThomas Haller2018-09-032-15/+15
|
* libnm/crypto: clean crypto implementations for gnutls/nssThomas Haller2018-09-032-181/+171
| | | | | | - refactor to use cleanup attribute and return-early - reorder some code
* libnm/crypto: rework endianness detection for crypto_verify_pkcs12()Thomas Haller2018-09-034-31/+16
| | | | | | | | 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.
* libnm/crypto: refactor to use enum for supported ciphersThomas Haller2018-09-035-134/+190
| | | | | | | | | 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.
* libnm/crypto: remove unused argument key_type for decrypt functionsThomas Haller2018-09-034-6/+0
|
* libnm/crypto: don't initialize buffer for nm_crypto_make_des_aes_key() with zeroThomas Haller2018-09-031-1/+1
| | | | | | | | @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.
* libnm/crypto: adjust signature of crypto functionsThomas Haller2018-09-035-67/+67
| | | | | | | - 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.
* libnm/crypto: rename crypto functions used for testing onlyThomas Haller2018-09-035-110/+88
| | | | | | | | - 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.
* libnm: cleanup conversion from NMCryptoFileFormat to NMSetting8021xCKFormat enumThomas Haller2018-09-031-7/+21
|
* libnm: fix race in nm-setting-x802-1x's setting private key functionsThomas Haller2018-09-031-69/+119
| | | | | | | | | | | | | | | | | | | 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.
* libnm: clear private-key passwords in NMSetting8021xThomas Haller2018-09-031-8/+9
| | | | | Yes, there are countless other places where we don't get this right and leave sensitive data in memory. Anyway, fix these places.
* libnm/keyfile: avoid GByteArray to construct path uri in ↵Thomas Haller2018-09-031-10/+9
| | | | nm_keyfile_detect_unqualified_path_scheme()
* libnm: avoid intermediate GByteArray in path_to_scheme_value()Thomas Haller2018-09-031-8/+7
| | | | | It's not that to directly initialize the GBytes without an intermediate GByteArray.
* libnm/crypto: fix loading certificates from file securelyThomas Haller2018-09-033-28/+18
| | | | | | | | 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.
* libnm/crypto: ensure not leaking sensitive information when loading filesThomas Haller2018-09-031-1/+8
| | | | | | | | | | | | | | 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.
* libnm/crypto: refactor nmtst_crypto_rsa_key_encrypt() and clear memoryThomas Haller2018-09-033-64/+67
| | | | | | | | 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).
* libnm/crypto: move and mark nm_utils_rsa_key_encrypt() as test codeThomas Haller2018-09-035-115/+117
| | | | | | 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.
* libnm-core/trivial: rename testing related functions in crypto codeThomas Haller2018-09-032-30/+36
| | | | | | | | | | | | | | | | 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.
* libnm/crypto: refactor crypto test functions to return GBytesThomas Haller2018-09-033-68/+38
| | | | | Using GBytes consistently simplifies the code. Also use it for the test related functions.
* libnm/crypto: refactor nm_crypto_load_and_verify_certificate() and return GBytesThomas Haller2018-09-035-73/+71
| | | | | | | | | | 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.
* libnm/crypto: use nm_explicit_bzero() instead of plain memset()Thomas Haller2018-09-032-10/+9
|
* libnm/crypto: rework _nm_crypto_verify_cert() to return booleanThomas Haller2018-09-034-28/+48
| | | | | | | | | 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.
* build/travis: build both against crypto "gnutls" and "nss"Thomas Haller2018-09-031-1/+12
| | | | | | | 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.
* build: enable building both crypto backends for testsThomas Haller2018-09-035-34/+110
| | | | | | | | | | | | 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.
* libnm/crypto: rename libnm crypto API to have consistent NM prefixThomas Haller2018-09-038-262/+295
| | | | | | | | 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.
* libnm/crypto: add header "nm-crypto-impl.h" for crypto implementationThomas Haller2018-09-039-33/+69
| | | | | | 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.
* libnm/crypto: rename libnm's crypto filesThomas Haller2018-09-0312-22/+24
| | | | | "crypto.h" did not follow our common NM style naming. Rename the files.
* libnm/crypto: refactor decrypt_key() to use NMSecretPtrThomas Haller2018-09-031-29/+38
|
* libnm/crypto: refactor parse_pkcs8_key_file() to bzero loaded dataThomas Haller2018-09-031-24/+20
|
* libnm/crypto: refactor parse_old_openssl_key_file() to bzero loaded dataThomas Haller2018-09-031-63/+99
| | | | | Ensure that data processed by parse_old_openssl_key_file() is cleared from memory.
* libnm/crypto: clear data loaded from filesThomas Haller2018-09-031-57/+71
| | | | | | | | 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.
* libnm/crypto: rename crypto functions that are only used by testsThomas Haller2018-09-033-25/+25
| | | | | These functions are only used by tests, hence they are much less important. Mark them as such, by naming them accordingly.
* libnm/crypto: cleanup convert_iv() and handle more errorsThomas Haller2018-09-031-22/+37
| | | | | | | | | | | | 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.
* libnm/crypto: adjust argument types for crypto_md5_hash()Thomas Haller2018-09-034-26/+35
| | | | | | | | There should be a clear distinction between whether an array is a NUL terminated string or binary with a length. crypto_md5_hash() is already complicated enough. Adjust it's API to only support binary arguments, and thus have "guint8 *" type.
* libnm/crypto: cleanup error paths and use cleanup-attributeThomas Haller2018-09-031-157/+114
|
* libnm/crypto: minor cleanup confusing comment in ↵Thomas Haller2018-09-031-6/+4
| | | | | | | | | | | crypto_decrypt_openssl_private_key_data() the comment and code made it sound like parse_old_openssl_key_file() would set @key_type if the parsing was only done partially. That is not the case, @key_type is only set, if parsing was successful. Adjust the code. While at it, don't require the caller to initialize @out_key_type. It's just an enum, if we care to always set it, just do it.
* shared: rename PROP_0 in NM_GOBJECT_PROPERTIES_DEFINE() and skip it in ↵Thomas Haller2018-09-031-3/+6
| | | | | | | | | | | | | | nm_gobject_notify_together() PROP_0 is how we commonly name this property when we don't use NM_GOBJECT_PROPERTIES_DEFINE(). Rename it. Also, allow to skip PROP_0 in nm_gobject_notify_together(), that is handy to optionally invoke a notification, like nm_gobject_notify_together (obj, PROP_SOMETHING, changed ? PROP_OTHER : PROP_0);
View Lorry Controller Status