[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
    at the IANA DNSSEC-Keytools KSK generation tools -
    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):
    * 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
    if(ts > 0) { /* DUPLICATE - try again */
       logger_warning("Found %u duplicate keys labeled \"%s\". Try
       if(++tries < 10) {
       goto regen;
    logger_error("Can't get a unique DNSSEC tag after %d tries. Giving
      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.
    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.
    ksk-rollover mailing list
    ksk-rollover at icann.org
-------------- 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