Closed Bug 1577688 Opened 6 years ago Closed 6 years ago

Support SCRAM-SHA-256 authentication mechanism

Categories

(Chat Core :: XMPP, enhancement)

Product:
Chat Core
Component:
XMPP
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Instantbird 71

People

(Reporter: Neustradamus, Assigned: clokep)

Details

Attachments

(2 files)

Add SCRAM-SHA-256 support
16.92 KB, patch
florian
: review+
Details | Diff | Splinter Review
Patch without whitespace changes
11.30 KB, patch
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Steps to reproduce:

My request is for all actual Thunderbird stable branches in more than trunk.

Can you add SCRAM-SHA-1(-PLUS) and SCRAM-SHA-256(-PLUS) support?

After SCRAM-SHA-1(-PLUS):

Now there is SCRAM-SHA-256(-PLUS):

"When using the SASL SCRAM mechanism, the SCRAM-SHA-256-PLUS variant SHOULD be preferred over the SCRAM-SHA-256 variant, and SHA-256 variants [RFC7677] SHOULD be preferred over SHA-1 variants [RFC5802]".

Actual results:

Expected results:

What protocol is this in reference to? XMPP?

Flags: needinfo?(Neustradamus)

Yes!

But SCRAM is not only for XMPP, for other parts, there is already a ticket here: https://bugzilla.mozilla.org/show_bug.cgi?id=1503382.

Flags: needinfo?(Neustradamus)

The code for XMPP authentication is in https://searchfox.org/comm-central/source/chat/protocols/xmpp/xmpp-authmechs.jsm.

We currently support PLAIN and SCRAM-SHA-1. It shouldn't be too hard to add the other ones. The prioritization of these is weirdly defined right now (with the assumption that only two will ever exist, I think).

Component: Instant Messaging → XMPP
Product: Thunderbird → Chat Core
Summary: Security: SCRAM-SHA → Support additional SCRAM-SHA authentication mechanisms

This adds support for SCRAM-SHA-256. It pretty much abstracts the SCRAM-SHA-1 support to allow choosing a hashing algorithm. The diff is pretty messy due to whitespace changes. Using hg diff -w helps quite a bit on it.

Tests are included for SCRAM-SHA-256 and slightly expanded for PLAIN and SCRAM-SHA-1.

I'm unsure if I need to worry about ordering of the supported mechanisms since I think the server will send us them in the order we care to use them? I'm not 100% sure on that though.

I think adding SCRAM-SHA-1-PLUS and SCRAM-SHA-256-PLUS needs to be a separate bug. I frankly don't really know what the improvement is there.

Assignee: nobody → clokep
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9090115 - Flags: review?(florian)

Neustradamus: Is there a server I can test this against?

Flags: needinfo?(Neustradamus)
Type: defect → enhancement

Thanks for your job Patrick!

Easy to add for mail section?

A lot of informations here and it is not only SCRAM-SHA-1(-PLUS) and SCRAM-SHA-256(-PLUS): https://github.com/scram-xmpp/info/issues/1
PLUS variants are complicated?

XMPP servers which already support it: Metronome IM, Tigase, Jackal :)

To test:

  • lightwitch.org has up-to-date Metronome IM
  • jackal.im I think too?
  • tigase.im no sure

