some pkinit plugin PKCS11 related issues

[Will Fiveash]

I've found a couple issues with the way the pkinit plugin interacts with
PKCS11.  The first is that way the "slotid" in krb5.conf is handled.  It
should be used as a filter to choose one or more slots from the list of
slots returned by C_GetSlotList()  Instead it is being directly assigned
to the slotlist[] arg which is passed to C_OpenSession() and if the
value is invalid can cause C_OpenSession() to segfault.  Here is the
broken code in
src/plugins/preauth/pkinit/pkinit_crypto_openssl.c:pkinit_open_session():

    if (cctx->slotid != PK_NOSLOT) {
        /* A slot was specified, so that's the only one in the list */
        count = 1;
        slotlist = malloc(sizeof(CK_SLOT_ID));
        slotlist[0] = cctx->slotid;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^ wrong

Instead this should be something like:

    if (cctx->p11->C_GetSlotList(TRUE, slotlist, &count) != CKR_OK) {
        krb5_set_error_message(context, KRB5KDC_ERR_PREAUTH_FAILED,
                       gettext("Error trying to get PKCS11 slot list: %s"),
                       pkinit_pkcs11_code_to_text(r));
        pkiDebug("C_GetSlotList: %s\n", pkinit_pkcs11_code_to_text(r));
        r = KRB5KDC_ERR_PREAUTH_FAILED;
        goto out;
    }

    /* examine all the tokens */
    for (i = 0; i < count; i++) {
        /*
         * If a slotid was specified skip slots that don't match.
         */
        if (cctx->slotid != PK_NOSLOT && cctx->slotid != slotlist[i])
            continue;

        /* Open session */
        if ((r = cctx->p11->C_OpenSession(slotlist[i], CKF_SERIAL_SESSION,
                          NULL, NULL, &tmpsession)) != CKR_OK) {
            pkiDebug("C_OpenSession: %s\n", pkinit_pkcs11_code_to_text(r));
            krb5_set_error_message(context, KRB5KDC_ERR_PREAUTH_FAILED,
                       gettext("Error trying to open PKCS11 session: %s"),
                       pkinit_pkcs11_code_to_text(r));
            r = KRB5KDC_ERR_PREAUTH_FAILED;
            goto out;
        }
...

Second, this same function has logic that assumes the CK_TOKEN_INFO
tinfo.label will contain some white space padding at the end:

        for (cp = tinfo.label + sizeof (tinfo.label) - 1;
             *cp == '\0' || *cp == ' '; cp--)
            *cp = '\0';
        pkiDebug("open_session: slotid %d token \"%s\"\n",
                 (int) slotlist[i], tinfo.label);
        if (cctx->token_label == NULL ||
            !strcmp((char *) cctx->token_label, (char *) tinfo.label))

There is no guarantee this is the case however as the PKCS11 spec states
this label is an array of characters, padded with white space if
necessary, not a C string.  So if the last byte of tinfo.label is a
non-white space character "*cp = '\0';" will never be executed which
means strcmp() will have been passed a bogus arg in tinfo.label.

-- 
Will Fiveash
Oracle Solaris Software Engineer