Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15909 closed patch (fixed)

[Patch] Support TLS 1.1/1.2 on NSS

Reported by: elrond Owned by: MarkDoliner
Milestone: 2.10.10 Component: libpurple
Version: 2.10.8 Keywords: ssl tls nss
Cc: ashmew2

Description (last modified by elrond)

NSS supports TLS 1.1 and 1.2 for some time now (for example 3.14, which is in debian/stable, brought TLS 1.1). One has to enable this in the applications (no idea, why).

pidgin should offer the maximum TLS version to the server.

This is a more dedicated followup on #8061.

Attachments (2)

pidgin-2.10.8-nss-bettertls.diff (936 bytes) - added by elrond 3 years ago.
Patch: pidgin-2.10.8-nss-bettertls.diff
pidgin-2.10.8-nss-bettertls-withlog.diff (1.2 KB) - added by ashmew2 3 years ago.

Download all attachments as: .zip

Change History (26)

Changed 3 years ago by elrond

Patch: pidgin-2.10.8-nss-bettertls.diff

comment:1 Changed 3 years ago by elrond

Okay, this patch works for a friend and has the advertised features.

Could you please consider merging this?

comment:2 Changed 3 years ago by elrond

  • Description modified (diff)

comment:3 Changed 3 years ago by deryni

Assuming those functions work the way I would naively expect them too this looks reasonable to me.

It might make sense to log things like the original default version range, the new version range and any failures in setting the new range though.

comment:4 Changed 3 years ago by elrond

These functions are documented in /usr/include/nss/ssl.h. For ease of use, I am quoting them here from version 3.14.5:

