Pin should be cleared if it is known to be wrong

[bircoph]

Hi!

I use 20 char pin and made a typo by entering 21 chars, so I get:
$ nitrocli pws status Could not access the password safe: The supplied string is too long

If I try to run this command again (or any other command requiring pin), I get this message again and no way to enter pin anew until I clear the cache manually using 'nitrocli pin clear'.

I think that in cases when pin is obviously wrong (like too long string) it should be purged from cache automatically.

[robinkrahl]

Interesting point, thanks for the report! The problem is that we cannot be sure whether this error is caused by the PIN or by some other string. For example, when changing a PIN, this error could be caused by the original PIN or by the new PIN.

I can think of three possible fixes:

• Change libnitrokey to return a wrong password error instead of a string too long error.
• Add a case for CommandError::StringTooLong to the match in try_with_pin_and_data_with_pinentry (commands.rs line 193) – or just handle it the same way as the wrong password error. But this might cause a wrong error message if the error is not caused by the PIN.
• Manually check the PIN length. But that would mean hardcoding some magic value intro nitrocli.

[d-e-s-o]

I think there may be multiple bugs here.

1. Aren't we lacking a step to clear the PIN from the cache after it has been changed? If I have the current user PIN cached, then change it, then ask for an OTP I see a message about a wrong password. We are using the old password still.

2. If I use a 21 character PIN and request an OTP that works just fine. Shouldn't that behave the same way as PWS?
   In fact, digging a little more it appears that I cannot set a (user) PIN longer than 25 characters. If I try I correctly get reminded of this length limit when attempting to change the PIN.
   So isn't the problem rather that PWS bails out (wrongly?)?

So I checked what libnitrokey does. The call ends up in enable_password_safe:

    void NitrokeyManager::enable_password_safe(const char *user_pin) {
        //The following command will cancel enabling PWS if it is not supported
        auto a = get_payload<IsAESSupported>();
        strcpyT(a.user_password, user_pin);
        IsAESSupported::CommandTransaction::run(device, a);

        auto p = get_payload<EnablePasswordSafe>();
        strcpyT(p.user_password, user_pin);
        EnablePasswordSafe::CommandTransaction::run(device, p);
    }

which notably does two things. So checking IsAESSupported we see:

class IsAESSupported : Command<CommandID::DETECT_SC_AES> {
 public:
  struct CommandPayload {
    uint8_t user_password[20];            <------------ spurious user_password buffer size
    ...
};

compare that to

class EnablePasswordSafe : Command<CommandID::PW_SAFE_ENABLE> {
 public:
  struct CommandPayload {
    uint8_t user_password[30];                <-------------- ten bytes more buffer?!
    ...
};

And in fact its the strcpyT that seemingly reports this error, if I were to make a guess:

    template <typename T>
    void strcpyT(T& dest, const char* src){

        if (src == nullptr)
//            throw EmptySourceStringException(slot_number);
            return;
        const size_t s_dest = sizeof dest;
        LOG(std::string("strcpyT sizes dest src ")
                                       +std::to_string(s_dest)+ " "
                                       +std::to_string(strlen(src))+ " "
            ,nitrokey::log::Loglevel::DEBUG_L2);
        if (strlen(src) > s_dest){
            throw TooLongStringException(strlen(src), s_dest, src);  <---- password does not fit into 20 byte buffer
        }
        strncpy((char*) &dest, src, s_dest);
    }

[d-e-s-o]

I can think of three possible fixes:

• Change libnitrokey to return a wrong password error instead of a string too long error.

Ultimately I'd say that this does sound like the best approach.

If all commands used in libnitrokey were to have a buffer that is, say, five bytes larger than the longest accepted user password that would make at least this case rather unlikely :D

[d-e-s-o]

1. Aren't we lacking a step to clear the PIN from the cache after it has been changed? If I have the current user PIN cached, then change it, then ask for an OTP I see a message about a wrong password. We are using the old password still.

I doubled checked that this is indeed a problem and fixed it in devel.

[robinkrahl]

I can think of three possible fixes:

• Change libnitrokey to return a wrong password error instead of a string too long error.

Ultimately I'd say that this does sound like the best approach.

Thinking about it again, this would not solve the underlying problem: If there is a library error before the command is sent to the device – in this case, an overlong password, but in other cases, it could also be an invalid OTP slot etc. –, we don’t know if the password is correct and should not cache it.

It might be possible to solve this issue differently: In try_with_pin_and_data_with_pinentry, we could check whether the passphrase is already cached by adding the --no-ask option to the GET_PASSPHRASE command. If not, we call pinentry again, this time without --no-ask. Then we would always clear the passphrase if an error occurred and it has not been in the cache previously. This would make sure that the passphrase is only cached after an operation has been executed successfully.

[d-e-s-o]

If there is a library error before the command is sent to the device – in this case, an overlong password, but in other cases, it could also be an invalid OTP slot etc. –, we don’t know if the password is correct and should not cache it.

I am not sure I necessarily follow the conclusion you are drawing ("should not cache it"). The way I see it there are two main cases to consider:

1. If the user enters the wrong password and we error out due to an unrelated issue, I'd assume that the retry count is unchanged

   • if that is the case, retrying with the same password and a presumably fixed input (lets say a corrected OTP slot) would then result in a single decrement of the retry counter, because now the password would be checked
   • that's fine, because it would already have been decremented the first time, had other inputs been proper
   • assuming we subsequently clear the cache (because now we know that the password was wrong because of the return value indicated by libnitrokey [i.e., following your suggestions]), we should be good

2. If the user enters the correct password right away, we now behave correctly (or rather: as one would intuitively expect) in all cases, even if the user made some other input mistake: the password would still be cached properly and we would not ask for it again

With your proposal, on the other hand, we would ask the user again for the password had it not been cached so far; while not a correctness issue, I'd say that is at least somewhat annoying.

So my take would be that we would need to verify that the retry counter would indeed be left unchanged if other inputs are invalid. If that is the case, I'd still be of the opinion that your original proposal is the most appealing. If that is not the case, I'd argue that's a firmware bug, but I'd be happy to be convinced otherwise.

Did I overlook something in this assessment?

[robinkrahl]

Did I overlook something in this assessment?

What led me to a different conclusion is the fact that the next attempt to use the passphrase does not necessarily happen directly after entering the passphrase. (I actually had this case a few days ago.) So you might get some library error, walk away and come back after 9:59 minutes, just to get a Wrong Password error without even entering a passphrase. In the best case, this is really confusing. In the worst case, this is the first step to locking your Nitrokey device.

[d-e-s-o]

You are free to set a 9:58 minute TTL for the cache ;-) But more seriously, that just sounds like the nature of caching to me.

In any case, given that we disagree, and this is a tiny issue with (in my opinion) no correctness issues on either side of the fence, let's just roll with either. I am happy to accept your proposed try_with_pin_and_data_with_pinentry change.