Replace FIDO2 PIN storage with its hash

[szszszsz]

Replace literal FIDO2 PIN storage with its salted hash. Work in progress. Request for comments.

Edit: the basic idea is to store, and manipulate the hash of the PIN, instead of raw data. Additionally add salt, to prevent PIN recovering via rainbow tables in the event of MCU's flash content is read.

[szszszsz]

Build passed. To clean-up and test.

[nickray]

Nice, I'll defer to @conorpp regarding technical details, he has limited online availability currently though.

(How) do you intend to test this? FYI, we recently started moving out tests to https://github.com/solokeys/fido2-tests. Might also involve solokeys/solo1-cli#20.

[nickray]

@all-contributors add szszszsz for code

[szszszsz]

(How) do you intend to test this? FYI, we recently started moving out tests to https://github.com/solokeys/fido2-tests.

It should suffice to run the tests a couple of times against a physical device, with some power-cycles. I do not know the new tests yet, will check.

One last thing before merging - a transition code has to be written to port the raw PIN to the hashed version, perhaps leaving out the old field in the STATE struct.

[My1]

generally a nice Idea although the question is whether or not it will help much.

after a quick Readthrough of your commit this looks like a sha256 with the pin and salt. and since the salt are generally stored next to each other (and they are both in that STATE thing, honestly I didn't check the fw enough to know anything serious about it) and in that case if one can read the PIN currently (which should be stored at the most secure location possible) they would also be able to read the hash and the salt, and sha256 cannot really be compared to fun things like argon2 or even bcrypt.

SHA256 is by design a very fast function and for it's usage scenarios that's a good thing, but password storage is not a sha256 scenario. let's look at this piece of fun:

https://gist.github.com/binary1985/c8153c8ec44595fdabbf03157562763e
a benchmark of a whole bunch of hashes or other things to store passwords with on a single RTX 2080Ti

to make it simple what we are looking for is hashmode 1410 sha256($pass.$salt), where we have "pinhashenc" (which kinda already sounds like a hash but I am not going to question variable naming or anything here) and salt obviously being a salt.

there we have 7074,7 Million hashes per second, add more gpus for more hashes.
depending on the strength possibilities of that PIN, that may be anywhere between not awesome (depending on user choice) to utterly pointless.

for example a SIM card PIN supports anywhere from 4 to 8 digits (and assuming less digits are not cast into a higher digit format by prepending zeros or whatever) that should be about
10000 (4 digits)
+100000 (5 digits)
+1000000 (6 digits)
+10000000 (7 digits)
+100000000 (8 digits)
=111110000

according to Math we are done in less than 16 msec in this scenario, meaning also one can use MUCH cheaper and less expensive solutions and still be reasonably fast

also even if one can set much more decent things there is the user choice and perception problem. a PIN after all is generally a Personal Identification Number most people would not use anything that isnt a number just based on what they know a pin to be, and on top they would most probably go to 4 digits as bank cards (at least the ones I know) only do 4 digits and SIM cards (at least the ones I know) all come with a 4 digit pin by default. and with a 4 digit pin, probably even the most potato PC can brute force that thing in not too long (the RTX2080Ti is finished in less than 1,5 microseconds by the way)

so while generally a good Idea I am not entirely sure about how useful this actually is

[conorpp]

It's good to have -- it may not be that effective against simple, low character PINs, but it'd be good to support those that do have more secure PINs.

Also note that PINs for CTAP2 are anything utf8, so not just numbers (despite it being called PIN).

[conorpp]

Re the transition code, maybe add another byte in the STATE struct to indicate the PIN format? 0xff == plaintext, and maybe 0x00 == hash.

[My1]

It's good to have -- it may not be that effective against simple, low character PINs, but it'd be good to support those that do have more secure PINs, as said, bank cards I know .

well depends on how long you can go with PINs only do 4 digits, while for SIM cards go to 10.

Also note that PINs for CTAP2 are anything utf8, so not just numbers (despite it being called PIN).

okay that changes a lot, but the user needs to know that,, which with the basic perception of the word "PIN" isnt gonna go anywhere, why even say "PIN" for something that clearly isnt, the bank I am also calls their online banking password pin despite it even forcing 3 character classes on you.

I mean even if you can do a lot in the scope of security, if a user doesnt know that it kinda sux.

Re the transition code, maybe add another byte in the STATE struct to indicate the PIN format? 0xff == plaintext, and maybe 0x00 == hash.

good idea.

[nickray]

PIN terminology comes from FIDO (so we should use that terminology), all we can do is document :)

@My1: it's useful to at least browse over https://fidoalliance.org/specs/fido-v2.0-rd-20180702/fido-client-to-authenticator-protocol-v2.0-rd-20180702.html

