MAX_PIN_LENGTH 32 is shorter than some tokens permit

[cjwatson]

My team is currently working with a YubiHSM 2, using yubihsm_pkcs11.so accessed via libp11's engine. Relying on https://docs.yubico.com/hardware/yubihsm-2/hsm-2-user-guide/hsm2-pkcs11-guide.html#logging-in, we created an authentication key with a 30-character password and constructed a PKCS#11 URI to use that password via pin-source, only to find that OpenSSL-based operations failed at the stage of logging into the module.

After quite a bit of debugging we eventually realized that the failure depended on the length of the password. The YubiHSM documentation says "Currently the total PIN length must be 12 to 68 bytes (including the encoded auth key id, so 8 to 64 bytes for the actual PIN)", but libp11 in fact imposes a shorter limit; after the four bytes that the YubiHSM module uses to identify the authentication key to use, and after the extra byte that BIO_gets subtracts from the given length, this leaves 27 characters for the password. (Also note that MAX_PIN_LENGTH is also defined separately in src/p11_key.c.)

Is there any reason libp11 couldn't allow longer PINs? In addition, it would be helpful if it could issue some kind of diagnostic if the given PIN is longer than it's prepared to read; that would have saved us quite a bit of needle-in-a-haystack debugging.

[mtrojnar]

32 seemed like a reasonable upper limit for the number of PIN characters entered by the user. If YubiHSM 2 uses (abuses?) it for providing an additional identifier, I'm perfectly fine with increasing the value to 68. Does the change fix your particular configuration? Could you elaborate on BIO_gets? It seems to be a separate issue.

[dengert]

PKCS11 leaves it up to the PKCS11 module to return in ulMaxPinLen; of a CK_TOKEN_INFO request. Libp11 could use that value as the upper limit. C_Login passes the pin as a CK_UTF8CHAR_PTR of length CK_ULONG ulPinLen So libp11 should make no assumptions or the contents of the pin or its length.

https://github.com/OpenSC/libp11/blob/master/src/p11_slot.c#L521-L570 does a C_GetTokenInfo, but does not look at the ulMaxPinLen. Can you trace the call with a PKCS11 SPY or debugger to see what the YubiHSM pkcs11 module returns?

[mtrojnar]

So libp11 should make no assumptions or the contents of the pin or its length.

AFAIK libp11 doesn't make such assumptions. Only the pkcs11 engine does.

[dengert]

AFAIK libp11 doesn't make such assumptions. Only the pkcs11 engine does.

Engine is part of libp11 git repository.

If you are purposing to only change #define MAX_PIN_LENGTH 32 in ./src/p11_key.c and ./src/eng_back.c why stop at 68? at least go to 256.

[mtrojnar]

Engine is part of libp11 git repository.

This is correct. The pkcs11 engine is part of the libp11 git repository now. It is not part of the libp11 library though, and I'm not aware of a libp11 interface that could by used by the engine to retrieve ulMaxPinLen. I admit I may have missed something.

If you are purposing to only change #define MAX_PIN_LENGTH 32 in ./src/p11_key.c and ./src/eng_back.c why stop at 68? at least go to 256.

I agree.

[cjwatson]

@mtrojnar Increasing the limit would deal with our immediate problem, yes. We don't need something arbitrarily long, but having a decently long password for HSM operations seems worthwhile, especially if it's coming from a credentials file using pin-source rather than being entered manually by an operator every time.

Regarding BIO_gets, this appears to be a minor off-by-one error in the engine. ctx_try_load_object does char tmp_pin[MAX_PIN_LENGTH+1]; size_t tmp_pin_len = MAX_PIN_LENGTH;, allowing an extra byte for the NUL terminator. So far so good. It then (among other things) passes those down via parse_pkcs11_uri and parse_pin_source to read_from_file, which calls BIO_gets(fp, field, *field_len). However, BIO_gets is documented as "Usually this operation will attempt to read a line of data from the BIO of maximum length size-1". The effect is that an additional byte of space is unnecessarily left for the NUL terminator. Obviously this should be checked carefully in case my analysis is wrong and results in an overflow, but I think BIO_gets(fp, field, *field_len + 1) would be correct here since BIO_gets apparently expects the allocated size and not the maximum available string length.

@dengert The YubiHSM PKCS#11 module is open source, and here's its definition of C_GetTokenInfo: YUBIHSM_PKCS11_MAX_PIN_LEN there is defined to 64, although higher-level code can't reasonably be expected to know about the extra four bytes for the encoded authentication ID, so I guess that would unnecessarily limit the actual PIN to 60 characters if I understand those fields correctly (though maybe I'm misunderstanding something since those were apparently changed recently). Still, honouring that would also be an improvement if you wanted to take that approach instead of just using a bigger hardcoded limit.