Fwd: LP#929108 support reading PIN from file when using PKCS#11 devices

[Stef Walter]

On 2012-04-16 18:28, Nikos Mavrogiannopoulos wrote:
> On 04/16/2012 06:02 PM, Stef Walter wrote:
>> By default no pin-source handlers are installed. By adding the following
>> default handler, p11-kit to default to treating pin-source (or pinfile)
>> actual files. It will handle invocations of p11_kit_pin_request() by
>> reading actual files:
>>
>>   p11_kit_pin_register_callback (P11_KIT_PIN_FALLBACK,
>>                                  p11_kit_pin_file_callback,
>>                                  NULL, NULL);
>>
>> It's up to you if you want this as default behavior for gnutls. It may
>> make sense.
> 
> Indeed it makes sense to be the default. Could this, however, have bad
> interactions with other callbacks that may be registered by other
> programs or libraries?

Other programs or libraries can install handlers for specific pin
sources. Or they can register fallback handlers. Relevant handlers each
have a turn to return a PIN when p11_kit_pin_request() is called, so
technically it should all work.

Not sure if there is a possible theoretical remote security issue if:

 * An application uses a URI which it received from an untrusted
   source.
 * The URI contains a pin-source which nobody in the stack has
   registered to handle (and thus the gnutls installed fallback file
   handler is used).
 * And the application wasn't expecting the PKCS#11 URI to read from a
   file and use it as a PIN.
 * And somehow this gives an attacker an advantage they would not
   otherwise have.

I think that's a pretty remote possibility, and if an attacker can
specify a PKCS#11 URI at all, then they are able to control which keys
and certs are used. In that case it seems that being able to specify a
PIN read from a file is irrelevant. PKCS#11 URIs should not come from
untrusted sources.

But for sanity's sake would we want to limit the size of the file that
p11-kit will read in its p11_kit_pin_file_callback() handler?

>> The patch adds that line so I guess that's the real meat of the
>> suggested change.
> 
> There is also a change to avoid calling retrieve_pin_for_pinfile if
> attempts is zero. I've currently included it but although it seems
> sensible for a file read, it might break other callbacks. Does the
> p11-kit file read callback fail if the attempt is not the first one?
> 
> I've currently added the check, but if the file callback fails
> I should remove it.

Now that I think about it, I don't think the attempts check is correct.
It assumes that the PIN returned from a registered pin-source handler
will always be identical on each try. That's the case for the
p11_kit_pin_file_callback() but not the case for pretty much any other
handler (such as prompts and such).

In fact I'm pretty sure this change would break glib-networking. Let me
know if you want me to test further.

To mitigate this change, do you think that p11_kit_pin_file_callback()
should not handle the request if it detects that this isn't the first
attempt to read a PIN? Seems like this could fix the issue of an endless
loop, where callers assume that the pin-source handlers will 'give up'
but the p11_kit_pin_file_callback() never gives up returning the same PIN.

Cheers,

Stef