/* Returns, in |*vrange|, the range of SSL3/TLS versions supported for the
** given protocol variant by the version of libssl linked-to at runtime.
SSL_IMPORT SECStatus SSL_VersionRangeGetSupported(
    SSLProtocolVariant protocolVariant, SSLVersionRange *vrange);

/* Returns, in |*vrange|, the range of SSL3/TLS versions enabled by default
** for the given protocol variant.
SSL_IMPORT SECStatus SSL_VersionRangeGetDefault(
    SSLProtocolVariant protocolVariant, SSLVersionRange *vrange);

/* Sets the range of enabled-by-default SSL3/TLS versions for the given
** protocol variant to |*vrange|.
SSL_IMPORT SECStatus SSL_VersionRangeSetDefault(
    SSLProtocolVariant protocolVariant, const SSLVersionRange *vrange);

And probably also from interest from sslt.h:

typedef enum {
    ssl_variant_stream = 0,
    ssl_variant_datagram = 1
} SSLProtocolVariant;

typedef struct SSLVersionRangeStr {
    PRUint16 min;
    PRUint16 max;
} SSLVersionRange;

Logging .min and .max should be pretty easy. About the failure logging, that's not so easy, as SECStatus is an enum with three values: SECWouldBlock, SECFailure, SECSuccess. So only a "it failed" log entry is possible.

As you know way more about the logging, could you please add that part?

comment:5 Changed 3 years ago by datallah

I added some debugging to the NSS plugin in [9728bb0f6dcc] that is useful for testing this.

The patch seems to work well for me, but I don't know what the potential negative side-effects of doing this would be (i.e. Why is this not enabled by default in NSS?).

comment:6 Changed 3 years ago by elrond

That connection display function looks nice!

Possible reasons, why NSS did not yet change its default:

  • Some webservers are broken when contacted with tls1.2. Browsers usually try to downgrade then for the next connect. So maybe NSS wanted to stay more compatible?
  • Stay more compatible in general with old applications expect? Like "If you programmed your application with an old version, a new version will still use the exact same bytes on the wire"

Truth is: I don't know, why they did not yet upgrade the defaults. One might ask them.

comment:7 Changed 3 years ago by elrond

Regarding possible negative side effects of TLS1.2 and broken servers xnyhps on pidgin-devel has the following statements:

(18:29:51) Elrond: xnyhps: Would you suggest that those existing servers are neglible? (As in "It's 2014, let's go and offer tls1.0..1.2 to all servers, and if it breaks, the server admin has to fix it") Or would you rather suggest, that this is a real problem and one would need some downgrade option? (18:30:08) xnyhps: Yes, I would ignore them. (18:30:27) xnyhps: Adium advertises TLS 1.2 and I've had 0 reports of people failing to connect due to the version.

I have no idea about the other services/plugins/backends in purple using TLS. I hope there are only few of them and easy to test?

comment:8 Changed 3 years ago by deryni

That's a fair point. Changing this is unlikely to have an effect on XMPP servers (given xnyhps's comments based on his experience, etc.) but that says nothing about the vast and varied http servers/proxies/etc. that pidgin will need to connect through for BOSH, other protocols, etc.

Those may very well have issues with advertisements of TLS 1.2, etc.

comment:9 Changed 3 years ago by elrond

Not only bosh, but also maybe other account types that use https.

Does pidgin have beta releases / release candidates? Then I'd propose to add this patch for one of those and advertise this correctly like:

"This version enables TLS version 1.1 and 1.2 to improve security. Firefox enabled them by default in version 27, and they have been standardized for more than five years now. We do not expect any compatibility issues. If you nevertheless experience problems, please let us, your proxy operator, and your server operator know."

If this becomes a real problem, a per account config option hopefully can be added easily enough. For this, NSS has functions without the Default postfix that operate on a PRFileDesc *.

Last edited 3 years ago by elrond (previous) (diff)

comment:10 Changed 3 years ago by elrond

After some lengthy discussion on pidgin-devel, the current consensus seems to be:

  1. This should go in soon. Without any config knob for now. The only real expected problem are completely broken mitm/ssl proxies.
  2. debug logging is needed.
  3. gnutls should do the same.

About Logging:

  • I already asked for someone else helping with it. It shouldn't be too hard, really.

About GnuTLS:

  • If one wants to change things, the place is the priority string. One can do things like "+TLS-VERS-ALL" which adds everything from SSL3 to TLS1.2.
  • From looking at the GnuTLS sources, this seems to be the default.
  • If this is correct, then all that's needed for (3) above is to verify this on the wire.

comment:11 Changed 3 years ago by elrond

noon and I just tested gnutls in pidgin 2.10.8 on/from debian. It offers SSL3.0…TLSv1.2 to the server (including an impressive cipher list, not the topic on this bug).

So we're left with (2) from the previous posting.

In theory, (1) could be commited already and logging being added shortly afterwards?

comment:12 Changed 3 years ago by MarkDoliner

  • Milestone set to 2.10.10

comment:13 Changed 3 years ago by deryni

Just so I don't have to keep this open somewhere else.

(06:13:01) Elrond: It should be something like
printf("TLS versioning (min/max): Supported=%d/%d, old default=%d/%d", supported.min, supported.max, enabled.min, enabled.max);
enabled.max = supported.max;
if (setrange != SECSuccess) printf("error setting new range");
printf("new range: %d/%d", enabled.min, enabled.max);
(06:13:49) Elrond: s/%d/%ud/g
(06:14:48) Elrond: replace printf with whatever purple loves.

comment:14 Changed 3 years ago by ashmew2

Added logging to the existing patch. Check the attachment.

comment:15 follow-up: Changed 3 years ago by deryni

elrond do you know if there is a way to translate those min/max numbers into "human"? That would be a lot more useful in the log than the raw numbers.

comment:16 Changed 3 years ago by ashmew2

After applying , I get "SSL Handshake Failed" and random disconnect issues from server (using a gmail ID) .

I'm on Linux, running nss version 3.15.4

As soon as I reverted to the unpatched version of pidgin, the issue seems to have gone away,

Changed 3 years ago by ashmew2

comment:17 Changed 3 years ago by ashmew2

. Looks like there was a minor bug in the patch. I've fixed the patch and Fixing it seems to have takenm care of the issue.

comment:18 in reply to: ↑ 15 Changed 3 years ago by elrond

Replying to deryni:

elrond do you know if there is a way to translate those min/max numbers into "human"? That would be a lot more useful in the log than the raw numbers.

I have searched around the nss sources, and didn't find anything useful, sadly. What I have found, are some #defines in /usr/include/nss/sslproto.h:

/* All versions less than 3_0 are treated as SSL version 2 */
#define SSL_LIBRARY_VERSION_2                   0x0002
#define SSL_LIBRARY_VERSION_3_0                 0x0300
#define SSL_LIBRARY_VERSION_TLS_1_0             0x0301
#define SSL_LIBRARY_VERSION_TLS_1_1             0x0302
#define SSL_LIBRARY_VERSION_TLS_1_2             0x0303

