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

Improve KeePassHTTP security #147

Closed
sfunk1x opened this Issue Jan 9, 2017 · 43 comments

Comments

Projects
None yet
7 participants
@sfunk1x

sfunk1x commented Jan 9, 2017

After looking through the issues list for KeePassHTTP, I noticed issue #258 and it's criticism of the encryption of the connection between the HTTP clients and the HTTP server. I'm not seeing where this has been addressed in KeePassXC, so I'd like to bring this up here.

The concept of generating a set of self-signed certificates to ensure a TLS connection on either the exposed LAN connection (remote) or on localhost seems like it would go a long way to securing against potential password leak without using a roll-your-own-crypto approach, which seems to be what's going on currently.

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 9, 2017

Member

Note that on KeePassXC by default KeePassHTTP is disabled at compile-time, is disable at runtime, and listen only on localhost.

The security is not a roll-your-own-crypto based, simply KeePassHTTP is an API to access the kdbx, that is encrypted with AES-CBC. Asks for the master passwords, decrypt the database, sends the password to client.

Now, for making a CBC Padding oracle attack you need to have a padding oracle in a decryption method. I don't know the KeePassHTTP protocol well but the only decryption is done when asking for the master password and unlocking the db (I think).
Also I don't know KeePassHTTP but the padding oracle can be done only if the server report a padding error.

The flaw in KeePassHTTP is not described in detail and is only "theoretical" so there is no need to panic IMHO.
Should be nice a talk with @Quiark

Member

TheZ3ro commented Jan 9, 2017

Note that on KeePassXC by default KeePassHTTP is disabled at compile-time, is disable at runtime, and listen only on localhost.

The security is not a roll-your-own-crypto based, simply KeePassHTTP is an API to access the kdbx, that is encrypted with AES-CBC. Asks for the master passwords, decrypt the database, sends the password to client.

Now, for making a CBC Padding oracle attack you need to have a padding oracle in a decryption method. I don't know the KeePassHTTP protocol well but the only decryption is done when asking for the master password and unlocking the db (I think).
Also I don't know KeePassHTTP but the padding oracle can be done only if the server report a padding error.

The flaw in KeePassHTTP is not described in detail and is only "theoretical" so there is no need to panic IMHO.
Should be nice a talk with @Quiark

@Quiark

This comment has been minimized.

Show comment
Hide comment
@Quiark

Quiark Jan 11, 2017

