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

Support the password safe #18

Closed
robinkrahl opened this issue Dec 19, 2018 · 5 comments
Closed

Support the password safe #18

robinkrahl opened this issue Dec 19, 2018 · 5 comments
Assignees

Comments

@robinkrahl
Copy link
Collaborator

nitrocli should support the password safe (PWS). The password safe is unlocked using the user password. Once it is unlocked, any client can access the PWS without further authentication. The PWS can be locked using the NK_lock function. On the Pro, NK_lock does not have side effects. On the Storage, NK_lock also closes the encrypted device. I requested a lock function for the PWS without side effects (see Nitrokey/nitrokey-storage-firmware#65), but currently, there is none. (Ideally the PWS communication would be protected by a temporary password similar to the OTP communication.)

So we have three options:

  • Do not support the PWS until we can lock it without side effects.
  • Do not lock the PWS after every use, leaving it unprotected until the device is removed or the machine is restarted.
  • Lock the PWS after every access, potentially closing the encrypted volume. We could try to check whether the volume is enabled and warn the user.

What do you think?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 19, 2018

Thanks for bringing up this issue.

On the Storage, NK_lock also closes the encrypted device.
Ugh. Out of curiosity: do you know why that is? Is that "just" a bug or is there a rationale behind connecting the two?

So we have three options:

  • Do not support the PWS until we can lock it without side effects.
  • Do not lock the PWS after every use, leaving it unprotected until the device is removed or the machine is restarted.
  • Lock the PWS after every access, potentially closing the encrypted volume. We could try to check whether the volume is enabled and warn the user.

I think it would help to understand whether the witnessed behavior is a bug or a feature (see above). Also, mostly for my understanding: you are doing most of the work right now and I'd like to understand your objective some more. Is your plan to expose pretty much all the functionality the device (or libnitrokey) supports, or are you mostly interested in implementing the features you'd have a strong use case for? If the latter, do you plan to use the PWS? (that sort of ties into your first bullet point)

I don't think I have a strong opinion about options 2 & 3, other than stating that I'd rather not have a non-obvious side-effect like this take effect. So silently closing the encrypted volume is something I would not do. We could ask the user to take measures to close the volume and only allow the command to go through after that. Alternatively we could also ask how to proceed or, perhaps better, avoid interactivity by adding a --force option that will allow the close to go through, with the side-effect of closing the encrypted volume if open.

I would also not want to leave the device in an unlocked state (due to the reason mentioned above) iff we decide to go down the road of locking after every access. That means if we lock after every access we would need to check the state of the encrypted volume before opening the PWS. That, however, leaves us with the potential of a race condition where the encrypted volume is opened after the PWS is opened for a one-time access but before it is closed (unlikely, and perhaps prevented by serialization inside the device, but theoretically possible).

Given that last complication, let me ask what would be the advantage of the lock-on-every-access case? Given that we open the device, for whatever short amount, and it is connected to the computer during that time, I think we cannot be concerned about the computer being compromised (as then all bets would be off). Hence, at least from a theoretical standpoint it ultimately should not matter whether the PWS stays open (I am no security expert, but that's how I see it).

So that moves us away from a lock-on-every-access implementation, I would say. Now I would still think that allowing for a manual close is a nice to have (perhaps with the --force option). I am thinking of the case where, say, the stick is left with the workstation but the user leaves for a while. It would be good to allow for an explicit close of the PWS in such a case (perhaps triggered by a screen saver/untrusted screen lock).

I guess that rambling may have helped me more than it did help you. I believe I may have convinced myself that the most suitable implementation is similar to your second bullet point where the device is left open but with the possibility of closing it manually. The one open question is whether to explicitly let the user open it or do that implicitly through a get, set, clear, or status (list taken from issue #4 ). I think doing the open (unlock) implicitly but allowing for an explicit close (lock) is fine. So the list would be expanded by a close (or lock). Does that make sense?

@robinkrahl
Copy link
Collaborator Author

Ugh. Out of curiosity: do you know why that is? Is that "just" a bug or is there a rationale behind connecting the two?

I don’t know. I think they just did not see any reason to separate the two.

Also, mostly for my understanding: you are doing most of the work right now and I'd like to understand your objective some more. Is your plan to expose pretty much all the functionality the device (or libnitrokey) supports, or are you mostly interested in implementing the features you'd have a strong use case for? If the latter, do you plan to use the PWS? (that sort of ties into your first bullet point)

For the nitrokey crate, my goal is to provide all major features of libnitrokey. I might not provide every single function, but developers should be able to use all Nitrokey features.

For nitrocli, my primary objective is to implement support for the Nitrokey features I intend to use – this includes OTP, PIN operations, firmware locks and so on (probably not the PWS). But as I work on nitrokey anyway, my secondary objective is to implement all available (and reasonable) features – of course only to the extent you agree to. If nothing else, I don’t want anyone to feel the need to create yet another application e. g. for the password safe.

Given that last complication, let me ask what would be the advantage of the lock-on-every-access case? Given that we open the device, for whatever short amount, and it is connected to the computer during that time, I think we cannot be concerned about the computer being compromised (as then all bets would be off). Hence, at least from a theoretical standpoint it ultimately should not matter whether the PWS stays open (I am no security expert, but that's how I see it).

We would send the open, read and lock commands with virtually no delay. It is hard to insert a command without interference. I agree that you are in a bad position if the computer is compromised, but there is a difference regarding the viability of attacks.

I guess that rambling may have helped me more than it did help you. I believe I may have convinced myself that the most suitable implementation is similar to your second bullet point where the device is left open but with the possibility of closing it manually. The one open question is whether to explicitly let the user open it or do that implicitly through a get, set, clear, or status (list taken from issue #4 ). I think doing the open (unlock) implicitly but allowing for an explicit close (lock) is fine. So the list would be expanded by a close (or lock). Does that make sense?

I basically agree, but I would not call it close if there is no open (also for consistency with the volume operations). Instead, I’d introduce a new top-level command lock. This would make it clear that it does not only affect the PWS.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 19, 2018

For nitrocli, my primary objective is to implement support for the Nitrokey features I intend to use – this includes OTP, PIN operations, firmware locks and so on (probably not the PWS). But as I work on nitrokey anyway, my secondary objective is to implement all available (and reasonable) features – of course only to the extent you agree to. If nothing else, I don’t want anyone to feel the need to create yet another application e. g. for the password safe.

Makes sense, thanks!

We would send the open, read and lock commands with virtually no delay. It is hard to insert a command without interference. I agree that you are in a bad position if the computer is compromised, but there is a difference regarding the viability of attacks.

Hm. I am not quite convinced. The mere fact that the PIN is entered through the user's keyboard and not a dedicated key pad is probably already a no-go if the computer is not to be trusted. That's not even speaking of the encrypted volume, which obviously would be exposed (although I can see different security domains being a possibility). I do see a difference to actions that require something non-extractable on the stick (e.g., signing using the non-extractable private key), but a password that can just be copied in a one-time operation?

I guess that rambling may have helped me more than it did help you. I believe I may have convinced myself that the most suitable implementation is similar to your second bullet point where the device is left open but with the possibility of closing it manually. The one open question is whether to explicitly let the user open it or do that implicitly through a get, set, clear, or status (list taken from issue #4 ). I think doing the open (unlock) implicitly but allowing for an explicit close (lock) is fine. So the list would be expanded by a close (or lock). Does that make sense?

I basically agree, but I would not call it close if there is no open (also for consistency with the volume operations). Instead, I’d introduce a new top-level command lock. This would make it clear that it does not only affect the PWS.

Yeah, that seems fine, too. I am also not opposed to switching to the lock-after-use scheme once the problem of closing the encrypted volume is solved; in that case if we go with a top-level lock command we would not even have to remove it (well, perhaps for the Pro stick, not sure).

Hm, sort of off-topic, but do you see value in or have you planned providing context sensitive commands? As in, if the Storage stick is present then let's provide storage commands, otherwise don't. Similarly, Pro-specific commands would not show up if a Storage stick is present. I am tempted to say "too much" (and also perhaps not that reliable), but haven't thought about it much.

@robinkrahl
Copy link
Collaborator Author

robinkrahl commented Dec 19, 2018

Hm. I am not quite convinced.

Let’s agree to disagree. It’s not that important after all.

Hm, sort of off-topic, but do you see value in or have you planned providing context sensitive commands? As in, if the Storage stick is present then let's provide storage commands, otherwise don't. Similarly, Pro-specific commands would not show up if a Storage stick is present. I am tempted to say "too much" (and also perhaps not that reliable), but haven't thought about it much.

Interesting thought. As argparse implements subcommands using enums, I don’t think we can currently do that as it would require us to dynamically modify the enum. We could use two different Command enums for the two models, but that would make the argument parsing code even more complicated. And I think as a user, I would be confused if the output of --help changes depending on the devices I connect to my machine.

robinkrahl added a commit to robinkrahl/nitrocli that referenced this issue Dec 27, 2018
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue d-e-s-o#18 for
details on the locking mechanism.
robinkrahl added a commit to robinkrahl/nitrocli that referenced this issue Dec 27, 2018
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue d-e-s-o#18 for
details on the locking mechanism.
robinkrahl added a commit to robinkrahl/nitrocli that referenced this issue Dec 27, 2018
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue d-e-s-o#18 for
details on the locking mechanism.
robinkrahl added a commit to robinkrahl/nitrocli that referenced this issue Dec 27, 2018
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue d-e-s-o#18 for
details on the locking mechanism.
d-e-s-o pushed a commit that referenced this issue Dec 29, 2018
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue #18 for
details on the locking mechanism.
@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 29, 2018

I believe there is no more work to be done on this front. Correct me if I am wrong.

@d-e-s-o d-e-s-o closed this as completed Dec 29, 2018
d-e-s-o pushed a commit that referenced this issue Jan 2, 2019
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue #18 for
details on the locking mechanism.
d-e-s-o pushed a commit that referenced this issue Jan 8, 2019
This patch implements the lock command that locks the password safe and,
on the Nitrokey Storage, the encrypted volume.  See issue #18 for
details on the locking mechanism.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants