some pkinit plugin PKCS11 related issues

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

some pkinit plugin PKCS11 related issues

Will Fiveash-2
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
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: some pkinit plugin PKCS11 related issues

Greg Hudson
On 05/13/2014 06:41 PM, Will Fiveash wrote:
> 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()

PKCS11 doesn't seem to say whether C_GetSlotList is allowed to crash on
an invalid slot ID.  But I assume it does so on Solaris, which is reason
enough to change it.

I would assume that at most one slot from the list will match the slot
number in a sane PKCS11 implementation.

>     if (cctx->p11->C_GetSlotList(TRUE, slotlist, &count) != CKR_OK) {
[...]
>     /* examine all the tokens */
[...]
>         if (cctx->slotid != PK_NOSLOT && cctx->slotid != slotlist[i])
>             continue;
[...]
>         if ((r = cctx->p11->C_OpenSession(slotlist[i], CKF_SERIAL_SESSION,
>                           NULL, NULL, &tmpsession)) != CKR_OK) {

This seems reasonable, and should come to a little less code than is
already present.

>         for (cp = tinfo.label + sizeof (tinfo.label) - 1;
>              *cp == '\0' || *cp == ' '; cp--)
>             *cp = '\0';
[...]
>         if (cctx->token_label == NULL ||
>             !strcmp((char *) cctx->token_label, (char *) tinfo.label))

That does seem like a botch.  It should find the last non-whitespace
character, then compare the length against strlen(cctx->token_label) and
memcmp() the contents.  There should be no need to alter tinfo.label as
we scan backwards, but we should guard against underrunning tinfo.label.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev