The diff below has three independent parts which I intend to commit
separately. It does not change anything.
In op_thumbprint() use EVP_Digest() for simplicity. The conversion
is straightforward and it's an obvious win.
The second part converts EVP_Sign*() usage to EVP_DigestSign(). This is
a bit more ergonomic, in particular it allows us to allocate the exact
amount needed, rather than some mysterious estimate from EVP_PKEY_size().
The third part (the last hunk) is the most interesting bit. We can
simplify this using API that wasn't available at the time this was
written.
1. bn_len can be determined with EVP_PKEY_bits(). This uses the
group order internally, which is more correct for signatures
than the degree, even if it works out to the same number 48.
Move this a bit down to when it's needed.
2. Use the saner API ECDSA_SIG_get0_{r,s}() which is less error
prone than ECDSA_SIG_get0().
3. Zero padding can be done with BN_bn2binpad() which is less
fiddly than doing this by hand. Also, error check.
Index: acctproc.c
===================================================================
RCS file: /cvs/src/usr.sbin/acme-client/acctproc.c,v
retrieving revision 1.24
diff -u -p -r1.24 acctproc.c
--- acctproc.c 14 Dec 2022 15:02:43 -0000 1.24
+++ acctproc.c 15 Dec 2022 06:13:36 -0000
@@ -133,8 +133,7 @@ static int
op_thumbprint(int fd, EVP_PKEY *pkey)
{
char *thumb = NULL, *dig64 = NULL;
- EVP_MD_CTX *ctx = NULL;
- unsigned char *dig = NULL;
+ unsigned char dig[EVP_MAX_MD_SIZE];
unsigned int digsz;
int rc = 0;
@@ -161,32 +160,21 @@ op_thumbprint(int fd, EVP_PKEY *pkey)
* it up in the read loop).
*/
- if ((dig = malloc(EVP_MAX_MD_SIZE)) == NULL) {
- warn("malloc");
- goto out;
- } else if ((ctx = EVP_MD_CTX_new()) == NULL) {
- warnx("EVP_MD_CTX_new");
- goto out;
- } else if (!EVP_DigestInit_ex(ctx, EVP_sha256(), NULL)) {
- warnx("EVP_SignInit_ex");
+ if (!EVP_Digest(thumb, strlen(thumb), dig, &digsz, EVP_sha256(),
+ NULL)) {
+ warnx("EVP_Digest");
goto out;
- } else if (!EVP_DigestUpdate(ctx, thumb, strlen(thumb))) {
- warnx("EVP_SignUpdate");
- goto out;
- } else if (!EVP_DigestFinal_ex(ctx, dig, &digsz)) {
- warnx("EVP_SignFinal");
- goto out;
- } else if ((dig64 = base64buf_url((char *)dig, digsz)) == NULL) {
+ }
+ if ((dig64 = base64buf_url((char *)dig, digsz)) == NULL) {
warnx("base64buf_url");
goto out;
- } else if (writestr(fd, COMM_THUMB, dig64) < 0)
+ }
+ if (writestr(fd, COMM_THUMB, dig64) < 0)
goto out;
rc = 1;
out:
- EVP_MD_CTX_free(ctx);
free(thumb);
- free(dig);
free(dig64);
return rc;
}
@@ -265,17 +253,16 @@ op_sign(int fd, EVP_PKEY *pkey, enum acc
{
EVP_MD_CTX *ctx = NULL;
const EVP_MD *evp_md = NULL;
- EC_KEY *ec;
ECDSA_SIG *ec_sig = NULL;
const BIGNUM *ec_sig_r = NULL, *ec_sig_s = NULL;
- int cc, rc = 0;
- unsigned int digsz, bufsz, degree, bn_len, r_len, s_len;
+ int bn_len, cc, rc = 0;
char *nonce = NULL, *pay = NULL, *pay64 = NULL;
char *prot = NULL, *prot64 = NULL;
char *sign = NULL, *dig64 = NULL, *fin = NULL;
char *url = NULL, *kid = NULL, *alg = NULL;
- unsigned char *dig = NULL, *buf = NULL;
const unsigned char *digp;
+ unsigned char *dig = NULL, *buf = NULL;
+ size_t digsz;
/* Read our payload and nonce from the requestor. */
@@ -349,27 +336,28 @@ op_sign(int fd, EVP_PKEY *pkey, enum acc
goto out;
}
- if ((dig = malloc(EVP_PKEY_size(pkey))) == NULL) {
- warn("malloc");
- goto out;
- }
-
/*
- * Here we go: using our RSA key as merged into the envelope,
- * sign a SHA256 digest of our message.
+ * Using the key as merged into the envelope, sign the message.
*/
if ((ctx = EVP_MD_CTX_new()) == NULL) {
warnx("EVP_MD_CTX_new");
goto out;
- } else if (!EVP_SignInit_ex(ctx, evp_md, NULL)) {
- warnx("EVP_SignInit_ex");
+ }
+ if (!EVP_DigestSignInit(ctx, NULL, evp_md, NULL, pkey)) {
+ warnx("EVP_DigestSignInit");
+ goto out;
+ }
+ if (!EVP_DigestSign(ctx, NULL, &digsz, sign, cc)) {
+ warnx("EVP_DigestSign");
goto out;
- } else if (!EVP_SignUpdate(ctx, sign, strlen(sign))) {
- warnx("EVP_SignUpdate");
+ }
+ if ((dig = malloc(digsz)) == NULL) {
+ warn("malloc");
goto out;
- } else if (!EVP_SignFinal(ctx, dig, &digsz, pkey)) {
- warnx("EVP_SignFinal");
+ }
+ if (!EVP_DigestSign(ctx, dig, &digsz, sign, cc)) {
+ warnx("EVP_DigestSign");
goto out;
}
@@ -381,40 +369,35 @@ op_sign(int fd, EVP_PKEY *pkey, enum acc
}
break;
case EVP_PKEY_EC:
- if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) == NULL) {
- warnx("EVP_PKEY_get0_EC_KEY");
- goto out;
- }
- degree = EC_GROUP_get_degree(EC_KEY_get0_group(ec));
- bn_len = (degree + 7) / 8;
-
- digp = dig; /* d2i_ECDSA_SIG advances digp */
+ digp = dig;
if ((ec_sig = d2i_ECDSA_SIG(NULL, &digp, digsz)) == NULL) {
warnx("d2i_ECDSA_SIG");
goto out;
}
- ECDSA_SIG_get0(ec_sig, &ec_sig_r, &ec_sig_s);
-
- r_len = BN_num_bytes(ec_sig_r);
- s_len = BN_num_bytes(ec_sig_s);
-
- if((r_len > bn_len) || (s_len > bn_len)) {
+ if ((ec_sig_r = ECDSA_SIG_get0_r(ec_sig)) == NULL ||
+ (ec_sig_s = ECDSA_SIG_get0_s(ec_sig)) == NULL) {
warnx("ECDSA_SIG_get0");
goto out;
}
- bufsz = 2 * bn_len;
- if ((buf = calloc(1, bufsz)) == NULL) {
+ if ((bn_len = (EVP_PKEY_bits(pkey) + 7) / 8) <= 0) {
+ warnx("EVP_PKEY_bits");
+ goto out;
+ }
+
+ if ((buf = calloc(2, bn_len)) == NULL) {
warnx("calloc");
goto out;
}
- /* put r and s in with leading zeros if any */
- BN_bn2bin(ec_sig_r, buf + bn_len - r_len);
- BN_bn2bin(ec_sig_s, buf + bufsz - s_len);
+ if (BN_bn2binpad(ec_sig_r, buf, bn_len) != bn_len ||
+ BN_bn2binpad(ec_sig_s, buf + bn_len, bn_len) != bn_len) {
+ warnx("BN_bn2binpad");
+ goto out;
+ }
- if ((dig64 = base64buf_url((char *)buf, bufsz)) == NULL) {
+ if ((dig64 = base64buf_url((char *)buf, 2 * bn_len)) == NULL) {
warnx("base64buf_url");
goto out;
}