Unfortunately I have a working exploit code for KeePassHTTP (that I'm not going to share until this hole is resolved). The KeepassHTTP protocol is not very good and there are fields that are being decrypted and where padding attack can be applied. And even if padding error is not reported, a timing attack could still be probably used.

TLS may be an option (though I realize how complicated it can be, especially in C++). Maybe TweetNaCl could be used. But I ended up not using browser integration at all because I see it as a too large security risk (issues found by taviso in other password managers could potentially apply here as well).

Quiark commented Jan 11, 2017

Unfortunately I have a working exploit code for KeePassHTTP (that I'm not going to share until this hole is resolved). The KeepassHTTP protocol is not very good and there are fields that are being decrypted and where padding attack can be applied. And even if padding error is not reported, a timing attack could still be probably used.

TLS may be an option (though I realize how complicated it can be, especially in C++). Maybe TweetNaCl could be used. But I ended up not using browser integration at all because I see it as a too large security risk (issues found by taviso in other password managers could potentially apply here as well).

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 11, 2017

Member

If you share to me the exploit code privately (you can get my email in m profile->website or from a GPG key in a verified commit) I can better understand where the flaw is, detemine if it's in the protocol or in our implementation, and decide if needs TLS or just a patch in our implementation.

Also note that our KeePassHTTP implementation is different from https://github.com/pfn/keepasshttp

I will dive more into our implementation in the next few days to see if a Padding Oracle apply somewhere.

For an eventual TLS implementation we can maybe rely on https://doc.qt.io/qt-5/ssl.html and https://github.com/GuiTeK/Qt-SslServer

Member

TheZ3ro commented Jan 11, 2017

If you share to me the exploit code privately (you can get my email in m profile->website or from a GPG key in a verified commit) I can better understand where the flaw is, detemine if it's in the protocol or in our implementation, and decide if needs TLS or just a patch in our implementation.

Also note that our KeePassHTTP implementation is different from https://github.com/pfn/keepasshttp

I will dive more into our implementation in the next few days to see if a Padding Oracle apply somewhere.

For an eventual TLS implementation we can maybe rely on https://doc.qt.io/qt-5/ssl.html and https://github.com/GuiTeK/Qt-SslServer

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 13, 2017

Member

Summary: the key exchange in KeePassHTTP protocol between the browser and the server (KeePass/X/XC) happens in pure plain-text.
This is a bad security practice, if an attacker can succeed in this, he can use the KeePassHTTP protocol to request almost all of your passwords. (limited to the one with an URL set or an URL in the title)

All the users that are using KeePass/X/XC are recommended to stop using KeePassHTTP plugin.
At least ensure that the server (KeePass/X/XC) is ONLY listening on your PC only (host: 127.0.0.1).

Listening on 127.0.0.1 limits attacks coming from outside your network, but malicious application on your computer can still intercept the protocol's key exchange.

Thanks also to @Quiark

Member

TheZ3ro commented Jan 13, 2017

Summary: the key exchange in KeePassHTTP protocol between the browser and the server (KeePass/X/XC) happens in pure plain-text.
This is a bad security practice, if an attacker can succeed in this, he can use the KeePassHTTP protocol to request almost all of your passwords. (limited to the one with an URL set or an URL in the title)

All the users that are using KeePass/X/XC are recommended to stop using KeePassHTTP plugin.
At least ensure that the server (KeePass/X/XC) is ONLY listening on your PC only (host: 127.0.0.1).

Listening on 127.0.0.1 limits attacks coming from outside your network, but malicious application on your computer can still intercept the protocol's key exchange.

Thanks also to @Quiark

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 13, 2017

If you have a malicious application on your system capable of sniffing localhost traffic, then you have already lost the security war. Game over, full stop.

Using tls and installing a cert in the browser is not practical unless shipping you're own tls implementation.

pfn commented Jan 13, 2017

If you have a malicious application on your system capable of sniffing localhost traffic, then you have already lost the security war. Game over, full stop.

Using tls and installing a cert in the browser is not practical unless shipping you're own tls implementation.

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 13, 2017

Your... Stupid autocorrect.

Anyway, if you're using nacl, sounds like using your own tls implementation is the plan

pfn commented Jan 13, 2017

Your... Stupid autocorrect.

Anyway, if you're using nacl, sounds like using your own tls implementation is the plan

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 13, 2017

Member

We can't just insert TLS if chromeIPass and PassIFox are not designed for it.
In KeePassXC, the KeePassHTTP protocol is listening on 127.0.0.1 and disabled by default but lot of implementation are listening over 0.0.0.0 and accepting requests across the web

Member

TheZ3ro commented Jan 13, 2017

We can't just insert TLS if chromeIPass and PassIFox are not designed for it.
In KeePassXC, the KeePassHTTP protocol is listening on 127.0.0.1 and disabled by default but lot of implementation are listening over 0.0.0.0 and accepting requests across the web

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 13, 2017

Member

Using plain HTTP is not generally a problem. Security can be implemented on any layer. That's how OTR chats work, that's how email encryption works. Not using HTTPS can sometimes even be a good idea, especially when you want or need to avoid the hassle that evolves around the TLS PKI.
The problem here is rather this is a poorly designed (and also completely undocumented) proprietary crypto protocol. It would be much better to use a proven standard protocol.

Member

phoerious commented Jan 13, 2017

Using plain HTTP is not generally a problem. Security can be implemented on any layer. That's how OTR chats work, that's how email encryption works. Not using HTTPS can sometimes even be a good idea, especially when you want or need to avoid the hassle that evolves around the TLS PKI.
The problem here is rather this is a poorly designed (and also completely undocumented) proprietary crypto protocol. It would be much better to use a proven standard protocol.

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 23, 2017

Member

Due to the popularity of this feature, I'm thinking about rescheduling this issue to 2.1.1 and declaring it as a bugfix therefore. Any thoughts on that @droidmonkey @TheZ3ro ?

Member

phoerious commented Jan 23, 2017

Due to the popularity of this feature, I'm thinking about rescheduling this issue to 2.1.1 and declaring it as a bugfix therefore. Any thoughts on that @droidmonkey @TheZ3ro ?

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 23, 2017

Member

I've already said that in #177 (comment)
A bugfix is obviously needed.
We should also continue to release 2 separate release with and without KeePassHTTP support.

Member

TheZ3ro commented Jan 23, 2017

I've already said that in #177 (comment)
A bugfix is obviously needed.
We should also continue to release 2 separate release with and without KeePassHTTP support.

@phoerious phoerious modified the milestones: v2.1.1, v2.2.0 Jan 23, 2017

phoerious added a commit that referenced this issue Jan 23, 2017

Only listen to local loopback instead of user-configured host as disc…
…ussed in issue #147

Also issue warning when trying to bind to a port below 1024 and use default port in that case
@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 23, 2017

Member

I started something in the hotfix/147-keepasshttp branch. I removed the hostname setting and only bind to localhost now. I also added a warning when the user tries to configure a port below 1024.

Member

phoerious commented Jan 23, 2017

I started something in the hotfix/147-keepasshttp branch. I removed the hostname setting and only bind to localhost now. I also added a warning when the user tries to configure a port below 1024.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

Hi all,

I don't understand what is all this noise about...

  1. Where is exactly the security "hole" everybody talking about?
  2. Is referenced by Quiark exploit able to "suck" the passwords without me interacting with the KeePassX UI?
  3. Are we responsible for the cases where user overrides default localhost binding?

I tend to agree with https://github.com/pfn, once your localhost is compromised you are already screwed and if you're messing with default network binding you are on your own...

eugenesan commented Jan 24, 2017

Hi all,

I don't understand what is all this noise about...

  1. Where is exactly the security "hole" everybody talking about?
  2. Is referenced by Quiark exploit able to "suck" the passwords without me interacting with the KeePassX UI?
  3. Are we responsible for the cases where user overrides default localhost binding?

I tend to agree with https://github.com/pfn, once your localhost is compromised you are already screwed and if you're messing with default network binding you are on your own...

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

Running this protocol over a remote network connection is inherently insecure. KeePassX(C) does not only target expert users with in-depth cryptography and security knowledge and we therefore need to take good precautions that people don't expose their passwords. Also even if the user configured the application correctly, but for some reason binding to the specified hostname and port fails, KeePassXC silently falls back to listening to 0.0.0.0 which is a no-go.
My patch removes all that and enforces running on localhost or not at all. That is the only configuration in which this protocol has no known exploits.

Member

phoerious commented Jan 24, 2017

Running this protocol over a remote network connection is inherently insecure. KeePassX(C) does not only target expert users with in-depth cryptography and security knowledge and we therefore need to take good precautions that people don't expose their passwords. Also even if the user configured the application correctly, but for some reason binding to the specified hostname and port fails, KeePassXC silently falls back to listening to 0.0.0.0 which is a no-go.
My patch removes all that and enforces running on localhost or not at all. That is the only configuration in which this protocol has no known exploits.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@phoerious nobody argues with the fact that configuring non-local binding is a security issue.
The problem is the way the issue is presented and treated by developers.

This feature by it's dangerous nature is targeting toward more advanced/cautious users.

Regarding your modifications, I partially agree with your reasoning. Fallback to 0.0.0.0 is silent only to user using the GUI. I agree I should have added the warning to GUI also.
Note, your modification breaks systems with IPv6.

eugenesan commented Jan 24, 2017

@phoerious nobody argues with the fact that configuring non-local binding is a security issue.
The problem is the way the issue is presented and treated by developers.

This feature by it's dangerous nature is targeting toward more advanced/cautious users.

Regarding your modifications, I partially agree with your reasoning. Fallback to 0.0.0.0 is silent only to user using the GUI. I agree I should have added the warning to GUI also.
Note, your modification breaks systems with IPv6.

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

No, it does not break IPv6 systems. That would require disabling IPv4 completely (which we can assume is not the case and even then I'm not sure if lo wouldn't still accept requests over IPv4).

Member

phoerious commented Jan 24, 2017

No, it does not break IPv6 systems. That would require disabling IPv4 completely (which we can assume is not the case and even then I'm not sure if lo wouldn't still accept requests over IPv4).

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 24, 2017

Member

@eugenesan

  1. If there is an error binding 127.0.0.1, Default fallback to 0.0.0.0 -> key is exchanged in plain-text
  2. Once you have the key you can follow the protocol and request every password you want (with website set in title)
  3. If the default fallback is 0.0.0.0 then yes.

I tend to agree with https://github.com/pfn, once your localhost is compromised you are already screwed and if you're messing with default network binding you are on your own...

What the purpose of having a password-protected offline KDBX file if when my PC is compromised everyone can read my passwords? Just store them in plain-text and don't even bother with passwords managers.

Member

TheZ3ro commented Jan 24, 2017

@eugenesan

  1. If there is an error binding 127.0.0.1, Default fallback to 0.0.0.0 -> key is exchanged in plain-text
  2. Once you have the key you can follow the protocol and request every password you want (with website set in title)
  3. If the default fallback is 0.0.0.0 then yes.

I tend to agree with https://github.com/pfn, once your localhost is compromised you are already screwed and if you're messing with default network binding you are on your own...

What the purpose of having a password-protected offline KDBX file if when my PC is compromised everyone can read my passwords? Just store them in plain-text and don't even bother with passwords managers.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@TheZ3ro

  1. I thought HTTP used asymetric crypto and TLS handshake therefor plaintext key is not a problem.
  2. I still not sure there is no enforcement of IF and which password is pulled for each request, unless you did put the checkbox on "Remember my choice" which IS the rea security flaw.
  3. The default is "localhost" and "0.0.0.0" is the fallback with warning (currently in console only which should be fixed).

eugenesan commented Jan 24, 2017

@TheZ3ro

  1. I thought HTTP used asymetric crypto and TLS handshake therefor plaintext key is not a problem.
  2. I still not sure there is no enforcement of IF and which password is pulled for each request, unless you did put the checkbox on "Remember my choice" which IS the rea security flaw.
  3. The default is "localhost" and "0.0.0.0" is the fallback with warning (currently in console only which should be fixed).
@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

Just to rule out any misunderstandings: the KeePassHTTP protocol works over plain (!) HTTP, not HTTPS. It is therefore not TLS encrypted. The encryption is handled by the sending and receiving parties without using any official crypto protocol. The catch with this is that the messages are unauthenticated and the handshake is done without encryption in plaintext. Therefore intercepting that handshake enables you to decrypt all follow-up traffic. Furthermore, a padding oracle attack is possible on the AES-CBC cipher texts due to the lack of authentication.

Member

phoerious commented Jan 24, 2017

Just to rule out any misunderstandings: the KeePassHTTP protocol works over plain (!) HTTP, not HTTPS. It is therefore not TLS encrypted. The encryption is handled by the sending and receiving parties without using any official crypto protocol. The catch with this is that the messages are unauthenticated and the handshake is done without encryption in plaintext. Therefore intercepting that handshake enables you to decrypt all follow-up traffic. Furthermore, a padding oracle attack is possible on the AES-CBC cipher texts due to the lack of authentication.

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 24, 2017

Member

@eugenesan

  1. HTTP does not use any crypto or TLS. The key is sended in the associate request base64 encoded (here the reference https://github.com/pfn/keepasshttp)
  2. Depends on users preference. Attacker can still sniff get-logins request-response and decrypt passwords with the key that he sniffed previously
  3. If localhost:19455 is already binded what will happened?
Member

TheZ3ro commented Jan 24, 2017

@eugenesan

  1. HTTP does not use any crypto or TLS. The key is sended in the associate request base64 encoded (here the reference https://github.com/pfn/keepasshttp)
  2. Depends on users preference. Attacker can still sniff get-logins request-response and decrypt passwords with the key that he sniffed previously
  3. If localhost:19455 is already binded what will happened?
@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@TheZ3ro
1,2. If that's the case, there is a reason to be worried ( A LOT :-o )
3. It warns the user (only in console) and falls back to the default of the Qt network stack which is 0.0.0.0.

I always assumed original protocol developer did the crypto correctly...

eugenesan commented Jan 24, 2017

@TheZ3ro
1,2. If that's the case, there is a reason to be worried ( A LOT :-o )
3. It warns the user (only in console) and falls back to the default of the Qt network stack which is 0.0.0.0.

I always assumed original protocol developer did the crypto correctly...

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

That's what we are talking about. This protocol is poorly designed and must therefore not be used over any remote network connection.

And BTW it does not fall back to any Qt defaults. We don't even use Qt for running the network daemon at the moment. That is handled by libmicrohttpd and the fallback to 0.0.0.0 was hardcoded.

Member

phoerious commented Jan 24, 2017

That's what we are talking about. This protocol is poorly designed and must therefore not be used over any remote network connection.

And BTW it does not fall back to any Qt defaults. We don't even use Qt for running the network daemon at the moment. That is handled by libmicrohttpd and the fallback to 0.0.0.0 was hardcoded.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@phoerious

  1. I am not sure the code currently in this repo inclueds all the intended fixes it originally had. Please see recent commits at https://github.com/eugenesan/keepassx/commits/2.0-http-totp-2.0.3
  2. libmicrohttpd handles the binding while Qt network stack handles the IP:PORT availability and DNS. And once more, 0.0.0.0 is only a fallback, just remove it or request user interaction to solve that.

eugenesan commented Jan 24, 2017

@phoerious

  1. I am not sure the code currently in this repo inclueds all the intended fixes it originally had. Please see recent commits at https://github.com/eugenesan/keepassx/commits/2.0-http-totp-2.0.3
  2. libmicrohttpd handles the binding while Qt network stack handles the IP:PORT availability and DNS. And once more, 0.0.0.0 is only a fallback, just remove it or request user interaction to solve that.
@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

As an immediate resolution I suggest disabling 0.0.0.0 fallback.
But a more serious issue is a lack of proper TLS-style crypto, that should be discussed with browser side maintainer. KeePassX project can't handle it by it's own.

eugenesan commented Jan 24, 2017

As an immediate resolution I suggest disabling 0.0.0.0 fallback.
But a more serious issue is a lack of proper TLS-style crypto, that should be discussed with browser side maintainer. KeePassX project can't handle it by it's own.

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

pfn made it pretty clear, that TLS will not be an option, so we have to work with what we have. Therefore restricting to localhost is our only option.

Member

phoerious commented Jan 24, 2017

pfn made it pretty clear, that TLS will not be an option, so we have to work with what we have. Therefore restricting to localhost is our only option.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@phoerious
Well, I am not sure how to comment on pfn's opinion regarding TLS but I agree regarding localhost.

Note, I still think localhost is a DNS record "localhost" and not IP address 127.0.0.1 and should not be treated as such.

eugenesan commented Jan 24, 2017

@phoerious
Well, I am not sure how to comment on pfn's opinion regarding TLS but I agree regarding localhost.

Note, I still think localhost is a DNS record "localhost" and not IP address 127.0.0.1 and should not be treated as such.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@pfn
Earlier, I've agreed with you regarding concept of loosing to "local sniffing agent" but now, after some thought gymnastics, I think I've missed important part of the problem.
I still agree regarding regarding real "local sniffing agent" but since half of the variability is in the browser it is essentially a "remote sniffing agent" and must be treated with at least secure handshake.

eugenesan commented Jan 24, 2017

@pfn
Earlier, I've agreed with you regarding concept of loosing to "local sniffing agent" but now, after some thought gymnastics, I think I've missed important part of the problem.
I still agree regarding regarding real "local sniffing agent" but since half of the variability is in the browser it is essentially a "remote sniffing agent" and must be treated with at least secure handshake.

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

That's a different factor, but a secure handshake won't help you there. The browser extension has the decryption key for retrieved passwords. Therefore, if your browser extension is compromised, an attacker can read the passwords even if a proper cryptographic protocol had been used. Letting your browser query passwords from your database opens lots of attack vectors which cannot be mitigated in any way except not using KeePassHTTP in the first place.

I added another commit to my branch which disables the plugin by default so that users have to enable it deliberately.

Member

phoerious commented Jan 24, 2017

That's a different factor, but a secure handshake won't help you there. The browser extension has the decryption key for retrieved passwords. Therefore, if your browser extension is compromised, an attacker can read the passwords even if a proper cryptographic protocol had been used. Letting your browser query passwords from your database opens lots of attack vectors which cannot be mitigated in any way except not using KeePassHTTP in the first place.

I added another commit to my branch which disables the plugin by default so that users have to enable it deliberately.

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 24, 2017

pfn commented Jan 24, 2017

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

Also the cipher text must not go over a network connection since it does not follow the Encrypt-then-MAC idiom and the CBC cipher is therefore vulnerable to timed padding oracle attacks (even if the server does not produce any particular error message).

Member

phoerious commented Jan 24, 2017

Also the cipher text must not go over a network connection since it does not follow the Encrypt-then-MAC idiom and the CBC cipher is therefore vulnerable to timed padding oracle attacks (even if the server does not produce any particular error message).

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 24, 2017

pfn commented Jan 24, 2017

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@phoerious
I am afraid we'll have to rely on Browsers enforcing security/sandboxing of the extensions.
I do worry about silent sniffing of "approved by user passwords" but I am worried more about other extensions or/and JS fetched from the web being able to decipher/fetch the key and fetch the passwords silently (by overcoming KeePassX UI requests) or/and using timing attack to fetch specific passwords.
This is where we need cooperation of @pfn.

Update: I do agree with #147 (comment)

eugenesan commented Jan 24, 2017

@phoerious
I am afraid we'll have to rely on Browsers enforcing security/sandboxing of the extensions.
I do worry about silent sniffing of "approved by user passwords" but I am worried more about other extensions or/and JS fetched from the web being able to decipher/fetch the key and fetch the passwords silently (by overcoming KeePassX UI requests) or/and using timing attack to fetch specific passwords.
This is where we need cooperation of @pfn.

Update: I do agree with #147 (comment)

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

Since this issue affect, many project I think this discussion should be taken out of this issue context.
For one we should hear the option of KeeFoxRPC author https://github.com/luckyrat.

eugenesan commented Jan 24, 2017

Since this issue affect, many project I think this discussion should be taken out of this issue context.
For one we should hear the option of KeeFoxRPC author https://github.com/luckyrat.

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 24, 2017

pfn commented Jan 24, 2017

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

What if we:

  1. Add a secure handshake
  2. Add some form of "user side" authorization like it is done in many protocols as zrtsp and signal. Meaning a random user readable text appears on both sides, in our case in the extention and the KeePass*'s password fetch approval UI?

eugenesan commented Jan 24, 2017

What if we:

  1. Add a secure handshake
  2. Add some form of "user side" authorization like it is done in many protocols as zrtsp and signal. Meaning a random user readable text appears on both sides, in our case in the extention and the KeePass*'s password fetch approval UI?
@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member

@pfn you can use a side-channel attack by measuring timings to find out whether a code decrypted correctly or not. I'm not quite sure what happens when the ciphertext is invalid in our case, but it's surely not handled properly. Encyption modes that require padding ALWAYS need message authentication and if the MAC of a message fails, no attempt must be done to decrypt the message.
Cloudflare has a pretty good read about the weaknesses of CBC: https://blog.cloudflare.com/padding-oracles-and-the-decline-of-cbc-mode-ciphersuites/

@eugenesan If a website manages to break out of the sandbox and make requests to localhost, it is probably also able to retrieve the encryption key from the browser extension.
And if we talk about secure handshakes, we also need to talk about authenticated encryption. Otherwise our secure handshake isn't work anything.

Member

phoerious commented Jan 24, 2017

@pfn you can use a side-channel attack by measuring timings to find out whether a code decrypted correctly or not. I'm not quite sure what happens when the ciphertext is invalid in our case, but it's surely not handled properly. Encyption modes that require padding ALWAYS need message authentication and if the MAC of a message fails, no attempt must be done to decrypt the message.
Cloudflare has a pretty good read about the weaknesses of CBC: https://blog.cloudflare.com/padding-oracles-and-the-decline-of-cbc-mode-ciphersuites/

@eugenesan If a website manages to break out of the sandbox and make requests to localhost, it is probably also able to retrieve the encryption key from the browser extension.
And if we talk about secure handshakes, we also need to talk about authenticated encryption. Otherwise our secure handshake isn't work anything.

@droidmonkey

This comment has been minimized.

Show comment
Hide comment
@droidmonkey

droidmonkey Jan 24, 2017

Member

You would need to modify the browser plugin to enable this security. Certainly not impossible (it is hosted on Github), but will take a lot of coordination and people to update their browser plugins.

Member

droidmonkey commented Jan 24, 2017

You would need to modify the browser plugin to enable this security. Certainly not impossible (it is hosted on Github), but will take a lot of coordination and people to update their browser plugins.

@eugenesan

This comment has been minimized.

Show comment
Hide comment
@eugenesan

eugenesan Jan 24, 2017

@phoerious
Leak of the key is a serious issue but user interaction is an addition line of defense. While the protection of the key is the job of the browser it should be possible to enhance browser and KeePass plugins to provide UI protection and it will make the job of the "rouge" agent substantially more difficult since altering UI would require modifying the code of the browser plugin which is not a trivial task.

eugenesan commented Jan 24, 2017

@phoerious
Leak of the key is a serious issue but user interaction is an addition line of defense. While the protection of the key is the job of the browser it should be possible to enhance browser and KeePass plugins to provide UI protection and it will make the job of the "rouge" agent substantially more difficult since altering UI would require modifying the code of the browser plugin which is not a trivial task.

@Quiark

This comment has been minimized.

Show comment
Hide comment
@Quiark

Quiark Jan 24, 2017

Quiark commented Jan 24, 2017

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member
Member

phoerious commented Jan 24, 2017

@TheZ3ro

This comment has been minimized.

Show comment
Hide comment
@TheZ3ro

TheZ3ro Jan 24, 2017

Member

Please guys, here we are discussing KeePassHTTP implementation in KeePassXC.
If you want to talk about KeePassHTTP protocol the issue is this -> pfn/keepasshttp#258

Member

TheZ3ro commented Jan 24, 2017

Please guys, here we are discussing KeePassHTTP implementation in KeePassXC.
If you want to talk about KeePassHTTP protocol the issue is this -> pfn/keepasshttp#258

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 24, 2017

pfn commented Jan 24, 2017

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 24, 2017

Member
Member

phoerious commented Jan 24, 2017

@pfn

This comment has been minimized.

Show comment
Hide comment
@pfn

pfn Jan 24, 2017

pfn commented Jan 24, 2017

@phoerious phoerious self-assigned this Jan 24, 2017

jsha added a commit to jsha/keepassxreboot.github.io that referenced this issue Jul 13, 2017

Remove note about KeePassHTTP
I am helping to edit a guide to using KeePassXC, and found that the author had inserted a warning: "If your machine is compromised, an attacker can intercept the communication between your browser plug-in and KeePassXC." I believe that was motivated by the warning text here. As noted in pfn/keepasshttp#258 and keepassxreboot/keepassxc#147, communicating via HTTP with localhost is safe, since an attacker who can intercept localhost communications can just read your passwords directly.

Since localhost-only is now the default mode in KeePassHTTP, I think this note just creates confusion and unnecessary fear among users.

@varjolintu varjolintu referenced this issue Jun 9, 2018

Open

Security #28

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