Hello Mimi, thanks for feedback.
25.02.2020 16:44, Mimi Zohar пишет:
> On Sun, 2020-02-16 at 14:10 +0300, Mikhail Novosyolov wrote:
>> LibreSSL in most cases can be used as a drop-in replacement of OpenSSL.
>> Commit 07d799cb6c37 "ima-evm-utils: Preload OpenSSL engine via '--engine' option"
>> added OpenSSL-specific functions: "engines" were removed from LibreSSL long ago.
>> Instead of requiring to attach GOST support via an external library ("engine"),
>> LibreSSL has build-in implementation of GOST.
>
> OpenSSL had a builtin support for GOST, which was dropped. From the
> OpenSSL news "Changes between 1.0.2h and 1.1.0":
>
> The GOST engine was out of date and therefore it has been removed. An up
> to date GOST engine is now being maintained in an external repository.
> See: https://wiki.openssl.org/index.php/Binaries ; . Libssl still retains
> support for GOST ciphersuites (these are only activated if a GOST engine
> is present).
>
> Please update the patch description to reflect the reason for OpenSSL
> dropping GOST builtin support, while LibreSSL continues to build it
> in.
The reasons why OpenSSL decided to do it are out of my scope, I can just write that OpenSSL had GOST, then dropped it, then gost-engine appeared as an OpenSSL plugin and that LibreSSL has GOST built in and dropped engines API after forking from OpenSSL. Will it be OK?
>
>> Commit ebbfc41ad6ba "ima-evm-utils: try to load digest by its alias" is also not OK
>> for LibreSSL because LibreSSL uses different digest names:
>> md_gost12_256 -> streebog256
>> md_gost12_512 -> streebog512
>>
>> Example how it works when linked with LibreSSL:
>> $ libressl dgst -streebog256 testfile
>> streebog256(a)= 04123f539a213e97c802cc229d474c6aa32a825a360b2a933a949fd925208d9ce1bb
>> $ evmctl -v ima_hash -a streebog256 testfile
>> hash(streebog256): 04123f539a213e97c802cc229d474c6aa32a825a360b2a933a949fd925208d9ce1bb
>> $ evmctl -v ima_hash -a md_gost12_256 testfile
>> EVP_get_digestbyname(md_gost12_256) failed
>
> Removing "engine support" is one logical change. This sounds like it
> is a separate issue and should be addressed in its own patch.
LibreSSL removed engine API completely, but seems to keep the API to continue working as a drop in replacement of OpenSSL. There are no engines in LibreSSL, printing information about them in --help and trying to load them does not make sense and will confuse users. The main purpose of this patch is not to fix _buildability_ with LibreSSL, but to fix logical mistakes which appear when using LibreSSL.
>>
>> TODO: it would be nice to map
>> md_gost12_256 <-> streebog256
>> md_gost12_512 <-> streebog512
>> in evmctl CLI arguements to make the same commands work on systems both
>> where evmctl is linked with LibreSSL and with OpenSSL.
>>
>> Fixes: 07d799cb6c37 ("ima-evm-utils: Preload OpenSSL engine via '--engine' option")
>> Fixes: ebbfc41ad6ba ("ima-evm-utils: try to load digest by its alias")
>> Signed-off-by: Mikhail Novosyolov <m.novosyolov@xxxxxxxxxxxx>
>> ---
>> README | 2 +-
>> src/evmctl.c | 15 ++++++++++++++-
>> src/libimaevm.c | 2 ++
>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/README b/README
>> index 3603ae8..f843bbe 100644
>> --- a/README
>> +++ b/README
>> @@ -58,7 +58,7 @@ OPTIONS
>> --smack use extra SMACK xattrs for EVM
>> --m32 force EVM hmac/signature for 32 bit target system
>> --m64 force EVM hmac/signature for 64 bit target system
>> - --engine e preload OpenSSL engine e (such as: gost)
>> + --engine e preload OpenSSL engine e (such as: gost) (not valid for LibreSSL)
>> -v increase verbosity level
>> -h, --help display this help and exit
>>
>> diff --git a/src/evmctl.c b/src/evmctl.c
>> index 3d2a10b..f6507c1 100644
>> --- a/src/evmctl.c
>> +++ b/src/evmctl.c
>> @@ -62,7 +62,10 @@
>> #include <openssl/hmac.h>
>> #include <openssl/err.h>
>> #include <openssl/rsa.h>
>> +/* LibreSSL removed engines */
>> +#ifndef LIBRESSL_VERSION_NUMBER
>> #include <openssl/engine.h>
>> +#endif
>
> According to the LibreSSL wiki, both OpenSSL and LibreSSL may be
> installed on the same system in separate directories. Instead of
> using LIBRESSL_VERSION_NUMBER, consider defining an autotools option.
LibreSSL can be used either as a drop in replacement of OpenSSL or can be installed to a separate prefix.
What do you suggest to do with an autotools option? To define a prefix where libssl/libcrypto is, e.g. /usr or /opt/libressl? It may be useful. But, in my experience of building curl and other programs with LibreSSL from a custom prefix, any heaurestics in configure.ac cause much more troubles than profit, I had to hack curl's configure.ac to stop it from picking OpenSSL.
Right now the only line which detect libcrypto is "PKG_CHECK_MODULES(LIBCRYPTO, [libcrypto >= 0.9.8 ])". If we make an autotools option, we will have to somehow handle situation when headers and/or pkgconfig files are not in the prefix, like curl does (https://github.com/curl/curl/blob/master/configure.ac#L1642). Let's avoid such a bicycle. I had to hack it to build curl with libressl, and many people on the Internet complain that it works incorrectly in case of cross-compilation. The same problems will occure in ima-evm-utils.
Right now I build ima-evm-utils like this:
export LIBCRYPTO_CFLAGS="$(pkg-config --cflags-only-I --libs-only-L libressl-libcrypto)"
...where libressl-libcrypto is a not upstream name of the *.pc file, I renamed it from libcrypto.pc to libressl-libcrypto.pc. It works perfectly. There is no need in inventing a bicycle in configure.ac, I am pretty sure.
LIBRESSL_VERSION_NUMBER is defined in LibreSSL only and so to my mind is a good method of build-time detection of which library is being used. Many software uses it, see https://codesearch.debian.net/search?q=LIBRESSL_VERSION_NUMBER&literal=1
Even if we make an autotools option which sets a prefix for Open/LibreSSL, I think it is better to continue using LIBRESSL_VERSION_NUMBER in #ifdef's because otherwise there will be a lot of confusion when ima-evm-utils is built against a custom ssl library in a custom prefix but when the builder does not manually define that it is libressl.
If I misunderstood your idea about an autotools option, please correct me.
> thanks,
>
> Mimi
>
>>
>> #ifndef XATTR_APPAARMOR_SUFFIX
>> #define XATTR_APPARMOR_SUFFIX "apparmor"
>> @@ -1849,7 +1852,9 @@ static void usage(void)
>> " --selinux use custom Selinux label for EVM\n"
>> " --caps use custom Capabilities for EVM(unspecified: from FS, empty: do not use)\n"
>> " --list measurement list verification\n"
>> +#ifndef LIBRESSL_VERSION_NUMBER /* LibreSSL removed engines */
>> " --engine e preload OpenSSL engine e (such as: gost)\n"
>> +#endif
>> " -v increase verbosity level\n"
>> " -h, --help display this help and exit\n"
>> "\n");
>> @@ -1902,7 +1907,9 @@ static struct option opts[] = {
>> {"selinux", 1, 0, 136},
>> {"caps", 2, 0, 137},
>> {"list", 0, 0, 138},
>> +#ifndef LIBRESSL_VERSION_NUMBER
>> {"engine", 1, 0, 139},
>> +#endif
>> {"xattr-user", 0, 0, 140},
>> {}
>>
>> @@ -1947,7 +1954,9 @@ static char *get_password(void)
>> int main(int argc, char *argv[])
>> {
>> int err = 0, c, lind;
>> +#ifndef LIBRESSL_VERSION_NUMBER
>> ENGINE *eng = NULL;
>> +#endif
>>
>> #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
>> OPENSSL_init_crypto(
>> @@ -2065,7 +2074,8 @@ int main(int argc, char *argv[])
>> case 138:
>> measurement_list = 1;
>> break;
>> - case 139: /* --engine e */
>> +#ifndef LIBRESSL_VERSION_NUMBER
>> + case 139: /* --engine e, only in OpenSSL, not in LibreSSL */
>> eng = ENGINE_by_id(optarg);
>> if (!eng) {
>> log_err("engine %s isn't available\n", optarg);
>> @@ -2078,6 +2088,7 @@ int main(int argc, char *argv[])
>> }
>> ENGINE_set_default(eng, ENGINE_METHOD_ALL);
>> break;
>> +#endif
>> case 140: /* --xattr-user */
>> xattr_ima = "user.ima";
>> xattr_evm = "user.evm";
>> @@ -2108,6 +2119,7 @@ int main(int argc, char *argv[])
>> }
>> }
>>
>> +#ifndef LIBRESSL_VERSION_NUMBER
>> if (eng) {
>> ENGINE_finish(eng);
>> ENGINE_free(eng);
>> @@ -2115,6 +2127,7 @@ int main(int argc, char *argv[])
>> ENGINE_cleanup();
>> #endif
>> }
>> +#endif
>> ERR_free_strings();
>> EVP_cleanup();
>> BIO_free(NULL);
>> diff --git a/src/libimaevm.c b/src/libimaevm.c
>> index 7c17bf4..050ea78 100644
>> --- a/src/libimaevm.c
>> +++ b/src/libimaevm.c
>> @@ -71,8 +71,10 @@ static const char *const pkey_hash_algo[PKEY_HASH__LAST] = {
>> [PKEY_HASH_SHA384] = "sha384",
>> [PKEY_HASH_SHA512] = "sha512",
>> [PKEY_HASH_SHA224] = "sha224",
>> +#ifndef LIBRESSL_VERSION_NUMBER
>> [PKEY_HASH_STREEBOG_256] = "md_gost12_256",
>> [PKEY_HASH_STREEBOG_512] = "md_gost12_512",
>> +#endif
>> };
>>
>> /* Names that are primary for the kernel. */
>