Note that for older nss versions, the later ones (1.2 and/or 1.1) do not exist as defines. So code using this should be carefully #ifdef'd.

That said, I'd recommend to dump those version numbers in hex. That's not great, but anyone having ever seen those things in wireshark will understand them.

comment:19 follow-up: Changed 3 years ago by juergenhecht

Please also update the Pidgin SSL ciphers in file: /libpurple/plugins/ssl/ssl-nss.c

SSL_CipherPrefSetDefault(TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 1); SSL_CipherPrefSetDefault(TLS_DHE_RSA_WITH_AES_128_GCM_SHA256, 1);

SSL_CipherPrefSetDefault(TLS_DHE_RSA_WITH_AES_256_CBC_SHA256, 1); SSL_CipherPrefSetDefault(TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, 1); SSL_CipherPrefSetDefault(TLS_DHE_RSA_WITH_AES_128_CBC_SHA256, 1); SSL_CipherPrefSetDefault(TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, 1); SSL_CipherPrefSetDefault(TLS_DHE_RSA_WITH_AES_256_CBC_SHA, 1); SSL_CipherPrefSetDefault(TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, 1); SSL_CipherPrefSetDefault(TLS_DHE_RSA_WITH_AES_128_CBC_SHA, 1);

SSL_CipherPrefSetDefault(TLS_RSA_WITH_AES_256_CBC_SHA256, 1); SSL_CipherPrefSetDefault(TLS_RSA_WITH_AES_128_GCM_SHA256, 1); SSL_CipherPrefSetDefault(TLS_RSA_WITH_AES_128_CBC_SHA256, 1); SSL_CipherPrefSetDefault(TLS_RSA_WITH_AES_256_CBC_SHA, 1); SSL_CipherPrefSetDefault(TLS_RSA_WITH_AES_128_CBC_SHA, 1);

comment:20 in reply to: ↑ 19 Changed 3 years ago by elrond

Replying to juergenhecht:

Please also update the Pidgin SSL ciphers in file: /libpurple/plugins/ssl/ssl-nss.c

This is another topic. It should maybe go to #14668 or #8061?

comment:21 Changed 3 years ago by MarkDoliner

The attached patch does this:

if (supported.max >= enabled.max) {
    enabled.max = supported.max;
    [set range and log something]

That should be > and not >=, right? Seems like if they're equal then setting the enabled max to the supported max is a no-op. I just want to make sure that was unintentional.

comment:22 Changed 3 years ago by MarkDoliner

  • Owner set to MarkDoliner

comment:23 Changed 3 years ago by MarkDoliner

  • Resolution set to fixed
  • Status changed from new to closed

Submitted! The fix will be included in Pidgin 2.10.10, when it's released. Thanks for the patch and sorry for the delay.

Elrond: I credited you simply as "Elrond" in our ChangeLog file. If you'd prefer for us to use your full name and you'd like to be included in our COPYRIGHT file, please let me know what it is.


comment:24 Changed 3 years ago by Mark Doliner <mark@…>

(In [f4e63e354f45]):
Allow and prefer TLS 1.2 and 1.1 when using libnss. Patch from Elrond, with additional logging from Ashish Gupta.

Fixes #15909

FYI gnutls enables TLS 1.2 and 1.1 by default, when available, so there's no need to mirror this change in that code.

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!