Note: It has been added in Cyrus SASL (there are SCRAM-SHA-1(-PLUS), SCRAM-SHA-224(-PLUS), SCRAM-SHA-256(-PLUS), SCRAM-SHA-384(-PLUS), SCRAM-SHA-512(-PLUS) : https://github.com/cyrusimap/cyrus-sasl/commits/master

Flags: needinfo?(Neustradamus)

(In reply to Neustradamus from comment #6)

Thanks for your job Patrick!

Easy to add for mail section?

I have no idea, but please keep bugs to an individual task.

PLUS variants are complicated?

Frankly I don't understand the PLUS variants.

XMPP servers which already support it: Metronome IM, Tigase, Jackal :)

To test:

  • lightwitch.org has up-to-date Metronome IM
  • jackal.im I think too?
  • tigase.im no sure

I'll try to test on one of these at some point.

I tested this patch on lightwitch.org and confirmed that SCRAM-SHA-256 was used (and worked).

About -PLUS variants, Channel-Binding Support is needed in NSS: https://bugzilla.mozilla.org/show_bug.cgi?id=563276

I played with implementing SCRAM-SHA-512 a bit, but was unable to get logins to work using that. I've seen other reports of this happening as well. There's no RFC for SCRAM-SHA-512, so there's no official test vectors, which makes it a bit hard to test.

Florian -- I think this is ready for a final review.

Comment on attachment 9090115 [details] [diff] [review] Add SCRAM-SHA-256 support Magnus suggested that Alex could help out with this review. I'll leave Florian on here in case he has time. Alex -- I can help explain some of the surrounding code, if necessary. There isn't a huge rush on this, but it would be nice to get in sooner, rather than later, for some testing before ending up in an ESR.
Attachment #9090115 - Flags: review?(alessandro)

This is attachment 9090115 [details] [diff] [review], but with -w enabled to ignore whitespace.

Comment on attachment 9090115 [details] [diff] [review] Add SCRAM-SHA-256 support Review of attachment 9090115 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks for the diff -w and sorry for the delay.
Attachment #9090115 - Flags: review?(florian)
Attachment #9090115 - Flags: review?(alessandro)
Attachment #9090115 - Flags: review+

Pushed by clokep@gmail.com:
https://hg.mozilla.org/comm-central/rev/475ba5f24a14
Support SCRAM-SHA-256 for XMPP. r=florian

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

If any additional mechanisms are wanted, please file a follow-up bug to this one.

Summary: Support additional SCRAM-SHA authentication mechanisms → Support SCRAM-SHA-256 authentication mechanism
Target Milestone: --- → Instantbird 71

Thanks a lot to @clokep and @florian for SCRAM-SHA-256!

It can not possible to add in all actual Thunderbird branches (ESR,...)?

What is the problem with 512? :/
Have you looked in the code in Cyrus SASL, Metronome IM, Jackal, Tigase, etc.?

(In reply to Neustradamus from comment #16)

It can not possible to add in all actual Thunderbird branches (ESR,...)?

Changes need to be tested before being put onto a release branch. Once this goes through a beta period we can consider uplifting it to ESR, but I'm inclined not to. I don't see the urgency in uplifting this.

What is the problem with 512? :/

The authentication did not work when I implemented it. I do not know if it was an issue with my implementation or with the server's implementation since there's no test vectors (and no RFC).

Have you looked in the code in Cyrus SASL, Metronome IM, Jackal, Tigase, etc.?

Looking at the code of other products rarely helps. Other developers have run into issues with integration of SCRAM-SHA-512, see https://github.com/qxmpp-project/qxmpp/issues/177#issuecomment-455534422

It is already done for XMPP:

SCRAM-SHA-1-PLUS and SCRAM-SHA-256-PLUS are missing because https://bugzilla.mozilla.org/show_bug.cgi?id=563276
People can look?

Tickets:

RFCs:

IANA:

Cyrus SASL supports:

Dovecot SASL supports:

GNU SASL supports:

CRAM-MD5 to Historic:

RFC6331: Moving DIGEST-MD5 to Historic

More informations:

Dovecot supports SCRAM-SHA-256 per v2.3.10, which was released earlier last month.

After old TLS version, for TLS 1.3, there is: https://tools.ietf.org/html/draft-ietf-kitten-tls-channel-bindings-for-tls13

And there are other SCRAM too:

@clokep and @florian: Can you see to add SCRAM-SHA-512?
It is already in Cyrus SASL.

Do you wait about SCRAM-SHA3-512?

Thanks in advance.

Neustradamus -- please do not respond to closed bugs asking for additional features. Please file a separate bug for new features.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: