| Message ID | 20181126043953.1126-3-vt@altlinux.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] ima-avm-utils: Fix hash buffer overflow in verify_evm | expand |
On Mon, 2018-11-26 at 07:39 +0300, Vitaly Chikunov wrote: > There are two methods of using GOST algorithms in OpenSSL: via config > extension and via --engine option. Both require gost-engine to be > installed. Splitting this patch based on the "--engine" option, will make it easier to review. > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > --- > src/evmctl.c | 27 ++++++++++++++++++++++++--- > src/imaevm.h | 13 +++++++++++++ > src/libimaevm.c | 15 +++++++++++---- > 3 files changed, 48 insertions(+), 7 deletions(-) <snip> > diff --git a/src/imaevm.h b/src/imaevm.h > index 1bafaad..1a5ebbe 100644 > --- a/src/imaevm.h > +++ b/src/imaevm.h > @@ -149,6 +149,7 @@ struct signature_hdr { > char mpi[0]; > } __packed; > > +/* reflect enum hash_algo from include/uapi/linux/hash_info.h */ > enum pkey_hash_algo { > PKEY_HASH_MD4, > PKEY_HASH_MD5, > @@ -158,6 +159,18 @@ enum pkey_hash_algo { > PKEY_HASH_SHA384, > PKEY_HASH_SHA512, > PKEY_HASH_SHA224, > + PKEY_HASH_RIPE_MD_128, > + PKEY_HASH_RIPE_MD_256, > + PKEY_HASH_RIPE_MD_320, > + PKEY_HASH_WP_256, > + PKEY_HASH_WP_384, > + PKEY_HASH_WP_512, > + PKEY_HASH_TGR_128, > + PKEY_HASH_TGR_160, > + PKEY_HASH_TGR_192, > + PKEY_HASH_SM3_256, > + PKEY_HASH_STREEBOG_256, > + PKEY_HASH_STREEBOG_512, > PKEY_HASH__LAST > }; > > diff --git a/src/libimaevm.c b/src/libimaevm.c > index 714f1ac..8f74660 100644 > --- a/src/libimaevm.c > +++ b/src/libimaevm.c > @@ -50,6 +50,7 @@ > #include <string.h> > #include <stdio.h> > > +#include <openssl/crypto.h> > #include <openssl/pem.h> > #include <openssl/evp.h> > #include <openssl/x509.h> > @@ -66,6 +67,8 @@ const char *const pkey_hash_algo[PKEY_HASH__LAST] = { > [PKEY_HASH_SHA384] = "sha384", > [PKEY_HASH_SHA512] = "sha512", > [PKEY_HASH_SHA224] = "sha224", > + [PKEY_HASH_STREEBOG_256] = "streebog256", > + [PKEY_HASH_STREEBOG_512] = "streebog512", > }; hash_info.h is now included in kernel-headers package. Anyone using the hash_algo enumeration defined in hash_info.h, will probably also want to use an associated algorithm name. It would make more sense to keep the hash_algo enumeration, hash_algo_name[], and perhaps the hash_digest_size[] together. Maybe using macros to keep them in sync (eg. kernel_read_file_id/kernel_read_file_str). As new algorithms are added to hash_info.h, nothing would need to be done here in ima-evm-utils, other than updating the maximum digest size. What do you think? Mimi
Mimi, On Tue, Nov 27, 2018 at 06:56:51AM -0500, Mimi Zohar wrote: > On Mon, 2018-11-26 at 07:39 +0300, Vitaly Chikunov wrote: > <snip> > > diff --git a/src/imaevm.h b/src/imaevm.h > > index 1bafaad..1a5ebbe 100644 > > --- a/src/imaevm.h > > +++ b/src/imaevm.h > > @@ -149,6 +149,7 @@ struct signature_hdr { > > char mpi[0]; > > } __packed; > > > > +/* reflect enum hash_algo from include/uapi/linux/hash_info.h */ > > enum pkey_hash_algo { > > PKEY_HASH_MD4, > > PKEY_HASH_MD5, > > @@ -158,6 +159,18 @@ enum pkey_hash_algo { > > PKEY_HASH_SHA384, > > PKEY_HASH_SHA512, > > PKEY_HASH_SHA224, > > + PKEY_HASH_RIPE_MD_128, > > + PKEY_HASH_RIPE_MD_256, > > + PKEY_HASH_RIPE_MD_320, > > + PKEY_HASH_WP_256, > > + PKEY_HASH_WP_384, > > + PKEY_HASH_WP_512, > > + PKEY_HASH_TGR_128, > > + PKEY_HASH_TGR_160, > > + PKEY_HASH_TGR_192, > > + PKEY_HASH_SM3_256, > > + PKEY_HASH_STREEBOG_256, > > + PKEY_HASH_STREEBOG_512, > > PKEY_HASH__LAST > > }; > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > index 714f1ac..8f74660 100644 > > --- a/src/libimaevm.c > > +++ b/src/libimaevm.c > > @@ -50,6 +50,7 @@ > > #include <string.h> > > #include <stdio.h> > > > > +#include <openssl/crypto.h> > > #include <openssl/pem.h> > > #include <openssl/evp.h> > > #include <openssl/x509.h> > > @@ -66,6 +67,8 @@ const char *const pkey_hash_algo[PKEY_HASH__LAST] = { > > [PKEY_HASH_SHA384] = "sha384", > > [PKEY_HASH_SHA512] = "sha512", > > [PKEY_HASH_SHA224] = "sha224", > > + [PKEY_HASH_STREEBOG_256] = "streebog256", > > + [PKEY_HASH_STREEBOG_512] = "streebog512", > > }; > > hash_info.h is now included in kernel-headers package. I think, first it should not be coincided with the new algo adding and being follow-up patch (if any). > Anyone using the hash_algo enumeration defined in hash_info.h, will > probably also want to use an associated algorithm name. It would make > more sense to keep the hash_algo enumeration, hash_algo_name[], and > perhaps the hash_digest_size[] together. Maybe using macros to keep > them in sync (eg. kernel_read_file_id/kernel_read_file_str). On the first sight this sounds good, but... It sounds like it will require patching kernel's hash_info.h, so it will be not possible to transfer smoothly on the new scheme without breaking some compatibility (of the newer ima-evm-utils with older kernels). Also, it is possible that ima-evm-utils is used on the older stable box (where some reliable and stable build system run) which does not have such modification and/or new algorithms in the kernel but willing to generate signatures. So, I think code duplication between projects is good in this case. Some hash algorithms may be wished to be compatible with rsa pkcs1 signature scheme, which is also defined in kernel in crypto/rsa-pkcs1pad.c and in ima-evm-utils in src/libimaevm.c so code duplication and adding algorithms in both sources will happen anyway. Thanks, > As new algorithms are added to hash_info.h, nothing would need to be > done here in ima-evm-utils, other than updating the maximum digest > size. > > What do you think? > > Mimi
On Tue, 2018-11-27 at 16:08 +0300, Vitaly Chikunov wrote: > > hash_info.h is now included in kernel-headers package. > > I think, first it should not be coincided with the new algo adding and > being follow-up patch (if any). > > > Anyone using the hash_algo enumeration defined in hash_info.h, will > > probably also want to use an associated algorithm name. It would make > > more sense to keep the hash_algo enumeration, hash_algo_name[], and > > perhaps the hash_digest_size[] together. Maybe using macros to keep > > them in sync (eg. kernel_read_file_id/kernel_read_file_str). > > On the first sight this sounds good, but... > > It sounds like it will require patching kernel's hash_info.h, so it will > be not possible to transfer smoothly on the new scheme without breaking > some compatibility (of the newer ima-evm-utils with older kernels). Also, > it is possible that ima-evm-utils is used on the older stable box (where some > reliable and stable build system run) which does not have such modification > and/or new algorithms in the kernel but willing to generate signatures. > > So, I think code duplication between projects is good in this case. The sooner the kernel's hash_info.h is updated, the better. For the time being, ima-evm-utils could define these definitions in a separate file that is/isn't included based on autotools/buildtime option. > > Some hash algorithms may be wished to be compatible with rsa pkcs1 > signature scheme, which is also defined in kernel in > crypto/rsa-pkcs1pad.c and in ima-evm-utils in src/libimaevm.c so code > duplication and adding algorithms in both sources will happen anyway. > > Thanks, There might be multiple problems, but fixing one is better than not fixing either. Mimi
diff --git a/src/evmctl.c b/src/evmctl.c index 032ea9d..5b4d8d9 100644 --- a/src/evmctl.c +++ b/src/evmctl.c @@ -62,6 +62,7 @@ #include <openssl/hmac.h> #include <openssl/err.h> #include <openssl/rsa.h> +#include <openssl/engine.h> #ifndef XATTR_APPAARMOR_SUFFIX #define XATTR_APPARMOR_SUFFIX "apparmor" @@ -388,7 +389,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash) md = EVP_get_digestbyname(params.hash_algo); if (!md) { - log_err("EVP_get_digestbyname() failed\n"); + log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo); return 1; } @@ -1056,7 +1057,7 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h md = EVP_get_digestbyname(params.hash_algo); if (!md) { - log_err("EVP_get_digestbyname() failed\n"); + log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo); goto out; } @@ -1643,7 +1644,7 @@ static void usage(void) printf( "\n" - " -a, --hashalgo sha1 (default), sha224, sha256, sha384, sha512\n" + " -a, --hashalgo sha1 (default), sha224, sha256, sha384, sha512, streebog256, streebog512\n" " -s, --imasig make IMA signature\n" " -d, --imahash make IMA hash\n" " -f, --sigfile store IMA signature in .sig file instead of xattr\n" @@ -1669,6 +1670,7 @@ 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" + " --engine e preload OpenSSL engine e\n" " -v increase verbosity level\n" " -h, --help display this help and exit\n" "\n"); @@ -1721,6 +1723,7 @@ static struct option opts[] = { {"selinux", 1, 0, 136}, {"caps", 2, 0, 137}, {"list", 0, 0, 138}, + {"engine", 1, 0, 139}, {"xattr-user", 0, 0, 140}, {} @@ -1763,6 +1766,7 @@ static char *get_password(void) int main(int argc, char *argv[]) { int err = 0, c, lind; + ENGINE *eng = NULL; g_argv = argv; g_argc = argc; @@ -1873,6 +1877,16 @@ int main(int argc, char *argv[]) case 138: measurement_list = 1; break; + case 139: /* --engine e */ + eng = ENGINE_by_id(optarg); + if (!eng) + log_err("engine %s isn't available\n", optarg); + else if (!ENGINE_init(eng)) { + log_err("engine %s init failed\n", optarg); + ENGINE_free(eng); + eng = NULL; + } + break; case 140: /* --xattr-user */ xattr_ima = "user.ima"; xattr_evm = "user.evm"; @@ -1903,6 +1917,13 @@ int main(int argc, char *argv[]) } } + if (eng) { + ENGINE_finish(eng); + ENGINE_free(eng); +#if OPENSSL_API_COMPAT < 0x10100000L + ENGINE_cleanup(); +#endif + } ERR_free_strings(); EVP_cleanup(); BIO_free(NULL); diff --git a/src/imaevm.h b/src/imaevm.h index 1bafaad..1a5ebbe 100644 --- a/src/imaevm.h +++ b/src/imaevm.h @@ -149,6 +149,7 @@ struct signature_hdr { char mpi[0]; } __packed; +/* reflect enum hash_algo from include/uapi/linux/hash_info.h */ enum pkey_hash_algo { PKEY_HASH_MD4, PKEY_HASH_MD5, @@ -158,6 +159,18 @@ enum pkey_hash_algo { PKEY_HASH_SHA384, PKEY_HASH_SHA512, PKEY_HASH_SHA224, + PKEY_HASH_RIPE_MD_128, + PKEY_HASH_RIPE_MD_256, + PKEY_HASH_RIPE_MD_320, + PKEY_HASH_WP_256, + PKEY_HASH_WP_384, + PKEY_HASH_WP_512, + PKEY_HASH_TGR_128, + PKEY_HASH_TGR_160, + PKEY_HASH_TGR_192, + PKEY_HASH_SM3_256, + PKEY_HASH_STREEBOG_256, + PKEY_HASH_STREEBOG_512, PKEY_HASH__LAST }; diff --git a/src/libimaevm.c b/src/libimaevm.c index 714f1ac..8f74660 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -50,6 +50,7 @@ #include <string.h> #include <stdio.h> +#include <openssl/crypto.h> #include <openssl/pem.h> #include <openssl/evp.h> #include <openssl/x509.h> @@ -66,6 +67,8 @@ const char *const pkey_hash_algo[PKEY_HASH__LAST] = { [PKEY_HASH_SHA384] = "sha384", [PKEY_HASH_SHA512] = "sha512", [PKEY_HASH_SHA224] = "sha224", + [PKEY_HASH_STREEBOG_256] = "streebog256", + [PKEY_HASH_STREEBOG_512] = "streebog512", }; /* @@ -290,7 +293,7 @@ int ima_calc_hash(const char *file, uint8_t *hash) md = EVP_get_digestbyname(params.hash_algo); if (!md) { - log_err("EVP_get_digestbyname() failed\n"); + log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo); return 1; } @@ -508,14 +511,16 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size, asn1 = &RSA_ASN1_templates[hdr->hash_algo]; if (len < asn1->size || memcmp(out, asn1->data, asn1->size)) { - log_err("%s: verification failed: %d\n", file, err); + log_err("%s: verification failed: %d (asn1 mismatch)\n", + file, err); return -1; } len -= asn1->size; if (len != size || memcmp(out + asn1->size, hash, len)) { - log_err("%s: verification failed: %d\n", file, err); + log_err("%s: verification failed: %d (digest mismatch)\n", + file, err); return -1; } @@ -527,7 +532,8 @@ int get_hash_algo(const char *algo) int i; for (i = 0; i < PKEY_HASH__LAST; i++) - if (!strcmp(algo, pkey_hash_algo[i])) + if (pkey_hash_algo[i] && + !strcmp(algo, pkey_hash_algo[i])) return i; return PKEY_HASH_SHA1; @@ -899,5 +905,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c static void libinit() { OpenSSL_add_all_algorithms(); + OPENSSL_add_all_algorithms_conf(); ERR_load_crypto_strings(); }
There are two methods of using GOST algorithms in OpenSSL: via config extension and via --engine option. Both require gost-engine to be installed. Signed-off-by: Vitaly Chikunov <vt@altlinux.org> --- src/evmctl.c | 27 ++++++++++++++++++++++++--- src/imaevm.h | 13 +++++++++++++ src/libimaevm.c | 15 +++++++++++---- 3 files changed, 48 insertions(+), 7 deletions(-)