boltcard / bolt-nfc-android-app

Program your bolt card easily from your android mobile with this app
35 stars 14 forks source link

Fault in key change algorithm leading to bricked cards #42

Closed johnheenan closed 1 week ago

johnheenan commented 1 month ago

I am alleging there is an algorithmic fault that leads to cards becoming unnecessarily bricked when the five app keys are changed in a boltcard, such as with 'reset card' or 'set card'.

Section 10.6.1 of the NXP NTAG 424 DNA data sheet at https://www.nxp.com/docs/en/data-sheet/NT4H2421Gx.pdf has two key relevant parts for this issue with regard to use of their ChangeKey command:

  1. Table 63 shows the old key is necessary to change keys 1 to 4 (but not for key 0)
  2. Table 64 shows the the response data SW1SW2 for a successful execution of a key change is 9100h

Examining the SetupBoltcard function, for example in file https://github.com/boltcard/bolt-nfc-android-app/blob/f505be16cd1436da51229374b2336d9987308841/src/components/SetupBoltcard.js, shows use of await Ntag424.changeKey function for each key change. If this function does not find 9100h in the return code from the use of the NTAG ChangeKey command by an NPM provided library then an error is thrown and caught. Starting from line https://github.com/boltcard/bolt-nfc-android-app/blob/f505be16cd1436da51229374b2336d9987308841/src/components/SetupBoltcard.js#L219 the code executed is


      setTagTypeError(error);
      setStep(SetupStep.WritingCard);
    } finally {
      setWritingCard(false);
      nfcManager.cancelTechnologyRequest();
      setReadingNfc(false);

The order in which keys are changed are 1,2,3,4,0.

Suppose only keys 1,2 and 3 have been successfully changed but change of key 4 failed with return code not equal to 9100h. This means there is no attempt to change key 0 and the card has been bricked if no further recovery action is taken: keys 1,2 and 3 are changed but keys 0 and 4 have not been.

WHY BRICKED? BECAUSE I CANNOT FIND ANY INDICATION A RETRY WILL AVOID ATTEMPTING TO SKIP ALREADY CORRECTLY PROGRAMMED KEYS, WHICH IS UNNECESSARY AND WILL FAIL IN ITS ENTIRETY BECAUSE THE OLD KEYS ARE NO LONGER VALID FOR SUCCESSFULLY CHANGED KEYS.

So what can fix this? There are of course numerous methods and they can be combined.

1) Make five catch blocks, one for each key, and retry at the individual key level, instead of one catch block for all the keys together, as now practised.

2) Record how far successful overall key change got in a React useState and resume the entire process, skipping successfully changed keys. Inform users, if necessary, they must complete the process to avoid getting their card becoming bricked.

3) Provide an option to users to have the current set of mixed old and new keys recorded on a card downloaded to retry at a later time, but that this maybe a security risk if someone else gets a hold of both their card and download of mixed keys, despite card being bricked.

4) Users should be given an option to download what the complete set of old keys is and what the new keys will be. It would be even better if the app could show or dump to the number of keys changed. This maybe a completely fool proof method to avoid cards getting bricked, even if the number of keys changed is unknown (just mean more automated tries are required). It is a serious security risk if the card and records are found, even with a currently bricked card.

robertclarkson commented 1 month ago

These sound like good suggestions to me which would prevent card keys from getting into an inconsistent state

johnheenan commented 1 month ago

I have looked into potential issues in more detail.

TLDR: Do not close a session just because the NTAG424DNA card returns an error to a single command.

Currently when a card returns an error a session is dropped in Android requiring a new tap. This is not necessary

While the NTAG424DNA datasheet uses the word 'session', the context is different from what is normally understood. A NTAG424DNA 'session' can be continued years after first starting one. Normally a session means a store of intermediate state data and is separate from how the information for state is encrypted both for communication and to prevent replays,.

For the NTAG424DNA 'session' is all about the encryption side and preventing replays. It is fair to state all state for a NAG424DNA 'session' is stored on the app side and the session can be preserved even when the NTAG goes out of range or returns an error. The way the NTAG achieves this is to program into permanent memory a command counter that is reset at AuthenticateEV2First and to program into permanent memory new session keys at AuthenticateEV2First or AuthenticateEV2NonFirst.

It is up to the app to decide what a session is. The NTAG424DNA has no need to maintain an intermediate data state, which is what is normally associated with a session. The NATG424DNA notion of session state is to cooperate with the app but not to preserve any intermediate data state, other than what is explicitly and permanently programmed in at a point in time (including command counter and session keys for cooperative purposes).

A practical example of this is that if setting one of the five app keys fails. Instead of closing the session just retry again for a set amount of times. The only difference is that the command counter will have increased. The user does not need to know, except perhaps to ask user to hold card closer or steadier.

Another practical example is that a card that has not had its full set of new or changed app keys set, programming can be resumed later, as long as the current set of programmed keys is known and the complete desired set is known.

From section 9.1.2 of the NTAG424DNA datasheet on the Command Counter:

A command counter is included in the MAC calculation for commands and responses in order to prevent e.g. replay attacks. It is also used to construct the Initialization Vector (IV) for encryption and decryption.

Each command, besides few exceptions, see below, is counted by the command counter CmdCtr which is a 16-bit unsigned integer. Both sides count commands, so the actual value of the CmdCtr is never transmitted. The CmdCtr is reset to 0000h at PCD and PICC after a successful AuthenticateEV2First authentication and it is maintained as long as the PICC remains authenticated.

From section 9.1.4 on Encryption:

When an encryption or decryption is calculated, the CmdCtr to be used in the IV are the current values.

From section 9.1.7 on Session Key Generation:

At the end of a valid authentication with AuthenticateEV2First or AuthenticateEV2NonFirst, both the PICC and the PCD generate two session keys for secure messaging,

johnheenan commented 1 month ago

If anyone wants to comment more generally on this issue, I think @robertclarkson prefer they do so on issue he repoened at https://github.com/boltcard/boltcard/issues/80.

I have edited in addiional comments at https://github.com/boltcard/boltcard/issues/80#issuecomment-2241513342

johnheenan commented 1 week ago

There is a pull request to fix boltcard bricking at https://github.com/boltcard/bolt-nfc-android-app/issues/44

johnheenan commented 1 week ago

I am closing this issue as the immediate issue is being addressed at new, more specific issue, https://github.com/boltcard/bolt-nfc-android-app/issues/44

The fix for the boltcard bricking issue has an apk release at https://github.com/johnheenan/bolt-nfc-android-app/releases/tag/v0.3.2