From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 926F9C433B4 for ; Thu, 6 May 2021 00:54:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6A5EA613DD for ; Thu, 6 May 2021 00:54:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229603AbhEFAzw (ORCPT ); Wed, 5 May 2021 20:55:52 -0400 Received: from vmicros1.altlinux.org ([194.107.17.57]:56730 "EHLO vmicros1.altlinux.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229602AbhEFAzv (ORCPT ); Wed, 5 May 2021 20:55:51 -0400 Received: from imap.altlinux.org (imap.altlinux.org [194.107.17.38]) by vmicros1.altlinux.org (Postfix) with ESMTP id 5875172C8B5; Thu, 6 May 2021 03:54:53 +0300 (MSK) Received: from altlinux.org (sole.flsd.net [185.75.180.6]) by imap.altlinux.org (Postfix) with ESMTPSA id 4321A4A46E8; Thu, 6 May 2021 03:54:53 +0300 (MSK) Date: Thu, 6 May 2021 03:54:53 +0300 From: Vitaly Chikunov To: Stefan Berger Cc: Mimi Zohar , Dmitry Kasatkin , linux-integrity@vger.kernel.org Subject: Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file Message-ID: <20210506005453.6czsllqawzye4pzb@altlinux.org> References: <20210505064843.111900-1-vt@altlinux.org> <20210505064843.111900-3-vt@altlinux.org> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org Stefan, On Wed, May 05, 2021 at 07:13:39PM -0400, Stefan Berger wrote: > > On 5/5/21 2:48 AM, Vitaly Chikunov wrote: > > Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from > > SKID of the certificate file. PEM or DER format is auto-detected. > > > > `--keyid' option is reused instead of adding a new option (like possible > > `--cert') to signify to the user it's only keyid extraction and nothing > > more. > > > > This commit creates ABI change for libimaevm, due to adding new function > > ima_read_keyid(). Newer clients cannot work with older libimaevm. > > Together with previous commit it creates backward-incompatible ABI > > change, thus soname should be incremented on release. > > > > Signed-off-by: Vitaly Chikunov > > --- > > README | 1 + > > src/evmctl.c | 22 ++++++++++--- > > src/imaevm.h | 1 + > > src/libimaevm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/sign_verify.test | 1 + > > 5 files changed, 105 insertions(+), 5 deletions(-) > > > > diff --git a/README b/README > > index 8cd66e0..0e1f6ba 100644 > > --- a/README > > +++ b/README > > @@ -49,6 +49,7 @@ OPTIONS > > --rsa use RSA key type and signing scheme v1 > > -k, --key path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem) > > --keyid val overwrite signature keyid with a value (for signing) > > + val is a x509 cert file if prefixed with '@' > > -o, --portable generate portable EVM signatures > > -p, --pass password for encrypted signing key > > -r, --recursive recurse into directories (sign) > > diff --git a/src/evmctl.c b/src/evmctl.c > > index 8ae5488..6a60599 100644 > > --- a/src/evmctl.c > > +++ b/src/evmctl.c > > @@ -42,6 +42,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -57,12 +58,14 @@ > > #include > > #include > > +#include > > #include > > #include > > #include > > #include > > #include > > #include > > +#include > > #include "hash_info.h" > > #include "pcr.h" > > #include "utils.h" > > @@ -2447,6 +2450,7 @@ static void usage(void) > > " --rsa use RSA key type and signing scheme v1\n" > > " -k, --key path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n" > > " --keyid val overwrite signature keyid with a value (for signing)\n" > > + " val is a x509 cert file if prefixed with '@'\n" > > " -o, --portable generate portable EVM signatures\n" > > " -p, --pass password for encrypted signing key\n" > > " -r, --recursive recurse into directories (sign)\n" > > @@ -2572,7 +2576,6 @@ int main(int argc, char *argv[]) > > int err = 0, c, lind; > > ENGINE *eng = NULL; > > unsigned long keyid; > > - char *eptr; > > #if !(OPENSSL_VERSION_NUMBER < 0x10100000) > > OPENSSL_init_crypto( > > @@ -2718,10 +2721,19 @@ int main(int argc, char *argv[]) > > pcrfile[npcrfile++] = optarg; > > break; > > case 143: > > - errno = 0; > > - keyid = strtoul(optarg, &eptr, 16); > > - if (errno || eptr - optarg != strlen(optarg) || > > - keyid == ULONG_MAX || keyid > UINT_MAX || > > + if (optarg[0] == '@') { > > + keyid = ima_read_keyid(optarg + 1, NULL); > > + } else { > > + char *eptr; > > + > > + errno = 0; > > + keyid = strtoul(optarg, &eptr, 16); > > + if (eptr - optarg != strlen(optarg) || errno) { > > + log_err("Invalid keyid value.\n"); > > + exit(1); > > + } > > + } > > + if (keyid == ULONG_MAX || keyid > UINT_MAX || > > keyid == 0) { > > log_err("Invalid keyid value.\n"); > > exit(1); > > diff --git a/src/imaevm.h b/src/imaevm.h > > index 9f38059..eab7f32 100644 > > --- a/src/imaevm.h > > +++ b/src/imaevm.h > > @@ -219,6 +219,7 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509); > > void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len); > > void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey); > > int key2bin(RSA *key, unsigned char *pub); > > +unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid); > > int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig); > > int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen); > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > index 481d29d..a22d9bb 100644 > > --- a/src/libimaevm.c > > +++ b/src/libimaevm.c > > @@ -57,6 +57,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include "imaevm.h" > > @@ -748,6 +749,90 @@ void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey) > > X509_PUBKEY_free(pk); > > } > > +enum keyid_file_type { > > + KEYID_FILE_PEM_KEY = 0, > > + KEYID_FILE_UNK_CERT, > > +}; > > + > > +/* > > + * @is_cert: 1 - this is a x509 cert file (maybe PEM, maybe DER encoded); > > This probably change since you wrote it. Is this file_type now? Yes. (This is a consequence of making generic function out of N smaller functions- it's very hard to invent distinguisher that user could easily or understand at all.) > > + * 0 - this is a PEM encoded private key file with possible appended > > + * x509 cert. > > + */ > > +static unsigned long _ima_read_keyid(const char *certfile, uint32_t *keyid, > > + enum keyid_file_type file_type) > > +{ > > + uint32_t keyid_raw; > > + const ASN1_OCTET_STRING *skid; > > + int skid_len; > > + X509 *x = NULL; > > + FILE *fp; > > + > > + if (!(fp = fopen(certfile, "r"))) { > > + log_err("read keyid: %s: Cannot open: %s\n", certfile, > > + strerror(errno)); > > + return -1; > > + } > > + if (!PEM_read_X509(fp, &x, NULL, NULL)) { > > + if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { > > + ERR_clear_error(); > > + if (file_type == KEYID_FILE_PEM_KEY) { > > + log_debug("%s: x509 certificate not found\n", > > + certfile); > > + fclose(fp); > > + return -1; > > + } > > + rewind(fp); > > + d2i_X509_fp(fp, &x); > > + } > > + if (!x) { > > + ERR_print_errors_fp(stderr); > > + log_err("read keyid: %s: Error reading x509 certificate\n", > > + certfile); > > + fclose(fp); > > + return -1; > > + } > > + } > > + fclose(fp); > > + > > + if (!(skid = X509_get0_subject_key_id(x))) { > > + log_err("read keyid: %s: SKID not found\n", certfile); > > + goto err_free; > > + } > > + skid_len = ASN1_STRING_length(skid); > > + if (skid_len < sizeof(keyid_raw)) { > > + log_err("read keyid: %s: SKID too short (len %d)\n", certfile, > > + skid_len); > > + goto err_free; > > + } > > + memcpy(&keyid_raw, ASN1_STRING_get0_data(skid) + skid_len > > + - sizeof(keyid_raw), sizeof(keyid_raw)); > > + if (keyid) > > + memcpy(keyid, &keyid_raw, sizeof(*keyid)); > > > If keyid is supposed to be in native format then you have to apply ntohl() > to it.. Keyid is supposed to be in network order, it's documented (for ima_read_keyid which is just a wrapper for _ima_read_keyid). > > > > + log_info("keyid %04x (from %s)\n", ntohl(keyid_raw), certfile); > > + return ntohl(keyid_raw); > > I thought this should return 0š ? Why you thought so? It's documented for ima_read_keyid() return value is -1 or 32-bit keyid. > > > > + > > +err_free: > > + X509_free(x); > > + return -1; > > +} > > + > > +/** > > + * ima_read_keyid() - Read 32-bit keyid from the cert file. > > + * @certfile: File possibly containing certificate in DER/PEM format. > > + * @keyid: Output keyid in network order. > > + * > > + * Try to read keyid from Subject Key Identifier (SKID) of certificate. > > + * Autodetect if cert is in PEM or DER encoding. > > + * > > + * Return: -1 (ULONG_MAX) on error; > > + * 32-bit keyid as unsigned integer in host order. > That's confusing, two times the same result, one time in host order, on time > in network order. Why not just one return value in host order? Pointer API is similar to calc_keyid_v2(). Do you sugegst to change calc_keyid_v2() API too? To introduce non-confusing API that contradict other parts of API would be more confusing than it already is. Thanks, > > + */ > > +unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid) > > +{ > > + return _ima_read_keyid(certfile, keyid, KEYID_FILE_UNK_CERT); > > +} > > + > > static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass) > > { > > FILE *fp; > > diff --git a/tests/sign_verify.test b/tests/sign_verify.test > > index 2c21812..52ea33a 100755 > > --- a/tests/sign_verify.test > > +++ b/tests/sign_verify.test > > @@ -360,6 +360,7 @@ sign_verify rsa1024 md5 0x030201:K:0080 > > sign_verify rsa1024 sha1 0x030202:K:0080 > > sign_verify rsa1024 sha224 0x030207:K:0080 > > expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204aabbccdd0080 OPTS=--keyid=aabbccdd > > +expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204:K:0080 OPTS=--keyid=@test-rsa1024.cer > > sign_verify rsa1024 sha256 0x030204:K:0080 > > try_different_keys > > try_different_sigs