search

Nitrokey / nitrokey-start-firmware

A mirror of Gnuk's 1.0.x and 1.2.x branches.
56 stars 15 forks source link

upgrade_by_passwd.py can brick the key on single invocation #12

Open nakato opened 5 years ago

nakato commented 5 years ago

A single invocation of "upgrade_by_passwd.py" with the wrong admin key will brick the key in a single run if factory_reset=no

I would not expect the tool to try a single pin 3 times in a row without prompting, it would probably be best to make this less aggressive.

alex-nitrokey commented 5 years ago

@szszszsz I think, a fix would be to delete this loop. In my opinion a user, not the script should restart if something failed.

But this may should be fixed upstream, don't you think? Shall I ask NIIBE on the gnuk-mailingslist?

szszszsz commented 5 years ago

@nakato Agreed.

@alex-nitrokey That is a good candidate for the cause indeed. The loop should only continue in a case, where the connection issues had occurred, not when the PIN had been incorrect. It would be the safest to remove it for now. Could you prepare a patch for NIIBE and send him via the mailing list? It would be faster to explain the problem, when the fix is presented as well, and perhaps it will be merged directly. We should apply the patch in our repository anyway, until the handling is improved.

alex-nitrokey commented 5 years ago

I created a PR in #13

I added a check if auth attempt actually worked. The loop breaks if the authentication failed. This helps to keep the feature of killing scdaemon automatically intact.

jans23 commented 5 years ago

Did you check if latest Gnuk contains a fix for that issue already, perhaps?

alex-nitrokey commented 5 years ago

Did you check if latest Gnuk contains a fix for that issue already, perhaps?

Well... actually I should have, but I didn't.

alex-nitrokey commented 5 years ago

Doesn't look like it.