KEYP Vulnerability

[iceman50]

With the current vulnerability with DC++'s current KEYP implementation the underlying issue seems to be this ...

[2012-04-26 09:24] <Crise> anyways, the thing with keyp is entirely different problem... which is basically that it only verifies keyp on the peer level certificate and not on the whole chain as it should

Crise has stated he has another source who knows the exploit but will not divulge in who he is.

[poy (poy)]

DC++ follows the spec in this regard; this should be discussed with ADC maintainers to decide whether taking the cert chain into account is indeed necessary.
rev 34 of ADC might be relevant: <http://adc.svn.sourceforge.net/viewvc/adc/trunk/ADC-EXT.txt?r1=34&r2=33&pathrev=34>

is this really a security issue? if 2 peers disagree on their KEYP, the worse that could happen is a failure to establish the connection. i fail to see how a third party could exploit this divergence to "sneak a cert into the chain" while keeping the KEYP intact.

[Jacek Sieka (arnetheduck)]

would someone care to elaborate?

[Crise / MW (markuwil)]

Poy: where is the discussion concerning rev 34, it drastically changes the definition of KEYP from the original, to what end?

Simplicity is not a valid reason to change a specification, over security.

[Fredrik Ullner (ullner)]

The following discussion in the DCDev hub was most likely the cause of the change(s?):

- [2010-09-01 19:12:10] <Pretorian> KEYP state " followed by a base32-encoded cryptographic hash of either the certificate directly (which is appropriate in the case of a self-signed certificate), or a certificate providing the base of a valid signature chain (which may be more appropriate a CA-signed certificate). "
- [2010-09-01 19:12:25] <Pretorian> Do we or don't we need to specify which 'or' is applied?
- [2010-09-01 19:14:46] <Pretorian> I'd change the URI to be ":1234/kp=SHA256#foobar"
- [2010-09-01 19:15:16] <Pretorian> I dislike to use / as delimiter in this case.
- [2010-09-01 19:15:44] <poy> i thought it would always be the first case (hash of the cert itself), since the purpose is to make sure that the cert received is the one we were expecting. how it is signed and by whom can then be checked once the cert has been received.
- [2010-09-01 19:30:29] <Quicksilver> a signed cert has no meaning for us ... important is only that the cert shown by the hub is allways the same ... and that its the hub you received the address for ... a ca is only there for binding a virtual entity to a real one.. which we are not interested in ... as we are not companies trying to sell stuff
- [2010-09-01 19:31:46] <iceman50> i.e paypal needs ca
- [2010-09-01 19:35:58] <Quicksilver> also there were already attacks were CAs were compelled to validate fake certs by a government as valid to allow spying on TLS connections ....
- [2010-09-01 19:38:11] <Quicksilver> e.g. an attacker interrupts the connection and shows you a real looking cert signed by CA from bananarepublic A ... attacker pays A money to compell CA to valiudate their cert ... voila attack succeeded.. and your browser will probably not complain as the CA is valid... its just a bit implausible that paypal gets signed for example by some carribean CA ... but your browser won't mind to much...
- [2010-09-01 19:38:41] <iceman50> weird
- [2010-09-01 19:39:01] <poy> same opinion as me then, so in Pretorian's quote, always choose the first "or" case, right?
- [2010-09-01 19:39:01] <Pretorian> iceman50: Hm?
- [2010-09-01 19:39:19] <poy> Quicksilver: i thought you proposed KEYP?
- [2010-09-01 19:39:51] <iceman50> well unless you specifically check the cert yourself everytime you go through an encrypted channrel
- [2010-09-01 19:39:53] <Quicksilver> no not my preposition... and KEYP has nothing to do with CA ... for us KEYP is way better..
- [2010-09-01 19:41:20] <Quicksilver> the trick in all cases is use the CA for the first time to verify a cert.. in all later cases compare the cert to the cert that has been shown to you before i.e. do what KEYP does ... sadly thats not the common case for browsers... thats what you do when you use putty/ ssh client ... comparing current cert to last cert
- [2010-09-01 19:42:21] <poy> KEYP's only use is when connecting via a hub list / web site, correct? after that clients can handle that cert comparison themselves.
- [2010-09-01 19:42:49] <Quicksilver> yes and no ...
- [2010-09-01 19:43:12] <Quicksilver> keyp is a way to store the cert.. just keeping it behind the hub'...

