search

arekinath / pivy

Tools for using PIV tokens (like Yubikeys) as an SSH agent, for encrypting data at rest, and more
193 stars 26 forks source link

pivy-agent: add mode not to keep the PIN in memory #24

Open FiloSottile opened 4 years ago

FiloSottile commented 4 years ago

PIV tokens can be configured to only require the PIN once in a session. Assuming pivy-agent keeps the PIV session open, it doesn't need to keep the PIN in memory in that case. It would be nice to have a mode for pivy-agent not to keep the PIN.

The main difference in UX is that plugging the token out and back in would require providing the PIN again.

arekinath commented 4 years ago

The pivy-agent by default ends the transaction with the device once it's been idle for 2sec, so that it can co-exist with other applications that might want to use the device (and don't necessarily know the PIN themselves). At the end of the transaction, the smartcard is reset, which clears this "once a session" flag.

This would probably require adding a mode where we open the card in exclusive mode or maintain an indefinite transaction while the PIN is "loaded".

(FYI: the current UX requires the PIN to be provided again if the device is removed, as it is -- as soon as the pivy-agent detects device removal it erases the PIN from its memory)

rdslw commented 3 years ago

I support adding this mode. It would be logical, and much more secure, to keep excl. mode and remove PIN from memory.

Also I did a few test, with token being removed, and pivy-agent does NOT always detect it, and keeps indefintely PIN in the memory. This was a hit/miss. Over 5 tries, only one requstelt in PIN being required. In others (each around 1..2 minutes long) entering again PIN was not required.

arekinath commented 3 years ago

@rdslw Any chance I could get you to send through the debug level log output (obtained by running pivy-agent with the -d option) when this occurs (a >60sec disconnected window where it failed to evict the PIN)? The defaults are only guaranteed to detect a disconnection longer than 60 seconds (the trade-off here is against adding threads to the agent, which I'm loathe to do), but any disconnection longer than that should be detected -- if it's failing to do that there's a bug.

I don't have any desire to test or maintain support for opening the card in exclusive mode and re-entering the PIN every use right now, and it would require adding substantial complexity to the code to handle both modes of operation (there's a lot of logic which assumes the current transactional workflow associated with shared mode). I think it's best if you need that to look elsewhere -- that way you're getting simpler code which does only that workflow and isn't full of conditional logic (and pivy can have less conditional logic too).

rdslw commented 3 years ago

I did extensive tests, and I'm NOT able to reproduce it with disconnects LONGER than 60 seconds. Probably was ommision on my side. Sorry for that. Just to be sure, 30s works reliably without needing to provide PIN.

One question though in regard of not keeping PIN in the memory: what is proper way to erase it completly (overwrite etc) from memory on request WITHOUT having to remove token for longer >60secs. Is 'ssh-add -D' sufficient? Does it overwrite PIN in memory, or uses other measures?

arekinath commented 3 years ago

@rdslw Yes, ssh-add -D or ssh-add -x (with any password as input, it ignores it) will both drop the pin from memory. It does an explicit_bzero of the special page where the PIN is kept (we put the PIN in a page of its own between two PROT_NONE guard pages and mark the whole lot with mlock and MADV_DONTDUMP, so it hopefully doesn't come out in core dumps and is pretty hard to read out with a heartbleed-style bulk memory disclosure bug thanks to the guard pages)