Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V1.5 : NK3 currently accepts any PIN on HOTP secret sealing #30

Closed
tlaurion opened this issue Apr 16, 2024 · 20 comments
Closed

V1.5 : NK3 currently accepts any PIN on HOTP secret sealing #30

tlaurion opened this issue Apr 16, 2024 · 20 comments

Comments

@tlaurion
Copy link
Contributor

tlaurion commented Apr 16, 2024

Edit: Renamed title after #30 (comment) this is not just counter problems, nk3 requires another PIN (unknown until now) to be set which is not set and accepts anything provided. This VOIDS trustworthy remote attestation against nk3 altogether.

Explanations on nk3 implementation vs previous nitrokeys and librem key(nk2 pro) implementations at #30 (comment)


On boot:
image.jpg

On HOTP sealing:
PXL_20240422_205259223.MP.jpg

Those counters are used under Heads logic to facilite OEM->User first time use until re-ownership is run to control the security components of the computer. If signature age is young and PIN counter was 3, as it should be reported, both user experience and OEM experience would be facilitated.

But bugs prevent Heads code to do the right thing.

@tlaurion
Copy link
Contributor Author

Nevermind, #31 didn't fix it and I won't fix that tonight.

@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 17, 2024

Why https://github.com/Nitrokey/nitrokey-hotp-verification/blob/master/src%2Freturn_codes.c#L38?

At https://github.com/Nitrokey/nitrokey-hotp-verification/blob/master/src%2Fmain.c#L110?!

@szszszsz @sosthene-nitrokey ?

To be honest if I understand logic correctly, nothing should be outputted, since it is true that there is no pin retry counter here; heads doesn't try default PIN since key has been generated for a while compared to current time. Maybe this is old artifact prior of linuxboot/heads@99673d3

@daringer
Copy link
Collaborator

The firmware version is also obviously incorrect, I suppose some meta-data-struct is not parsed correctly

@tlaurion
Copy link
Contributor Author

The firmware version is also obviously incorrect, I suppose some meta-data-struct is not parsed correctly

Opened #33 to track this

@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 17, 2024

This issue is fixed under #32 and linuxboot/heads#1640

Edit: Not fixed.

@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 22, 2024

@daringer @jans23 @szszszsz impacts of this bug detailed at linuxboot/heads#1645 (comment) since hotp_verification output used to take various decisions under Heads

As of now, without going too deep in the details, the oem provisions with either default PINs or custom ones, where the current codebase takes decisions based on hotp_verification output for PIN retry counts (wrong as of writing), public signature age to decide if HOTP can be sealed with the default PIN. This bug was filed under #30


Edit: modified OP with additional picture and more succinct explanations of the impacts of this bug.

@daringer
Copy link
Collaborator

daringer commented Apr 23, 2024

ahhh, ok this makes perfect sense reading: #30 (comment)

I was looking in detail here and think I can clear up things:

There are significant changes between the Nitrokey Pro2/Start/Storage architecture and the Nitrokey 3 architecture.

The most important one is how (reverse) "HOTP" is accessed. The legacy devices use the OpenPGP Card Admin pin to verify write operations on these devices (e.g., adding/setting a HOTP secret), this is also why there was a "Pin counter" reported by hotp-verification.

For the Nitrokey 3, the OpenPGPCard and the (reverse) HOTP functionality are fully separated. This goes that far that the "secrets-app" (the component for the (reverse) HOTP verification) does have a fully independent PIN. Furthermore it is also not required to set this pin prior to creating a (reverse) HOTP entry.

So under the line the current output of hotp-verification info (for NK3) is correct, because:

  • 4.13 is the correct secrets-app firmware version
  • PIN is not set - set PIN before the first use is also correct (as it was never set)

Although "correct", this is clearly not what we want here, as the behavior should be (mostly) agnostic for any token.

Essentially we should do one of the following to resolve that:

  • "fake" a admin pin counter, and always output some number >= 3 (minimal invasive, I would suggest this)
  • change the hotp-verification behavior to also set the pin on the secrets app during initialization (this would most likely break backwards compatibility with already enrolled nitrokey 3 devices and on top might confuse users, which are using the secrets-app for more than only HEADS-based hotp)

@tlaurion do you have a preferred (or different) way to resolve that ?

@JonathonHall-Purism
Copy link

