Endless Loop on signature size about pam_pkcs11 HOT 10 OPEN

Yes. Say, in a week or two. Is that ok for you?

from pam_pkcs11.

I changed line 1814 back to *signature_length = 64; and added increment line to:

if (rv == CKR_BUFFER_TOO_SMALL) {
/* increase signature length as long as it it to short */
free(*signature);
*signature = NULL;
*signature_length+=64; // MAB added
DBG1("increased signature buffer-length to %ld", *signature_length);

A bit cleaner I suppose.

from pam_pkcs11.

I'm not sure if it is quite correct.

According to PKCS#11 standard , when buffer for signature is not large enough, PKCS#11 library implementation MUST return CKR_BUFFER_TOO_SMALL and update parameter signature_length to value large enough (not necessary exact) to hold resulting signature.

So I think you have some buggy implementation of PKCS#11 which return CKR_BUFFER_TOO_SMALL and does NOT update signature_length parameter when buffer is not large enough. But works well if it is enough.

We can make a workaround to make it work, but simply incrementing what was returned from PKCS#11 implementation is in my opinion not right solution.
I think we should somewhere remember how much we allocated previously and if PKCS#11 implementation didn't touch or even lowered signature_length parameter use own (preferably double) size.

On the other hand signature is done using RSA key and for ALL currently used sizes of RSA keys 64 bytes is much less than required, minimum key sizes now are 2048 bits, which results in 256 bytes of signature and call to C_Sign() will be done at least twice (after first call for most of PKCS#11 implementations will update required length), but for @matlabs solution and RSA 2048-bit key will be called four times.

Usually, when we need to minimize memory usage, we call C_Sign() twice: first time with signature=NULL, just to get correct buffer size, allocate memory of size returned by first C_Sign() call and then call again with signature pointing to allocated buffer and updated signature_length.

But for the case for pam_pkcs11 we don't need to minimize memory usage, so we should set first time buffer of size 1024, signature size then is large enough for keys up to 8192 bits, which is currently almost 99.999% RSA keys used and C_Sign() will be called once, which speeds up logging to system.

So summarizing changes, it could look like (my proposed changes marked as /* MS */):

  *signature = NULL;
  *signature_length =  1024; /* MS */
  while (*signature == NULL) {
    CK_ULONG current_signature_length = *signature_length; /* MS */
    *signature = malloc(*signature_length);
    if (*signature == NULL) {
      set_error("not enough free memory available");
      return -1;
    }
    rv = h->fl->C_Sign(h->session, hash + h_offset, sizeof(hash) - h_offset, *signature, signature_length);
    if (rv == CKR_BUFFER_TOO_SMALL) {
      /* increase signature length as long as it it to short */
      free(*signature);
      *signature = NULL;
      DBG1("increased signature buffer-length to %ld", *signature_length);
      if (current_signature_length >= *signature_length) {  /* MS */
        /* workaround for buggy  PKCS#11 implementation: it didn't change
           or even lowered buffer size - forcing using larger (double size) buffer */
        *signature_length = current_signature_length * 2;
      }
    } else if (rv != CKR_OK) {
      free(*signature);
      *signature = NULL;
      set_error("C_Sign() failed: 0x%08lX", rv);
      return -1;
    }
  }

from pam_pkcs11.

Created PR #40.

from pam_pkcs11.

I made your requested modifications and everything works great. I have tested this change on:
Ubuntu 18.04
Raspbian (raspberry pi)
RedHat 7

Thank you for your quick response.

from pam_pkcs11.

I have this same issue.
The latest official release/tag is 0.6.11, which does not contain this fix and others. Are you planning on doing a new release?

from pam_pkcs11.

I cannot see any references to fix this bug in master. Is #40 (or some other workaround) already incorporated in master branch?

from pam_pkcs11.

@mskalski is right. It is not yet merged. However, the patch works for me on Ubuntu 20.04 and the Yubikey 5 with libykcs11.so.

from pam_pkcs11.

Hi! I have some questions on #40 .

from pam_pkcs11.

#40 had been merged about a year ago. Shouldn't this issue be closed now?

from pam_pkcs11.