Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#15308 closed defect

SSL support needs some improvements

Reported by: athena Owned by:
Milestone: Component: libpurple
Version: 2.10.6 Keywords:
Cc: williamehlhardt@…, ari, kaie, DrWhax, calestyo

Description

Allow me to direct your attention to the function ssl_auth_cert() in libpurple/plugins/ssl/ssl-nss.c: http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugins/ssl/ssl-nss.c#l160

Note the rather ill-chosen preprocessor directives, making the use of SSL essentially worthless, as MITMing this crawling horror would be no more difficult than a plain, unencrypted TCP connection.

Furthermore, I will note that many common builds of libpurple, including the Ubuntu package provided by the Pidgin team at https://launchpad.net/~pidgin-developers/+archive/ppa/+files/pidgin_2.10.6-0ubuntu1%2Bpidgin1.12.10.debian.tar.gz and the build of 2.10.6 in Debian sid, enable the ssl-nss plugin, but not the ssl-gnutls one.

Not that it would help much, of course - see probe_ssl_plugins() of libpurple/plugins/ssl/ssl.c:

http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugins/ssl/ssl.c#l33

This is hard-coded to load whichever appears first in the global list of plugins returned from purple_plugins_get_all(), which is updated from purple_plugin_register() at plugin probe time:

http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugin.c#l1595

http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugin.c#l1484

http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugin.h#l232

http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugin.c#l300

The function purple_plugin_probe(), in turn, can be called from either purple_plugins_probe(), or from purple_plugins_load_saved(), which merely loads the user's selection from a UI which does not expose any choice about the SSL plugins. The ssl plugin would ultimately be determined by which is probed first from purple_plugins_probe, which calls g_dir_open() and invokes purple_plugin_probe() on the files returned by g_dir_read_name() in the order they are returned.h.

Per glib documentation, the order of entries returned by g_dir_read_name() is indeterminate and system-dependent:

http://developer.gnome.org/glib/2.29/glib-File-Utilities.html#g-dir-read-name

Per latest glib source, g_open_dir() and g_dir_read_name() call opendir() and readdir(), respectively:

http://git.gnome.org/browse/glib/tree/glib/gdir.c

Thus, the order of plugin loading and thus whether or not a user with both SSL plugins built will bother checking any certificates will ultimately be determined by such factors as the order in which the plugin binaries were installed and the choice of filesystem.

I strongly advise suggesting distributors prefer GnuTLS over NSS, then fixing NSS, then reconsidering your lack of OpenSSL support, and exposing a UI to let the user choose which SSL plugin to use when more than one is available. Passing a licensing purity test is not worth this absurd Potemkin village security.

Change History (16)

comment:1 Changed 5 years ago by foutrelis

Subscribing.

comment:2 Changed 5 years ago by deryni

Is there an actual complaint here?

We don't control what distributions choose to use by default.

Over time one or the other of the ssl plugins has had issues that the other one did not (largely due to issues in the underlying libraries but not always). People have thus had to choose which set of breakage they'd prefer. Distributions appear, largely, to have chosen NSS as their SSL library of choice for whatever reasons they have seen fit.

It is regrettable that our NSS plugin is not as complete as our gnutls plugin (assuming that is the main thrust of the this excessively verbose and meandering ticket). Patches are always welcome.

OpenSSL is a non-starter as the legal issues cannot be glibly wished away because you want them to be. Even if we were to choose to include support for it (support which does exist in at least some places), which would be legal for us to do as far as I'm aware, it wouldn't be legal for any distributions that ship binary packages to distribute those packages built with that turned on and so it would gain approximately nothing of real value for all those poor distribution package users you seem so worried about.

Most users don't even know what SSL is let alone care which of their available options for using it is actually in use and those that do care likely only build the one they want (as is the case for every distribution I've ever seen).

comment:3 in reply to: ↑ description Changed 5 years ago by datallah

Replying to athena:

Allow me to direct your attention to the function ssl_auth_cert() in libpurple/plugins/ssl/ssl-nss.c: http://hg.pidgin.im/pidgin/main/file/52cc04429e2c/libpurple/plugins/ssl/ssl-nss.c#l160

Note the rather ill-chosen preprocessor directives, making the use of SSL essentially worthless, as MITMing this crawling horror would be no more difficult than a plain, unencrypted TCP connection.

If you don't look carefully, it may appear that the NSS plugin doesn't do any validation of the SSL certificates, but that isn't the case; the validation is done, just not by the SSL_AuthCertificateHook hook.

If you look at ssl-nss.c#l454, you'll see that before the SSL connection is considered "connected" from libpurple's perspective, ssl_nss_handshake_cb is called to validate the certificate using the libpurple's purple_certificate_verify functionality.