@daringer Could you clarify the behavior of the independent HOTP PIN please? (I don't have any of the newer tokens)

  • If it's not set, does this mean a reverse HOTP initialize works with any PIN / the PIN is ignored?
  • Once it's set, are there a limited number of attempts allowed (like the PGP card admin PIN) or are the attempts unlimited?
  • Is there a way Heads could set the reverse HOTP PIN?
  • Is there a way Heads can tell whether the token's HOTP and PGP card admin PINs are shared or independent?

I tend to think that we should not try to hide this behavior from Heads, it seems like that would create a long drawn out problem of Heads not being able to correctly say which PIN it is asking for, not being able to reset PINs correctly, etc. I think Heads is pretty close to handling this behavior correctly too.

It looks like seal-hotpkey is mostly correct, it already says it is asking for the HOTP key admin PIN (not PGP card PIN). It already looks for the card counters and could detect "PIN is not set" without too much trouble, perhaps not ask for a PIN at all in that case rather than asking for one that doesn't exist yet.

OEM reset probably should consider this an additional PIN/password component if the HOTP pin is separate. It already negotiates several other components, so I think we could integrate this too.

@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 24, 2024

Essentially we should do one of the following to resolve that:\n\n"fake" a admin pin counter, and always output some number >= 3 (minimal invasive, I would suggest this)\nchange the hotp-verification behavior to also set the pin on the secrets app during initialization (this would most likely break backwards compatibility with already enrolled nitrokey 3 devices and on top might confuse users, which are using the secrets-app for more than only HEADS-based hotp)\n@tlaurion do you have a preferred (or different) way to resolve that ?

None of those solutions are "secure" @daringer and are regressions compared to nk1/nk2 I'm afraid (start didn't support reverse hotp and were never supported by hotp boards flavors nor the docs). The whole remote attestation concept is void today. :(

What I'm reading is that anyone having access to both unattended nk3 and laptop at the same time can actually tamper firmware and regenerate HOTP without the USB security dongle validating what up to today was thought being the GPG Admin PIN nor decrement its counter to 0 which is a regression compared to a nk1/nk2, on which bad Admin PIN would lock Admin out and require a USB Security dongle reset, which could not go unnoticed. While now it could be? HOTP basically would always show "Success" for whatever Admin PIN entered and only TPMTOTP counter will be invalid requiring user to resign /boot on next boot?!? Basically remote attestation on phone at each bokt is the only thing trustable today when having a NK3?

Am I getting the facts right?

@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 24, 2024

@JonathonHall-Purism

If it's not set, does this mean a reverse HOTP initialize works with any PIN / the PIN is ignored?

It does.

I entered 123456789 as Admin PIN (where it was expected to be 12345678 per factory reset on my test machine without known change of behavior) with the Regererate HOTP option.... and it succeeded.

@tlaurion tlaurion changed the title V1.5 : card counters : pin not set output, when set V1.5 : NK3 accepts bad ADMIN PIN on HOTP secret sealing Apr 24, 2024
@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 24, 2024

Otherwise I totally agree with @JonathonHall-Purism implementations sugestions.

The PIN needs to be seperated because it should and could be locked on its own, so the user will need to remember another PIN.

If I understand correctly, if that PIN is forgotten, that requires a reset from nitropy today? The Admin PIN cannot unlock that one? There is no way to set that PIN today?

@tlaurion tlaurion changed the title V1.5 : NK3 accepts bad ADMIN PIN on HOTP secret sealing V1.5 : NK3 currently accepts any PIN on HOTP secret sealing Apr 24, 2024
@tlaurion
Copy link
Contributor Author

tlaurion commented Apr 24, 2024

It looks like seal-hotpkey is mostly correct, it already says it is asking for the HOTP key admin PIN (not PGP card PIN).

No. It asks for the (GPG) Admin PIN.

As seen in original OP:

On HOTP sealing:
PXL_20240422_205259223.MP.jpg

You can type whatever you want there. It will pass and seal HOTP (remote attestation of whatever firmware state there is) without PIN protection nor lockout on nk3. Counter of 0 means locked per current assumptions. Which is why "not trying default PIN". Basically the HOTP secure element fails open.

A variation on the nk3 mini (picture): you need to touch the dongle for physical presence. On nk3 NFC it passes without further notice.

A reminder of what is sold to the user
https://archive.is/ho7Rm

@daringer
Copy link
Collaborator

daringer commented Apr 24, 2024

  • If it's not set, does this mean a reverse HOTP initialize works with any PIN / the PIN is ignored?
  • Once it's set, are there a limited number of attempts allowed (like the PGP card admin PIN) or are the attempts unlimited?
  • Is there a way Heads could set the reverse HOTP PIN?
  • Is there a way Heads can tell whether the token's HOTP and PGP card admin PINs are shared or independent?
  • yes
  • limited, the secrets app has 8 tries
  • not currently, hotp-verification needs changes for that
  • no, they are always separated

no discussion, that's a regression - we are already planning the changes and the respective changes.

None of those solutions are "secure"

we plan to have the same behavior, which would be option 2, this would make it behave like the pro?
In a 2nd step we could then add this pin explicitly to HEADS to be more transparent for the user, if the user requests that?

@JonathonHall-Purism
Copy link

It looks like seal-hotpkey is mostly correct, it already says it is asking for the HOTP key admin PIN (not PGP card PIN).

No. It asks for the (GPG) Admin PIN.

Yeah you're right. It's different from what GPG says for the admin pin, and it made sense to me as "different" when thinking about this issue, but it's not really that different.

@JonathonHall-Purism
Copy link

  • no, they are always separated

Well I meant "for any token" know whether they are the same or separate. So I guess at this moment we'd have to hard-code a list of tokens. But I suppose it's a moot point if we're moving back to unifying them again.

we plan to have the same behavior, which would be option 2, this would make it behave like the pro?
In a 2nd step we could then add this pin explicitly to HEADS to be more transparent for the user, if the user requests that?

By option 2, do you mean:

  • change the hotp-verification behavior to also set the pin on the secrets app during initialization (this would most likely break backwards compatibility with already enrolled nitrokey 3 devices and on top might confuse users, which are using the secrets-app for more than only HEADS-based hotp)

My gut reaction is that while this is better than the current Heads behavior with these tokens, it seems like an uncomfortable middle ground overall. The two pins are both reset during OEM reset, but if you change one it won't change the other. (If I understand the idea correctly, anyway.) I'm thinking about how I'd explain this to a user and it seems challenging to keep track of.

What is the use case for a user wanting these two PINs to be separate, at least separate such that changing one doesn't change the other?

Re: breaking compatibility with already-enrolled NK3 devices - I think breaking compatibility to the extent that an HOTP re-seal is needed is OK. Updating Heads requires an HOTP re-seal anyway. It should not require any further manual steps though. (But we could add additional logic to Heads during HOTP reseal if needed as long as it's automatic, IMO.)

@daringer
Copy link
Collaborator

ok just assembled a clearer plan - we will update hotp-verification + nk3 firmware asap - a first version + PR should be available this week.

The plan is to essentially make the reverse-hotp secret write-protected, if a pin is set for the secrets-app on the nk3. For now I would suggest that we keep the overall behavior similar to what we have today, which essentially is: gpg-pin is equal to secrets-pin.

There is only one combination which is critical: the user has set a secret-pin on their own, which is different from the gpg pin - if this happens hotp-verification will fail to write the hotp-secret and I would suggest to inform the user at this point about this and allow to change the secrets-pin. We'll take a look at what makes sense here and directly implement it in the upcoming PR - I would suggest to talk about the options there ...

@daringer
Copy link
Collaborator

daringer commented Apr 24, 2024

What is the use case for a user wanting these two PINs to be separate, at least separate such that changing one doesn't change the other?

It's more of an architectural decision to maintain multiple pins on the nk3 ... there are different apps, which all should be independant: fido2, openpgp, secrets, (piv) ... all these have to maintain their own pin, sharing these might generate quite some confusion that's why we decided against (e.g., someone changing the fido2-pin through the browser and this would then lead to the (or better some?) gpg-pin would change)

But essentially this is also the reason I would not explicitly tell the user that gpg and secrets are different pins, HEADS should just enforce that both are equal during oem-factory reset (or hotp regenerate) ... this will then only fail if the user has set a secrets-pin by themselves - this case should be caught and handled only if it occurs from my point of view.

@daringer
Copy link
Collaborator

daringer commented Apr 25, 2024

started working on a solution: 537957e

the plan is to first verify full backwards compatibility, the following situations are to be verified (with nk3 firmware <= v1.7):

in all cases we have: fully set up HEADS, just replaced hotp-verification (flash new heads version), re-creating hotp needed

  • secrets-app pin: not set: secrets-app pin will be set to gpg-admin-pin (during hotp-init) , no changes expected during hotp-verification check
  • secrets-app pin: set to gpg-pin: secrets-app pin will not be set (during hotp-init) , no changes expected during hotp-verification check
  • secrets-app pin: set to unknown-pin: secrets-app pin will not be set (during hotp-init) , no changes expected during hotp-verification check (this is the case which needs to be handled in HEADS, the user shall be asked for the unknown secrets-app pin)

for all operations a user-presence check is expected (and the pin is not), with the updated NK3 firmware, additionally for overwriting a reverse-hotp entry a user-verification (pin) will be needed

@tlaurion
Copy link
Contributor Author

tlaurion commented May 6, 2024

I saw Nitrokey/trussed-secrets-app#114

hotp-verification code is 8e63fa4

@tlaurion
Copy link
Contributor Author

Fixed with work related around linuxboot/heads#1684 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants