[ksk-rollover] Concerns about KSK generation software quality.
Andres Pavez
andres.pavez at iana.org
Fri Feb 16 21:25:41 UTC 2018
Hi Warren,
I appreciate your comments and also that you take time to look the code.
This is a very good discussion, so I have the following item/actions that we will implement based in your and others suggestions.
* For the ceremony key generation script: We will include a verification steps to check and read out the "keytag" and "keylabel" to ensure that the new key is not equal to the previous generated key.
* For the source code: We are in the process to start planning an upgrade in the software and components and definitely your comments will be included in a new version or release.
Thank you,
--
Andres Pavez
Cryptographic Key Manager
On 2/16/18, 12:01, "ksk-rollover on behalf of Warren Kumari" <ksk-rollover-bounces at icann.org on behalf of warren at kumari.net> wrote:
In the "Suggested update" thread, Andres pointed me
(http://mm.icann.org/pipermail/ksk-rollover/2018-February/000385.html)
at the IANA DNSSEC-Keytools KSK generation tools -
https://github.com/iana-org/dnssec-keytools/blob/master/kskgen/kskgen.c
I took a quick look at the code, and much of it scares me.
I suspect that Andres was talking about the pkcs11 CKA_LABEL label,
which turns into a "keypair label" - from the code, this is unique
because (dnssec-keytools/common/pkcs11_dnssec.c):
/*
* CKA_LABEL HACK
* AEP Keyper can only display 7 characters and
* cannot change the HSM internal CKA_LABEL once created.
* So, we label them with a monotonically increasing
* string based on seconds since epoch.
*/
This is (AFAICT), simply an internal label for the key, but is
distinct / unrelated to the DNSSEC keytag (as far as I can see). The
DNSSEC key tag is generated (by dnssec_keytag in
dnssec-keytools/common/dnssec.c:25) purely from the [Flags, Prot, Alg,
PubKey] (RFC4034, Appendix B).
The comments interested me, so I looked a little bit further, and now
I'm filled with unease.
What is concerning to me is that the "void *pkcs11_genrsakey(int
bits,int flags)" function
(dnssec-keytools/common/pkcs11_dnssec.c:1121) tries 10 times to find a
unique keypair label, but if it cannot, throws an error about DNSSEC
keytags:
if(ts > 0) { /* DUPLICATE - try again */
logger_warning("Found %u duplicate keys labeled \"%s\". Try
again...",ts,label);
if(++tries < 10) {
sleep(1);
goto regen;
}
logger_error("Can't get a unique DNSSEC tag after %d tries. Giving
up...",tries);
goto err;
}
Note that this all happens before the actual key generation (~1248).
While this is obviously just a misleading error message (it's not a
DNSSEC tag, it is a key label), it makes me wonder about how carefully
the whole CKA_LABEL HACK bit has been reviewed.
Looking a bit further, the keylabel comes from get_monotonic_str()
(dnssec-keytools/common/pkcs11_dnssec.c:1087). This bit of code seems,
er, interesting.
It starts with:
static time_t t1=0;
I'm surprised by the "static time_t" - function-local static
variables lead to all sorts to hard to debug side effects. Anyway, I
also cannot understand how the output of this could cause duplicates,
and so don't understand why this is being tried 10 times - if there
**is** already a keypair with this label, surely this is evidence of
much larger issue, and simply trying again is not a good idea?
A quick look also shows a bunch (13) of gotos, including at least 3
instances of "goto err;" without any useful sort of log message being
printed (e.g: if((rv=pfl->C_FindObjects(sh,hKeys,PKCS11_MAX_KEYS_PER_SLOT,&ts))
!= CKR_OK) goto err; -- this will fail with no output as to why.)
A *very* quick skim of the source code also shows all sorts of typoes:
if(t1) { /* since we are dropping the last base32 char, wait to
gurantee uniqueness */ --- guarantee
The CKA_LABEL is based on a time based sting for uniquness. ---
string, uniqueness
logger_error("Key generaton failed"); --- generation
* create a CSR - a simple private key proof of possesion ---- possession
\return NULL if failed; otherwise newlly crated key record associated
with pk. --- newly created
/* setup computation for DS rcords */ --- records
/*! check to see if we have access to the private key in this key
strust --- struct?
/*! log in to HSM slot referrenced by pk --- referenced
/* get correspoding private key if possible */ --- corresponding
While typos in error messages and comments are (clearly) not going to
cause breakage, it does make me concerned that this code, which
generates the single most critical DNSSEC keys, has not received
sufficient careful review. These sorts of obvious typos are not
themselves a problem, but rather indicative of a larger issue. While
the obvious retort is "It is publicly posted in GitHub, and we asked
the community to have a look. Send pull requests!", it appears that
this hasn't resulted in enough review. I used to program C, but it has
been long enough that now I'm only qualified to say "Well, that
doesn't seem right!" - how do we get (more) review from people who are
better qualified?
The KSK ceremonies provide important transparency - how can we get
more review of the tools used in the ceremonies? Having people fly in,
and then having the tools fail, or, worse yet, questions about the
correctness of the result does not seem good.
W
--
I don't think the execution is relevant when it was obviously a bad
idea in the first place.
This is like putting rabid weasels in your pants, and later expressing
regret at having chosen those particular rabid weasels and that pair
of pants.
---maf
_______________________________________________
ksk-rollover mailing list
ksk-rollover at icann.org
https://mm.icann.org/mailman/listinfo/ksk-rollover
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4604 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/ksk-rollover/attachments/20180216/96efa0ca/smime.p7s>
More information about the ksk-rollover
mailing list