<SNIP>

Thus, the order of plugin loading and thus whether or not a user with both SSL plugins built will bother checking any certificates will ultimately be determined by such factors as the order in which the plugin binaries were installed and the choice of filesystem.

I think this is accurate. I think the expectation is that there would be only one SSL plugin, perhaps that should be made more clear. This is presumably why builds like the ubuntu build you linked to build only one SSL plugin.

I strongly advise suggesting distributors prefer GnuTLS over NSS, then fixing NSS, then reconsidering your lack of OpenSSL support, and exposing a UI to let the user choose which SSL plugin to use when more than one is available. Passing a licensing purity test is not worth this absurd Potemkin village security.

The GnuTLS plugin performs the certificate validation in essentially the same way as the NSS plugin does.

It's simply not an option to use OpenSSL, our license (which we couldn't change if we wanted to because several contributers have stated that they are unwilling to give permission to do so) does not allow for it.

I strongly advise you to reconsider your attitude when reporting tickets; being intentionally inflammatory doesn't help any sort of interaction.

comment:4 Changed 5 years ago by datallah

  • Status changed from new to pending

comment:5 Changed 5 years ago by zooko

I opened #15309 about the option of adding an OpenSSL-allowance clause to pidgin's use of the GPL.

comment:6 follow-ups: Changed 5 years ago by abadidea

I did try to explain that you appear to have a homerolled certificate validator in lieu of the stubbed-out one but it was hard to have the conversation over twitter.

It really gave me a fright to see it stubbed out without remark though, so I have a question: what is the rationale for using a homerolled validation method separate from NSS, and could that rationale be added inline as a comment to forestall any gray hairs in the future? :)

That being said I have some other questions about said home-rolled implementation.

libpurple/certificate.c:298 /* If this is a single-certificate chain, say that it is valid */

... that doesn't sound right

libpurple/certificate.c:1671 /* Next, attempt to verify the last certificate is signed by a trusted

  • CA, or is a trusted CA (based on fingerprint). */

nor this, as it seems to be saying that you intend to accept certificates as signers that are not themselves authorities.

comment:7 in reply to: ↑ 6 Changed 5 years ago by abadidea

No edit function? By "try to explain" I mean to the original ticket opener, there was a brief twitter discussion. And my small text was supposed to be normal text with a leading carat, I got markdown'd :)

Replying to abadidea:

I did try to explain that you appear to have a homerolled certificate validator in lieu of the stubbed-out one but it was hard to have the conversation over twitter.

It really gave me a fright to see it stubbed out without remark though, so I have a question: what is the rationale for using a homerolled validation method separate from NSS, and could that rationale be added inline as a comment to forestall any gray hairs in the future? :)

That being said I have some other questions about said home-rolled implementation.

libpurple/certificate.c:298 /* If this is a single-certificate chain, say that it is valid */

... that doesn't sound right

libpurple/certificate.c:1671 /* Next, attempt to verify the last certificate is signed by a trusted

  • CA, or is a trusted CA (based on fingerprint). */

nor this, as it seems to be saying that you intend to accept certificates as signers that are not themselves authorities.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by datallah

  • Cc williamehlhardt@… added

Replying to abadidea:

I did try to explain that you appear to have a homerolled certificate validator in lieu of the stubbed-out one but it was hard to have the conversation over twitter.

It's always fun and exciting to jump to conclusions and it's easy to get people riled up on twitter :)

It really gave me a fright to see it stubbed out without remark though, so I have a question: what is the rationale for using a homerolled validation method separate from NSS, and could that rationale be added inline as a comment to forestall any gray hairs in the future? :)

That seems like a reasonable request, unfortunately I don't know the answer. The code came out of a Summer of Code project in 2008; I've CC'd William who worked on it in the hopes that he may be able to help us.

It is also possible that this is something that can be improved to take advantage of more functionality within NSS - unless there's a compelling reason to do it ourselves, it certainly seems like we'd want to make the dedicated library do what it's good at.

That being said I have some other questions about said home-rolled implementation.

Great. Code review on this is certainly welcome.

libpurple/certificate.c:298 /* If this is a single-certificate chain, say that it is valid */
^ ... that doesn't sound right

I am not particularly familiar with the cert validation code, but looking at that line, it appears that the intent of that code is to verify chained certificates' expiration and relationship to the first cert, not to compare whether the first cert itself can be traced to a valid CA and is itself effective based on date (if that makes any sense). The effective date of the first cert would have been checked previously in x509_tls_cached_start_verify and the validation against trusted CAs would happen later in x509_tls_cached_unknown_peer.

libpurple/certificate.c:1671 /* Next, attempt to verify the last certificate is signed by a trusted

  • CA, or is a trusted CA (based on fingerprint). */