[Fredrik Ullner (ullner)]

(Seems Launchpad doesn't handle formatting in the best of ways...)

Anyway, if the changes in the spec should be reverted, then I can do that without a problem.

Would reverting break all existing implementations? Or would they simply be 'not as secure'?

[Crise / MW (markuwil)]

If I am totally honest, the first thing I thought when I saw the changes in the diff was that it was done to make dcpp's simplistic implementation match the spec (or rather the other way around).

Changing the spec back to the way it was doesn't break any implementations as far as I know, all the more verbose version does is state explicitly what is to be done when the certificate is in a chain (as well as some subtle wording changes and a grammar fix or two).

Although notably DC++'s implementation fails to accept several valid KEYP scenarios (based on original text) and some MITM attacks utilizing chains (as I understand it, I am by no means a security expert, unlike the guy I am proxying here). Realistically speaking DC++'s implementation would be fine in something like 9 cases out of 10. Considering that typically hubs do use self signed certificates over more complex setups.

[Crise / MW (markuwil)]

Let's bring this back out of limbo.

Attached is a patch to CryptoManager to allow proper verification of KeyPrints additionally a bunch of changes have been made to the TLS related options handling, because the code was bad (as in it resulted us not having peer certificates at all with default setup, essentially making c<->c KEYP verifiction a no-op process)

[Crise / MW (markuwil)]

Attached patch fixes call order of openssl functions during setup.

Said patch is incremental.

[poy (poy)]

this is too evolved for me to fully comprehend so i am tempted to just trust the patch authors... however, some test cases (either direct ones or instructions on how to set them up) would be useful to 1) ensure the patch prevents impersonations the previous implementation would have allowed, 2) doesn't introduce regressions and 3) cross-test with other clients (notably Jucy).

nothing wrong jumps out in terms of code; some comments on the following would be welcome: a) new locking mechanism; b) DHs of different sizes; c) CryptoManager clean-up (really necessary since the process is exiting? there was none before - does this fix other issues?); d) changes to cert generation; e) the TODO / commented-out code.

[Crise / MW (markuwil)]

1-2) The verify_callback for the most part has been "empirically tested" over the past year in ApexDC, and before inclusion there it was tested against a set of such test cases (heck it was mostly created based on those test cases). The only notable change related to verify_callback that this does not apply is the logging stuff that I added fairly recently. Same applies to 3), howerver, the default cipher changes discussed on the hub is a different story when it comes to interop with Java 6 in particular but that should be a separate patch on its own.

Generally when it comes to interoperability the bottom line is, anything newer than openssl 1.x.x based or equivalent should be fine, the old 0.9 versions of openssl are a mystery at this point (although, openssl default build is rather conservative, so there should be no problems unless openssl has explicitly stated otherwise). And this particular patch isn't touching openssl itself anyways (which needs to be upgraded to 1.0.1f by the way).

