Closed Bug 1061273 Opened 5 years ago Closed 4 years ago

Re-assess cipher suite selection in background services

Categories

(Android Background Services Graveyard :: Core, defect)

All
Android
defect
Not set

Tracking

(firefox39 fixed, firefox40 fixed, firefox41 fixed, relnote-firefox 39+, fennec39+)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
relnote-firefox --- 39+
fennec 39+ ---

People

(Reporter: rnewman, Assigned: rnewman)

References

(Blocks 1 open bug)

Details

(Whiteboard: [relnote in Comment 45])

Attachments

(1 file)

57 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review
We no longer support SDK 8, so Bug 717691 and friends no longer apply, and our lives become substantially easier.

I imagine that the production Zeus configuration has changed since early 2012, when the supported ciphers were

 SSL_RSA_WITH_RC4_128_SHA
 SSL_RSA_WITH_RC4_128_MD5
 SSL_RSA_WITH_AES_256_CBC_SHA
 SSL_DHE_RSA_WITH_AES_256_CBC_SHA
 SSL_RSA_WITH_3DES_EDE_CBC_SHA
 SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA
 SSL_RSA_WITH_AES_128_CBC_SHA
 SSL_DHE_RSA_WITH_AES_128_CBC_SHA

We should re-assess which cipher suites we specify in our TLS socket factory, and indeed whether we should specify any at all.
I'll re-file my previous comment then since it stays relevant to that new issue :

Acceptance of all the modern cipher suites is necessary, especially the ones from TLS 1.2 suite B (RFC5288/RFC5289), TLS_(EC)DHE_(ECDSA|RSA)_WITH_AES_(128|256)_GCM_SHA(256|384).

Using this https://en.wikipedia.org/wiki/Transport_Layer_Security#Cipher as an example, it's clear that CBC cipher suites using TLS 1.0 are not enough for an acceptable security level.
N.B.: there's essentially no cleartext in payload bodies for Sync (only GUIDs and timestamps), so bulletproof transport encryption is less critical than it is for regular web traffic.
(In reply to Richard Newman [:rnewman] from comment #2)
> N.B.: there's essentially no cleartext in payload bodies for Sync (only
> GUIDs and timestamps), so bulletproof transport encryption is less critical
> than it is for regular web traffic.

Is this still true with the new sync, if somebody knows or can guess the user's password? (Often it is quite easy to guess the password.)

NIT: I suggest cleaning up the status whiteboard of this bug.
OpSec maintains guidelines for the configuration of TLS server side. We recommend that all services operated by Mozilla follow these guidelines. https://wiki.mozilla.org/Security/Server_Side_TLS

If backward compatibility is not a requirement, the guidelines provide a different configuration entitled "Non-Backward Compatible Ciphersuite".
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #3)

> Is this still true with the new sync, if somebody knows or can guess the
> user's password? (Often it is quite easy to guess the password.)

If they can perfectly guess the user's password (or can steal keys from a client), an attacker doesn't need to intercept, so no level of transport encryption will help. Otherwise, the situation isn't as bad as it might seem: the onepw doc has this to say.

  To protect the user's class-B data against a TLS-breaking
  eavesdropper...  (so the attacker gets to see authPW as it
  is sent to the server), we perform some key stretching on
  the client.

  ...

  All values sent over the wire or temporarily held in server
  memory should require at least the weaker PBKDF stretch for
  each guess.

https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol

Warner can speak more to this if needed.


> NIT: I suggest cleaning up the status whiteboard of this bug.