[My1]

well then this obviously means that Fido screwed up here, and should have chosen a better terminology, as Solo isnt the first one to support Fido2 and therefore not the first to use the terminology (because fido obviously set it) I am not even blaiming them or anything, but it doesnt change the fact that the common perception of "pin" is usually "4 digits"

[szszszsz]

@My1 Hi! Thank you for the input. Regarding PIN name, please continue discussion in a separate ticket, as this is not a topic for this one, and only clutters the subject.

As for the main matter, I should have specified indeed the targets for this change:

• remove clear text storage of the user's password (with assumption it is reused by user in other place), and make it harder to recover by using 256-bit salt;
• allow to accept PINs without maximum length constraint.

As you have mentioned, it would not be impossible to recover the PIN, although it will be a bit more complicated, than simple reading out the flash content. It is rather not possible to increase the computation time significantly to bother any GPU, thus making your claim valid that security-wise this might be pointless. Still there is the PIN length advantage - specification defines 256 bytes as the maximum size, if I am not wrong (FIDO2#PIN). Having it as hash allows to store any length of the input data (more than currently set in Solo 64 bytes limit).

To make it more computation hungry, PBKDF2 could be used (but limited to 1000 iterations for usability, to take no more than 1 second). Some different crypto-operations mix might be good as well to break the GPU's cache and making the final performance worse, but I do not think this would change the outcome much.

Edit: Correct me, if I am wrong, but with using roughly each of the characters' groups (80+ possible characters - small letters, big letters, digits, specials), you still get 25 years per GPU on average, with 10-character PIN:

t = (80^10)/7074700000 /2

Calculations: math

If you are using UTF-8, the space to search per byte has to be further multiplied 2-4 times (as character will take 1-4 bytes now). So it seems this is not as insecure, as you have stated initially.

[szszszsz]

@conorpp Format flag sounds good! Will implement it.

[szszszsz]

@My1 Last thing I wanted to mention is, that this is a storage of an incomplete hash of the input PIN (only the first 16-bytes), meaning that aside from getting the real PIN, and other usual sha256 collisions, you will get as well 2^128 other results, making it really hard to filter out the PIN by hand (even with filtering out the non-printable results).

Given this in the end I think the original PIN is pretty well concealed, and I change my mind: it is rather impossible to reveal user's PIN from the device's flash memory content with this implementation.

Edit: @My1 still thank you for this thought-provoking / design verification questions!

Edit: SHA2-512 has 1/3 of the SHA256 performance - another option to consider.

[My1]

oh I didnt know that you are storing the hash incompletely, which certainly sounds fun, as long as the extra collisions don't actually decrease security.

on the other hand what would the PIN knowledge do for an attacker. the pin is stored at least with the same security as any private keys the device has because that's what the fido link says. that means if they get that far they can basically just go and log into any site the user registered with that isnt having an online-password and is just relying on the device-password ("pin")

[szszszsz]

The idea is to not reveal the actual user's PIN, e.g. in case of the device being stolen. This will be harmful in case the PIN is shared among online accounts.
Agreed, that the moment the privates keys are revealed, the 1FA (FIDO2 resident keys) will make the accounts compromised. For 2FA there are still the user/passwords credentials (if not stolen already from user's PC).

Edit:

oh I didnt know that you are storing the hash incompletely, which certainly sounds fun, as long as the extra collisions don't actually decrease security.

This is why device has only 8 attempts for entering the valid PIN, and this is the main difference between the PIN and the password.

[My1]

about your Idea with SHA512 to further knock down performance, while technically not wrong we need to mind that the solo is just a small device with an obviously pretty weak cryptoprocessor so we can't go down too harsh on the solo.

also a fun thing is that passwordless with fido pin is usually advertised as 2FA and even as a better 2FA than just going the classic U2F route (and it certainly does have advantages, like that you don't send out the knowledge to the service to prove it) because you have 2 more or less distinct factors, the keys on the device and the pin to make the device use the keys, technically not wrong though.

I think the most obvious solution is obviously preventing readout by any means possible, e.g. unless already happening, make the chip super tamperproof so that if someone tries to physically read the chip out by screwing around with it itself that the data goes boom, in a similar way to an intrusion switch on some PC cases but just with more ramifications than just a warning.

[szszszsz]

I have added some basic migration framework code, to keep it in order, and corrected the final hash write, which was using too short buffer. Rebased on the current master.
Please review.
Checked simulation's hexdump of the STATE structure, with uninitialized PIN. To test with the PIN initialized, and with FIDO2 login before, and after the migration. Finally the same on the hardware.

Edit: FIDO2 tests passes with the simulation. Run multiple times.

[conorpp]

Thanks!!