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 <vt@xxxxxxxxxxxx>
> > ---
> > 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 <sys/param.h>
> > #include <sys/stat.h>
> > #include <sys/ioctl.h>
> > +#include <arpa/inet.h>
> > #include <fcntl.h>
> > #include <unistd.h>
> > #include <stdlib.h>
> > @@ -57,12 +58,14 @@
> > #include <termios.h>
> > #include <assert.h>
> > +#include <openssl/asn1.h>
> > #include <openssl/sha.h>
> > #include <openssl/pem.h>
> > #include <openssl/hmac.h>
> > #include <openssl/err.h>
> > #include <openssl/rsa.h>
> > #include <openssl/engine.h>
> > +#include <openssl/x509v3.h>
> > #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 <openssl/pem.h>
> > #include <openssl/evp.h>
> > #include <openssl/x509.h>
> > +#include <openssl/x509v3.h>
> > #include <openssl/err.h>
> > #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
