On Sun, Jun 16, 2019 at 08:44:46PM +0200, Theo Buehler wrote:
> On Sun, Jun 16, 2019 at 09:25:45AM +0200, Florian Obser wrote:
> > On Sat, Jun 15, 2019 at 06:20:49PM +0200, Florian Obser wrote:
> > > OK?
> >
> > update to apply cleanly on -current
>
> The acctproc.c change looks ok with some minor comments inline.
> I haven't tried to verify the JSON stuff.
that's fine, Benno reviewed that and ignored the crypto stuff :D
I commited it with the get0 change.
Questions inline
[...]
> > @@ -190,14 +267,19 @@ op_sign_rsa(char **prot, EVP_PKEY *pkey, const char
> > *nonce, const char *url)
> > static int
> > op_sign(int fd, EVP_PKEY *pkey, enum acctop op)
> > {
> > - char *nonce = NULL, *pay = NULL, *pay64 = NULL;
> > - char *prot = NULL, *prot64 = NULL;
> > - char *sign = NULL, *dig64 = NULL, *fin = NULL;
> > - char *url = NULL, *kid = NULL;
> > - unsigned char *dig = NULL;
> > - EVP_MD_CTX *ctx = NULL;
> > - int cc, rc = 0;
> > - unsigned int digsz;
> > + 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;
> > + 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;
>
> To simplify slightly, you could use
>
> unsigned char dig[EVP_MAX_MD_SIZE];
>
> instead of allocating dig dynamically (EVP_MAX_MD_SIZE is 64).
this doesn't seem to be correct.
} else if (!EVP_SignFinal(ctx, dig, &digsz, pkey)) {
this stores 104 bytes with an NID_secp384r1 key
EVP_MAX_MD_SIZE seems to be about digests, not signatures. Did you
have a different constant in mind or did you get confused by the
poorly worded variable?
[...]
> > + degree = EC_GROUP_get_degree(EC_KEY_get0_group(ec));
> > + bn_len = (degree + 7) / 8;
>
> This calculation is fine if you use curves over a prime field GFp (as
> you currently do), but it will be incorrect if you should ever want to
> use a curve over a binary field GF2m.
I know some of these words!
So this whole song-and-dance is needed because we need to store r and
s with leading zeros, so we need to find out what the maximum
representation length of r and s could be:
/* 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);
This is "documented" in RFC7518 Section 3.4.
https://tools.ietf.org/html/rfc7518#section-3.4
But it's kinda documented by example, i.e. it talks about P-256 and
there r and s representations are 32 bytes long. But for P-384 it's 48
bytes. Of course I'm making this all up as I go along.
What would be the correct calculation for a GF2m field? Should I maybe just
ignore it? RFC 7518 only lists P-256, P-384 and P-521 (and I think
Let's Encrypt only supports P-384 anyway), and they are all GFp, no?
Maybe it would be best to skip the whole degree calculation and
hardcode bn_len as 48.
Thanks for your help!
--
I'm not entirely sure you are real.
