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

Stop leaks; enable saved password #16

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Stop leaks; enable saved password #16

wants to merge 4 commits into from

Conversation

crosser
Copy link
Contributor

@crosser crosser commented Apr 20, 2013

Commit a04d7cc:
Fix several memory leaks and mishandling of the privilege status
where a function returned failure indication, and previously
allocated memory was not freed (and the referece was lost), or
previously droped privileges where not restored.

The rest of the commits:
Option to create and keep encrypted password (re-encrypting every time it is used) and supply it to subsequent PAM modules as PAM_AUTHTOK. This way, it is possible to use yubikey as a single authentication factor (i.e. without entering the password), and still have Gnome/KDE keyring decrypted on login. Addresses http://code.google.com/p/yubico-pam/issues/detail?id=52

Fix several memory leaks and mishandling of the privilege status
where a function returned failure indication, and previously
allocated memory was not freed (and the referece was lost), or
previously droped privileges where not restored.
Change the function that creates the path for challenge-response
storate to take a "suffix" argument, so we can have multiple files.
Isolate the part of the challenge-response forming function that
fills in the expected response into a callback, so we now can store
other things in the "expected response" field.
Implement a second 'action' of ykpamcfg to create a file containing
challenge and a secret string encrypted with the response (by OTP).
Future update in the PAM module will inject this string as
PAM_AUTHTOKEN, so it could be used by subsequent PAM modules.
Implement function in the PAM module, when given the argument
"supply_authtoken", to extract and decode data stored by
ykpamcfg action "add_saved_password" and supply it as
PAM_AUTHTOK to subsequent PAM modules. Particularly to
pam_gnome_keyring module (and equivalents) to decode the
keyring if yubikey was used as the sole authentication factor.
@jas4711
Copy link
Contributor

jas4711 commented Apr 24, 2013

Thanks for the patches! Will review in more detail soon...

I would prefer to use some standard encryption mechanism rather than an ad-hoc one-time-pad approach. Maybe look at some library like GPGME and encrypt data using the OpenPGP format with a shared secret? History is full of problems with custom crypto designs and I don't want a pandoras box in this project...

Do you have some pointers on how GNOME/KDE keyring unlocking works? I'm wondering if it is possible to do in some other way.

Let's discuss how to solve this problem, I understand and completely agree with your goals of getting keyring unlocking to work, so it is only a matter of how to implement it.

/Simon

@jas4711
Copy link
Contributor

jas4711 commented Apr 24, 2013

Btw, do you know of any other PAM modules that does GNOME/KDE keyring unlock? For inspiration, comparison and possible to end up with a best recommended approach that can be used by multiple PAM modules, and thus ultimately better reviewed.

@crosser
Copy link
Contributor Author

crosser commented Apr 24, 2013

Simon, thanks for taking a look at the matter!

Re. the choice of the encryption algo: I've been pondering three approaches: (1) use of some "industry standard" library like openssl/gnutls, (2) use yubico's implementation of aes128 which is a part of some library (c-clinet? don't remember), and (3) simple XOR "one-time-pad". I do not take gpgme seriously because, to my knowledge, it executes command-line gpg under the hood, which makes it hairy in terms of external dependencies, and prone to hard-to-debug problems as well as potential insecurity if for instance the gpg binary changes/moves.

Openssl/gnutls might be the "best" choice from the "robust security" standpoint, however it adds dependency, and a quite "political" one at that, as people tend to fight over which one of the two to use. However my main argument against it was that it's a rather heavy dependency library.

Yubico's implementation of aes128 could be the most "lightweight" solution in that it does not introduce external dependencies. But in some sense, it is as "home-grown" as my XOR approach. And after consulting with a friend cryptographer, I was pretty convinced that XOR is as good as any complicated algorithm as long as (a) the data is not longer than the key, and (b) the key is "sufficiently random". In fact, I would argue that the XOR approach may even be "more secure" because it is so simple, while even "industry standard" implementation of a complicated algorithm is potentially prone to programming errors.

That all said, I believe that you guys must have some good experts in cryptography on board at Yubico, and it would be great if some of them could take a look at the code (and take into account my reasoning about dependencies) and give their verdict. Do you think that it could be arranged?

Re. the way to unlock the keyring. I only have experience with Gnome keyring; I think that KDE must work on the same principle. Gnome keyring package includes a PAM module whose main function is to fetch PAM_AUTHTOK and use it with some keyring-implementation-specific function to decrypt the keyring store. When I first started to think about it, I thought that I'd need to supply a replacement for that functionality. Doing that would have an additional advantage: we would be able to (optionally) make the unlocking two-factor, i.e. to use a combination of the response and the user-supplied password as the unlock key for the keyring store.

However, in the end I judged against it, because such solution would be dependant on the particular implementation of the keyring. When I realised that I can simply inject the value of PAM_AUTHTOK and then any implementation of the keyring unlocker will pick it up and just work, it was a "eureka!" moment.

I think that designing some crypto-token oriented model for unlocking the keyring (that would also allow two-factor) would be a good thing to do, but I suspect that this would be a long an involved process, for which many parties need to be brought on board. I don't feel that I am up to such endeavour. I admit that my approach with PAM_AUTHTOK injection is dirty, but it works, and is probably sufficiently secure for many uses.

I do not know of anybody doing the keyring unlocking except the authors of the respective keyrings. Which of course does not mean that such works do not exist.

Regards,

Eugene

@klali
Copy link
Member

klali commented Sep 18, 2013

Hello,

Just an update here, we've not forgotten entirely about this..

I just merged the commit for memory leaks and privilege errors, though with reverting to malloc and freeing that memory.
For the other things we feel a need to think a bit more on it.

/klas

@crosser
Copy link
Contributor Author

crosser commented Sep 19, 2013

Thanks for that, @klali.

Somewhat off-topic, I think that we have another, much bigger, issue ahead, if I get around to make PC/SC backend for libykcore. I hope that I will, some time soon(ish). The issue is, NFC is prone to passive eavesdropping, so it is unsafe to get the expected next response from the device ahead of time, which the current module does. The attacker can eavesdrop on the response, replay it and get logged in. The solution would be to keep the (encrypted) shared key on the workstation, as described in the whitepaper on your site.

@itay-grudev
Copy link

Ping! Looks like a nice addition to the library so one can unlock their keyring as well. Could you guys look into it and assist the OP in resolving conflicts?

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

Successfully merging this pull request may close these issues.

None yet

4 participants