^ nor this, as it seems to be saying that you intend to accept certificates as signers that are not themselves authorities.

I think what this means is "Either the last in the cert chain is signed by one of the trusted CAs or the cert chain actually ends with a trusted CA directly"

comment:9 in reply to: ↑ 8 Changed 5 years ago by wehlhard

Replying to datallah:

Replying to abadidea:

I did try to explain that you appear to have a homerolled certificate validator in lieu of the stubbed-out one but it was hard to have the conversation over twitter.

...

It really gave me a fright to see it stubbed out without remark though, so I have a question: what is the rationale for using a homerolled validation method separate from NSS, and could that rationale be added inline as a comment to forestall any gray hairs in the future? :)

That seems like a reasonable request, unfortunately I don't know the answer. The code came out of a Summer of Code project in 2008; I've CC'd William who worked on it in the hopes that he may be able to help us.

It is also possible that this is something that can be improved to take advantage of more functionality within NSS - unless there's a compelling reason to do it ourselves, it certainly seems like we'd want to make the dedicated library do what it's good at.

I honestly don't remember any more why I hand-rolled a verifier; I suspect it had to do with GnuTLS API shortcomings, real or imagined. I agree that it'd be ideal to offload as much of the SSL functionality into the library as possible, *particularly since this is security software*. This unfortunately requires some significant plumbing, since libpurple abstracts away its SSL provider so that it can support multiple libraries.

libpurple/certificate.c:298 /* If this is a single-certificate chain, say that it is valid */
^ ... that doesn't sound right

I am not particularly familiar with the cert validation code, but looking at that line, it appears that the intent of that code is to verify chained certificates' expiration and relationship to the first cert, not to compare whether the first cert itself can be traced to a valid CA and is itself effective based on date (if that makes any sense). The effective date of the first cert would have been checked previously in x509_tls_cached_start_verify and the validation against trusted CAs would happen later in x509_tls_cached_unknown_peer.

Daniel's analysis here is correct. This function only checks the integrity of the chain, not its root.

I'm pleased to see that paranoid people are actually looking for problems in this code; SSL is frequently implemented dangerously wrongly, and while I did my best, I'm certain that I didn't have the time or skills to think of everything that could possibly go wrong with security implementation. Also, I've never had a lobotomy nor any other serious head trauma, so you can rule that out as a cause.

comment:10 Changed 5 years ago by QuLogic

  • Summary changed from SSL support appears to have been written by a lobotomy victim to SSL support needs some improvements

This ticket requires a better summary. Feel free to make this more specific if necessary.

However, in the future, it would better if you don't attempt to insult the people working on the things you wanted fixed.

comment:11 Changed 5 years ago by trac-robot

  • Status changed from pending to closed

This ticket was closed automatically by the system. It was previously set to a Pending status and hasn't been updated within 14 days.

comment:12 Changed 5 years ago by calestyo

Uhm... was this fixed now? In the sense are certificates correctly verified against locally stored roots?

btw: having tickets (especially security related ones) closed automatically by a robot is really really stupid,... Actually it seems that a lot tickets in pidgin are hidden away like that (it happened also to some of mine).

comment:13 Changed 5 years ago by rekkanoryo

If a ticket is closed automatically, we set it to a "pending" status, which means that we're expecting some form of response from the original ticket reporter. If that doesn't happen within 14 days, the ticket is closed and we assume that the reporter has abandoned the ticket. I suppose you're free to think it's stupid, but it's our policy and it works for us. It is our ticketing system, after all.

comment:14 follow-up: Changed 5 years ago by calestyo

Hi.

Well the problem is IMHO, that it wasn't fully obvious in the end whether there ever was a security issue and if it was/is fixed now.

When then a robot comes along and closes the ticket, it's easily conceived as being possibly wrongfully closed or forgotten,... which in turn makes life harder for those who check the security status/bugs ad the distro level bug trackers.

So I assume there never was a problem, right?

Cheers, Chris.

comment:15 in reply to: ↑ 14 Changed 5 years ago by datallah

Replying to calestyo:

Well the problem is IMHO, that it wasn't fully obvious in the end whether there ever was a security issue and if it was/is fixed now.

If you read the comments carefully, the situation isn't hard to follow - the original complaints are individually addressed.

When then a robot comes along and closes the ticket, it's easily conceived as being possibly wrongfully closed or forgotten,... which in turn makes life harder for those who check the security status/bugs ad the distro level bug trackers.

We wouldn't have let this stay closed if the original report was accurate.

comment:16 Changed 4 years ago by Tomasz Wasilczyk <twasilczyk@…>

(In [72bdcc0f7267]):
Clean up unused ssl/NSS code and write up some comments to resolve any doubts. Refs #15308

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!