Closed
Bug 1061273
Opened 10 years ago
Closed 9 years ago
Re-assess cipher suite selection in background services
Categories
(Android Background Services Graveyard :: Core, defect)
Tracking
(firefox39 fixed, firefox40 fixed, firefox41 fixed, relnote-firefox 39+, fennec39+)
RESOLVED
FIXED
Firefox 41
People
(Reporter: rnewman, Assigned: rnewman)
References
Details
(Whiteboard: [relnote in Comment 45])
Attachments
(1 file)
57 bytes,
text/x-github-pull-request
|
nalexander
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
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.
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
(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.
Comment 4•10 years ago
|
||
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".
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/63a11f9dc5d5
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8613088 -
Flags: review?(nalexander)
Attachment #8613088 -
Flags: feedback?(rsoderberg)
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29df8adf9994
Comment 17•9 years ago
|
||
Comment on attachment 8613088 [details] [review] Pull req. Nice patch.
Attachment #8613088 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8613088 [details] [review] Pull req. atoll commented on GitHub and IRC.
Attachment #8613088 -
Flags: feedback?(rsoderberg)
Comment 19•9 years ago
|
||
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)
Comment 21•9 years ago
|
||
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
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Assignee | ||
Comment 23•9 years ago
|
||
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.
Assignee | ||
Comment 24•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f28398433bfa
Comment 25•9 years ago
|
||
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.
Assignee | ||
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/56c1a3d3b105 https://hg.mozilla.org/integration/fx-team/rev/529811243472 https://hg.mozilla.org/integration/fx-team/rev/c854c48658d3 https://hg.mozilla.org/integration/fx-team/rev/7cae1097af43
Assignee | ||
Comment 27•9 years ago
|
||
Tracking to match Bug 1164408.
tracking-fennec: --- → 39+
Flags: sec-review?(dveditz)
Flags: needinfo?(dveditz)
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/56c1a3d3b105 https://hg.mozilla.org/mozilla-central/rev/529811243472 https://hg.mozilla.org/mozilla-central/rev/c854c48658d3 https://hg.mozilla.org/mozilla-central/rev/7cae1097af43
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 30•9 years ago
|
||
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 → ---
Assignee | ||
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/075b95cd092f
Assignee | ||
Comment 32•9 years ago
|
||
Regressed on 20+ only. Now fixed and tested Sync 1.1 and 1.5 on 9, 19, and 20.
Comment 33•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/075b95cd092f
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 34•9 years ago
|
||
Note to self to investigate this for stumbler code in Fennec.
Assignee | ||
Comment 35•9 years ago
|
||
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?
Assignee | ||
Comment 36•9 years ago
|
||
N.B. for whoever lands the uplift: needs all five commits. https://hg.mozilla.org/mozilla-central/rev/56c1a3d3b105 https://hg.mozilla.org/mozilla-central/rev/529811243472 https://hg.mozilla.org/mozilla-central/rev/c854c48658d3 https://hg.mozilla.org/mozilla-central/rev/7cae1097af43 https://hg.mozilla.org/mozilla-central/rev/075b95cd092f
Comment 37•9 years ago
|
||
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 38•9 years ago
|
||
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+
Assignee | ||
Comment 39•9 years ago
|
||
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".
Comment 41•9 years ago
|
||
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.
Comment 42•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/178433fbf5d2 https://hg.mozilla.org/releases/mozilla-aurora/rev/f30ea9298a58 https://hg.mozilla.org/releases/mozilla-aurora/rev/3a976dda1698 https://hg.mozilla.org/releases/mozilla-aurora/rev/17e11de9853a https://hg.mozilla.org/releases/mozilla-aurora/rev/d7f11dc4f9da
status-firefox40:
--- → fixed
Comment 43•9 years ago
|
||
And by all 5, you really meant all 6. https://hg.mozilla.org/releases/mozilla-aurora/rev/5d28d0c44fc1
Comment 44•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/d999d29c8062 https://hg.mozilla.org/releases/mozilla-beta/rev/d9943a662657 https://hg.mozilla.org/releases/mozilla-beta/rev/676ae5e1e869 https://hg.mozilla.org/releases/mozilla-beta/rev/c2f209debd59 https://hg.mozilla.org/releases/mozilla-beta/rev/b36c17437332 https://hg.mozilla.org/releases/mozilla-beta/rev/dd38d3ccbacd
status-firefox39:
--- → fixed
Assignee | ||
Comment 45•9 years ago
|
||
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]
Comment 46•9 years ago
|
||
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.
Description
•