a) This is mostly because of the use of the SSL_ex_app_data functions, and more of a precaution than anything else. Other clients have used this kind of setup far longer, and all testing was done on a setup with it. (see: https://bugs.launchpad.net/dcplusplus/+bug/378829)
b) This is a fairly common practice, look at f.ex. Apache, two years ago when I intially started the work on cryptomanager in ApexDC Firefox would fail without variable DH sizes (ApexDC does do some web serving as well, to give context).
c) You need to free memory even if the process is exiting, this is openssl, which is C and designed to work as external module, so it does lot of memory copying of objects passed to it
d) Changes to cert generation have been guided by the common practices (ie. don't use sha1 anymore, it has been on its way out for years from use with certificates now). Also without those changes openssl defaults to a rather conservative (read: bad) ciphers for reasons that are arcane by nature (ie. it shouldn't according to keyp author but quite obviously it does).
e) The commented out code is pretty cut and dry, isTrusted() returns based on cert verification status, and if said verification fails the connection never gets this far when ALLOW_UNTRUSTED_CLIENTS is false (this is doubly true for pre-patch code, due to use of SSL_VERIFY_NONE being used, in which case the server (the one who requested CTM) has no peer cert to verify to begin with. ConnectionManager::checkKeyprint() deals with the ALLOW_UNTRUSTED_CLIENTS option for servers anyways, and clients do it during handshake (see UserConnection::connect())

For the TODO's those only reflect the fact that in practice, hub without pinned KeyPrint is not trusted, so should the KeyPrints of its clients be blindly trusted (there is no chain of trust from hub to its peers and, possibly even not from whoerver distributes the hubs KeyPrint to users). Ideally, a hub has the capability to check the KP fields of connecting clients and not let them in if there is a mismatch.

[Crise / MW (markuwil)]

I will see if it is possible for me to get access to the test cases I used previously, but I know for a fact that they are no longer up (and probably he wouldn't want them public at least on that server).

[poy (poy)]

thanks for clarifying the side points. on #e, i seem to remember cryptic error messages because the connection never went far enough to reach that message. is it possible to transfer it to a better place, along with queue source removal? and i agree with the TODO; i think user objects have a flag that can be used to check (without needing to find the right hub etc).

the big chunk that remains is keyp validation, for which tests would be very useful.

let's wait until the end of the week if anyone has objections then this can go in.

[Crise / MW (markuwil)]

Re: moving the source removal.

It is possible to move it, just something I overlooked, because only downloaders (ie. servers) need to do it. The patch delays the trust option check for accepted connections which means it is technically possible to so the removal in the same case as checkKeyprint().

[Crise / MW (markuwil)]

Since there has been no further discussion, I intend to push this later today (Thursday), obviously including the fix for queue source removal (and probably few extra code comments for future reference.

[Quicksilver (christianortolf)]

I would really like it if we would go with KISS in ADCS and KEYP.
As in require hubs and clients to solely use self signed certs.

But others might have had some nice application in mind with certificate chains... still i am thinking that doing security any more complicated than it needs to be will bite us in the end.
Or am I way behind reality and that is already the case in current spec?

[Crise / MW (markuwil)]

The current spec of KeyPrint (in the repo, not the original draft) is not just keeping it simple, it is punching holes into it and it does not even explicitly forbid the use of chains anyways.

You can't impose such restriction as "only use self-signed certificates" when the text originally did not have such restriction, because it will leave all existing implementations vulnerable. In practice if you do impose such restriction at later date it has no impact in real world scenarios since implementations would still have to code defensively because of old clients and in order to retain the same level of security across the board.

The only restriction you can realistically impose at this point is that clients should use self signed certificates (because that is the status quo), for hubs this ship has long since sailed.

If you want a real world use case, then here is one. Every time hubs peer certificate expires the KeyPrint changes, if they can pin a root or an intermediate certificate with a longer lifetime instead then that problem is no longer as relevant without loosing the benefit of regenerating peer certificates as they are supposed to.

Right now clients out there do not care about expiration of certificates at all (because they can't, with the generous validity period of 10 days dcpp had), but who is to say this will not change. I would even go as far as to say that it should particularly for hub connections at least (which my patch incidentally also addressed, provided correct options are set).

You are essentially saying: "Let's promote bad TLS setup practices so we can keep our code simple" and this shouldn't sit well with anybody (especially for servers, who stick around for a long time).

[poy (poy)]

DC++ 0.840 has been released with fixes for this bug. please review the privacy of information exposed here so we can hide it before this bug is made available to public view (as it is referred to by changelog links).

[poy (poy)]

Fixed in DC++ 0.840.