Thanks; Bugzilla clone.
(In reply to Julien Vehent [:ulfr] from comment #4)
> OpSec maintains guidelines for the configuration of TLS server side. We
> recommend that all services operated by Mozilla follow these guidelines.
> https://wiki.mozilla.org/Security/Server_Side_TLS

This is about Android clients; the servers and LBs already exist, supporting 5+ years of existing clients.
Keywords: qawanted
Whiteboard: [qa+][fixed in elm]
(In reply to Richard Newman [:rnewman] from comment #5)
> If they can perfectly guess the user's password (or can steal keys from a
> client), an attacker doesn't need to intercept, so no level of transport
> encryption will help.

I don't thin that is true. The attacker may be unable to penetrate the client or the server, but may be able to eavesdrop on network traffic. In other words, TLS protects against network attackers, not endpoint attackers, and not all network attackers are endpoint attackers.

> Otherwise, the situation isn't as bad as it might
> seem: the onepw doc has this to say.
> 
>   To protect the user's class-B data against a TLS-breaking
>   eavesdropper...  (so the attacker gets to see authPW as it
>   is sent to the server), we perform some key stretching on
>   the client.

Key stretching mitigates brute-force attacks against strong passwords. But, not all sync passwords are strong. For example, consider the passwords "1234" and "password" or whatever the simplest ones allowed by sync are. No amount of key stretching, on the client or the server, will protect such passwords. It is likely that many sync passwords are protected by such simple passwords. Consequently, transport security (and endpoint security) is still extremely important, AFAICT.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #7)

> I don't thin that is true. The attacker may be unable to penetrate the
> client or the server, but may be able to eavesdrop on network traffic. In
> other words, TLS protects against network attackers, not endpoint attackers,
> and not all network attackers are endpoint attackers.

I mean that if they can guess your password, they can just legitimately sign in as you and download and decrypt all of your data. No need to attack the client, server, or network traffic.

(Indeed, the FxA login process itself acts as a handy oracle to know if you'll later be able to decrypt a user's data, because the passwords always match.)



> Key stretching mitigates brute-force attacks against strong passwords. But,
> not all sync passwords are strong.

Agreed on both counts.


> Consequently, transport security (and endpoint security)
> is still extremely important, AFAICT.

I said "less critical", not "unimportant" -- after all, I did file this bug. It's less critical than if we were sending cleartext banking info, for example.
(In reply to Richard Newman [:rnewman] from comment #8)
> (In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment
> #7)
> 
> > I don't thin that is true. The attacker may be unable to penetrate the
> > client or the server, but may be able to eavesdrop on network traffic. In
> > other words, TLS protects against network attackers, not endpoint attackers,
> > and not all network attackers are endpoint attackers.
> 
> I mean that if they can guess your password, they can just legitimately sign
> in as you and download and decrypt all of your data. No need to attack the
> client, server, or network traffic.

OK, I see what you're saying now. However, guessing your password to download your data is an online brute force attack that can be mitigated through rate limiting and account locking and similar strategies, whereas stealing the data off the wire and offline brute-forcing it is much less detectable and impossible for the server to do anything to protect against.

> > Consequently, transport security (and endpoint security)
> > is still extremely important, AFAICT.
> 
> I said "less critical", not "unimportant" -- after all, I did file this bug.
> It's less critical than if we were sending cleartext banking info, for
> example.

I think we mostly agree there.
Depends on: 1081953
Blocks: 1081953
No longer depends on: 1081953
Blocks: 1164408
This seems to be the intersection between currently acceptable protocols and cipher suites (Bug 1081953) and what's supported on various Android versions (http://developer.android.com/reference/javax/net/ssl/SSLSocket.html).

Gingerbread and early Honeycomb don't support TLS1.1 or 1.2 at all, nor does Gingerbread support ECDHE cipher suites. Our modern users will do OK, though.


    if (Versions.feature20Plus) {
      DEFAULT_CIPHER_SUITES = new String[]
          {
           "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384",     // 20+
           "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384",     // 20+
           "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",        // 11+
           "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",     // 20+
           "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256",     // 20+
           "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",        // 11+
          };
    } else if (Versions.feature11Plus) {
      DEFAULT_CIPHER_SUITES = new String[]
          {
           "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",        // 11+
           "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",        // 11+
          };
    } else {       // 9+
      // Fall back to the only half-decent cipher suite supported on Gingerbread.
      DEFAULT_CIPHER_SUITES = new String[]
          {
           "TLS_DHE_RSA_WITH_AES_256_CBC_SHA"           // 9+
          };
    }

    if (Versions.feature16Plus) {
      DEFAULT_PROTOCOLS = new String[]
          {
           "TLSv1.2",
           "TLSv1.1",
          };
    } else {
      // Fall back to TLSv1 if there's nothing better.
      DEFAULT_PROTOCOLS = new String[]
          {
           "TLSv1",
          };
    }
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Whiteboard: [leave open]
Right now my changes don't seem to be taking effect:

05-28 15:30:01.062 W/0-FxAccounts(15009): javax.net.ssl.SSLHandshakeException: javax.net.ssl.SSLProtocolException: SSL handshake aborted: ssl=0x5fee95c8: Failure in SSL library, usually a protocol error
05-28 15:30:01.062 W/0-FxAccounts(15009): error:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure (external/openssl/ssl/s23_clnt.c:741 0x5cd66d5c:0x00000000)
05-28 15:30:01.062 W/0-FxAccounts(15009): 	at com.android.org.conscrypt.OpenSSLSocketImpl.startHandshake(OpenSSLSocketImpl.java:448)
05-28 15:30:01.062 W/0-FxAccounts(15009): 	at ch.boye.httpclientandroidlib.conn.ssl.SSLConnectionSocketFactory.createLayeredSocket(SSLConnectionSocketFactory.java:275)
05-28 15:30:01.062 W/0-FxAccounts(15009): 	at ch.boye.httpclientandroidlib.conn.ssl.SSLConnectionSocketFactory.connectSocket(SSLConnectionSocketFactory.java:254)
05-28 15:30:01.062 W/0-FxAccounts(15009): 	at ch.boye.httpclientandroidlib.impl.conn.HttpClientConnectionOperator.connect(HttpClientConnectionOperator.java:123)
05-28 15:30:01.062 W/0-FxAccounts(15009): 	at ch.boye.httpclientandroidlib.impl.conn.PoolingHttpClientConnectionManager.connect(PoolingHttpClientConnectionManager.java:318)


More later.
Blocks: 1169678
Attached file Pull req.
Attachment #8613088 - Flags: review?(nalexander)
Attachment #8613088 - Flags: feedback?(rsoderberg)
Daniel: this PR switches the cipher suites that we use for FHR, Sync, and FxA on Android. The main constraints are described in Comment 11 and in the blocked bugs. Feedback most welcome.

I have tested FxA Sync on a modern device; I still need to test Old Sync and on the various version boundaries.
Flags: sec-review?(dveditz)
Comment on attachment 8613088 [details] [review]
Pull req.

Nice patch.
Attachment #8613088 - Flags: review?(nalexander) → review+
Comment on attachment 8613088 [details] [review]
Pull req.

atoll commented on GitHub and IRC.
Attachment #8613088 - Flags: feedback?(rsoderberg)
Looks OK to me, but I'm going to throw this dkeeler's way (see comment 15 and comment 11)
Flags: needinfo?(dveditz)
Flags: needinfo?(dkeeler)
Looks good to me. Just a few comments.

Are suites like TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 available?

(In reply to Richard Newman [:rnewman] from comment #11)
>     if (Versions.feature20Plus) {
>       DEFAULT_CIPHER_SUITES = new String[]
>           {
>            "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384",     // 20+
>            "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384",     // 20+
>            "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",        // 11+
>            "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",     // 20+
>            "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256",     // 20+
>            "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",        // 11+

My understanding is that the AES 256 suites aren't significantly stronger than the AES 128 suites. Since they require more device time/power, I think we should prioritize the AES 128 suites instead.

>           };
>     } else if (Versions.feature11Plus) {
>       DEFAULT_CIPHER_SUITES = new String[]
>           {
>            "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",        // 11+
>            "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",        // 11+

Same idea with prioritizing AES 128 over 256.

>           };
>     } else {       // 9+
>       // Fall back to the only half-decent cipher suite supported on
> Gingerbread.
>       DEFAULT_CIPHER_SUITES = new String[]
>           {
>            "TLS_DHE_RSA_WITH_AES_256_CBC_SHA"           // 9+

Is TLS_DHE_RSA_WITH_AES_128_CBC_SHA available?
Flags: needinfo?(dkeeler)
I concur with AES128 over AES256, as that matches our server cipher string prioritizations [1].

https://wiki.mozilla.org/Security/Server_Side_TLS#Zeus_Load_Balancer.28Riverbed_Stingray.29

(In reply to David Keeler [:keeler] (use needinfo?) from comment #20)
> Is TLS_DHE_RSA_WITH_AES_128_CBC_SHA available?

+1, prioritized over AES_256.

Also, we don't support ECDSA yet on the server I think, but if you choose to add it, I recommend the prioritization order from wikimo.

https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28default.29
(In reply to David Keeler [:keeler] (use needinfo?) from comment #20)
> Looks good to me. Just a few comments.
> 
> Are suites like TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 available?

On API 20+, yes. Will add; I don't remember seeing those in either our AWS or ZLB lists, so I excluded it, but no harm in adding it.


> My understanding is that the AES 256 suites aren't significantly stronger
> than the AES 128 suites. Since they require more device time/power, I think
> we should prioritize the AES 128 suites instead.

Good to know!


> Is TLS_DHE_RSA_WITH_AES_128_CBC_SHA available?

Yes.

Thanks, David and Richard, for your comments; I'll address them.
Here's the final set. Note that I'm landing with TLSv1 for compatibility reasons; I'll explore its removal in Bug 1164408.

https://github.com/mozilla-services/android-sync/blob/13308bdfaaea242d8744a46f27c753fbb8cfb5ce/src/main/java/org/mozilla/gecko/background/common/GlobalConstants.java#L39

This'll land when I've tested it on devices; probably an hour or two.
IT's target cipher list, eventually, for MEDIUM (as shown on the wiki link in comment 21) will be as per bug 1167011#c2, but I can't speak to what CloudOps will choose to set up for you on the sync stuff.
Tracking to match Bug 1164408.
tracking-fennec: --- → 39+
Flags: sec-review?(dveditz)
Flags: needinfo?(dveditz)
Whiteboard: [leave open]
ni me for uplift after bake.
Flags: needinfo?(rnewman)
Confusingly, this seems to regress Nightly syncing to the old Sync 1.1 service, despite test connections to the same host proceeding without issue. Looking into this now.
Status: RESOLVED → REOPENED
Flags: needinfo?(rnewman)
Resolution: FIXED → ---
Regressed on 20+ only. Now fixed and tested Sync 1.1 and 1.5 on 9, 19, and 20.
https://hg.mozilla.org/mozilla-central/rev/075b95cd092f
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Note to self to investigate this for stumbler code in Fennec.
Comment on attachment 8613088 [details] [review]
Pull req.

Approval Request Comment
[Feature/regressing bug #]:
  N/A.

[User impact if declined]:
  Won't be able to sync on some partner devices; won't use modern protocols or cipher suites.

[Describe test coverage new/current, TreeHerder]:
  Thorough manual testing.

[Risks and why]: 
  Could break self-hosted Sync users. Those users can unbreak themselves by fixing their server configuration.

  In theory could break Mozilla-hosted Sync or other background services, particularly on exotic devices. I've tested quite thoroughly on a range of devices, so I'm confident.

[String/UUID change made/needed]:
  None.
Attachment #8613088 - Flags: approval-mozilla-beta?
Attachment #8613088 - Flags: approval-mozilla-aurora?
Is there documentation we can link to that explains how self-hosted Sync users can fix their server configuration?  This might be good for release notes.
Flags: needinfo?(rnewman)
Comment on attachment 8613088 [details] [review]
Pull req.

Approved for uplift to aurora and beta after discussion with rnewman about a partner deal for 39. He will be testing and verifying this on various devices on beta and will write up a relnote.
Attachment #8613088 - Flags: approval-mozilla-beta?
Attachment #8613088 - Flags: approval-mozilla-beta+
Attachment #8613088 - Flags: approval-mozilla-aurora?
Attachment #8613088 - Flags: approval-mozilla-aurora+
Release note for word-hacking:

Sync now only uses the strongest SSL configuration available on your device. If you self-host Sync, see [link] for details.



I'll put together a wiki or SUMO page later today.
(In reply to Richard Newman [:rnewman] from comment #39)
> Release note for word-hacking:
> 
> Sync now only uses the strongest SSL configuration available on your device.
> If you self-host Sync, see [link] for details.
> 
> 
> 
> I'll put together a wiki or SUMO page later today.

Minor nit: we should be using the terminology "TLS" instead of "SSL".
I'll take a look at adding this cipher suite to the mozstumbler service.  We're not using the same TLSSocketFactory so we won't benefit from this set of patches.
Release note:
 
Sync now only uses the strongest TLS configuration available on your device.
If you self-host Sync, see <http://docs.services.mozilla.com/howtos/configure-tls.html> for details.
Flags: needinfo?(rnewman)
Whiteboard: [relnote in Comment 45]
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: See comment 45.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Added to Firefox for Android 39 Release Notes.
You need to log in before you can comment on or make changes to this bug.