Closed
Bug 847032
Opened 12 years ago
Closed 11 years ago
[email] IMAP only supports initially-SSL encrypted connections, STARTTLS upgrade not supported from unencrypted cleartext
Categories
(Firefox OS Graveyard :: Gaia::E-Mail, defect)
Firefox OS Graveyard
Gaia::E-Mail
Tracking
(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)
People
(Reporter: robert, Assigned: mcav)
References
Details
(Keywords: late-l10n)
Attachments
(7 files)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130218103317
Steps to reproduce:
tried autoconfig with STARTTLS
Actual results:
autoconfig with STARTTLS didnt work
cause of code only SSL is accepted, but STARTTLS is lookuped too before
{
* type: 'imap+smtp',
* incoming: {
* hostname: <imap hostname>,
* port: <imap port number>,
* socketType: <one of 'plain', 'SSL', 'STARTTLS'>,
* username: <imap username>,
* },
* outgoing: {
* hostname: <smtp hostname>,
* port: <smtp port>,
* socketType: <one of 'plain', 'SSL', 'STARTTLS'>,
* username: <smtp username>,
* },
* }
// We do not support unencrypted connections outside of unit tests.
if (config.incoming.socketType !== 'SSL' ||
config.outgoing.socketType !== 'SSL') {
callback('no-config-info', null, { status: 'unsafe' });
return;
Expected results:
STARTTLS is secure ,dont forbid it
Reporter | ||
Comment 1•12 years ago
|
||
try with testaccount decribed at
http://automx.org/en/
Comment 2•12 years ago
|
||
The problem is that mozTCPSocket doesn't support STARTTLS at this time. We had a bug on that, but no bug here in the e-mail components. I've marked this bug as dependent on the mozTCPSocket bug. Thanks for filing this bug!
Status: UNCONFIRMED → NEW
Depends on: 784816
Ever confirmed: true
Summary: autoconfig Gaia::E-Mail only SSL no STARTTLS → [email] IMAP only supports initially-SSL encrypted connections, STARTTLS upgrade not supported from unencrypted cleartext
Comment 4•11 years ago
|
||
Triage: This bug has been added to the v1.2 backlog but is not blocking release
blocking-b2g: koi? → -
Comment 5•11 years ago
|
||
Since bug 784816 has landed, this bug is now actionable. I've moved it into the e-mail app's Pivotal Tracker backlog from the icebox (our pivotal tracker dashboard is at https://www.pivotaltracker.com/s/projects/867311)
This primarily entails:
* UI
** un-collapse the 'select' combo-boxes in setup_manual_config.html and add startTLS options back. This includes adding a new string for startTLS and an l10n note that indicates the string, like SSL above it, probably should not be translated because it's technical.
** Probably add some logic to the combo-box so that we change the port to 587 when STARTTLS is selected (if previously set to 465) and to 465 when SSL is selected (if previously set to 587). It might make sense to move the select boxes above the port selection so that the user can avoid manually having to type the port-number (and potentially typo it) if they follow the order we put them in.
* Backend
** Modify mailapi/worker-support/configparser-main.js to allow socketType values of 'STARTTLS'. Right now we intentionally explode on things that aren't 'SSL'. 'plain' is still an illegal value, as are all other values.
** Uncomment the commented out logic in imap.js in ImapConnection.prototype.connect that has us try and perform the upgrade and fix it up. From https://bugzilla.mozilla.org/show_bug.cgi?id=784816#c56 and friends I think what we basically want is to send our 'STARTTLS' command over the wire, wait for the OK so we know the receiving pipline is clear and there's no chance of TLS upgrade getting confused, then trigger the socket upgrade via startTLS() then immediately call fnInit() so we get the CAPABILITY command in the pipe to trigger the upgrade and get those results back. I think the only reason we'd want to wait for the server to ACK our STARTTLS command with an OK
** Update node-net.js and net-main.js to relay the startTLS upgrade call across.
Note that the semantics of upgrade failure as documented in bug 784816 are that the connection will error out and close if we can't upgrade, so bug 911426 on having readyState indicate our security status is not required.
Automated testing of this is somewhat annoying, and indeed for that reason we currently don't actually use SSL in our tests. We'll want to add cases as follows, I think
1) in test_autoconfig.js, create a variation on goodImapXML and its uses to make sure we're okay with STARTTLS.
but otherwise I think we need to do manual verification for now. Namely:
1) Verify that we can setup a startTLS account with a server serving legit certificates on port 587. Ensure we're really using port 587 and that the connection is actually encrypted by using wireshark or other packet sniffer.
2) Verify that we fail to setup a startTLS account with a server with bad certificates with the appropriate error message. (Localhost dovecot can work for this.)
3) Verify that we fail to setup a startTLS account with a server that doesn't advertise STARTTLS. Ideally quickly. It would suck a bit if we did 'A01 STARTTLS' and it said 'NO' and then we still tried to perform a TLS upgrade but we timed out with the server in the ensuing confusion.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mcav
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #799627 -
Flags: review?(bugmail)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #799628 -
Flags: review?(bugmail)
Assignee | ||
Comment 8•11 years ago
|
||
There are three patches here: SimpleSMTP, GELAM, and Gaia. The Gaia PR currently includes all of the other changes (via "make install-into-gaia") because it's probably easier for you to test that way, but I'd be happy to pull that out and only show the UI changes if you'd prefer that, :asuth.
This adds support for STARTTLS for both IMAP and SMTP servers.
I tested STARTTLS support using an aol.com email account -- while the AOL documentation formally says you're supposed to use SSL ports, they also support STARTTLS for both IMAP and SMTP on the other ports. I verified using Wireshark that the encryption handshake occurs properly and that you can read and send e-mail using STARTTLS connections.
Using a local dovecot server with an invalid cert results in an error, as expected ("bad-security").
Connecting to a server that says NO to STARTTLS (tested using netcat) results in immediate connection failure, as expected.
Attachment #799659 -
Flags: review?(bugmail)
Updated•11 years ago
|
Target Milestone: --- → 1.2 FC (16sep)
Comment 9•11 years ago
|
||
I took a look at this when it went up last week but didn't want to formally review it since I thought the mozTCPSocket startTLS stuff might have regressed us. Which it did, so I backed it out. I'll review this for real once the fixed mozTCPSocket lands again. This looked good in general, but I'll do my test runs, etc. then.
Comment 10•11 years ago
|
||
Comment on attachment 799627 [details]
SimpleSMTP pull request
Looking at the simplesmtp code that we're using and on trunk, its TLS upgrade is opportunistic. If the server does not advertise STARTTLS support then an upgrade will not be initiated. We need to force the TLS upgrade and/or generate a connection error if support is not advertised more directly.
Attachment #799627 -
Flags: review?(bugmail) → review-
Comment 11•11 years ago
|
||
Comment on attachment 799628 [details]
gelam-pull-request.html
As noted in the pull request, there's an incoming/outgoing typo. Let's add another autoconfig entry that has STARTTLS not match SSL (only 1 of the 2 variants required).
I think we may also want tests in test_imap_prober and test_smtp_prober after all that verify that if the server does not advertise STARTTLS that we get angry or at least we try and initiate a STARTTLS upgrade regardless.
This worked for me with the incoming/outgoing change.
Attachment #799628 -
Flags: review?(bugmail) → review-
Comment 12•11 years ago
|
||
Comment on attachment 799659 [details]
Gaia Pull Request
Let's add a gaia unit test for the select box change-o magic. The logic looks good and works right when I test it by hand, but this is exactly what the unit test infrastructure is for. (I do *not* believe we need integration test coverage for it, however.)
Otherwise good!
Attachment #799659 -
Flags: review?(bugmail) → review-
Assignee | ||
Comment 13•11 years ago
|
||
Okay, have another look. I believe I addressed all of your comments:
- simplesmtp now takes just a "crypto" flag instead of "secureConnection" and "ignoreTLS", so that it can clearly distinguish between requests for a plain socket vs STARTTLS vs SSL. I was going to just keep the old flags and always force STARTTLS, but the tests require support for "plain", so rather than adding another flag I just passed along our existing ".crypto" flag. Seems clearer to me.
- in "starttls" mode, simplesmtp will always try to establish a STARTTLS connection whether the server says it supports it or not. If the server fails to establish the connection, it fails out.
- Fixed the glitches in GELAM as requested, and added tests for test_imap_prober and test_smtp_prober.
- Added a gaia unit test for port selection change logic.
The tests pass for me (with the checked-out simplesmtp) and I verified that I can still log in using a STARTTLS account. Let me know if I missed anything else. Thanks!
Assignee | ||
Updated•11 years ago
|
Attachment #799659 -
Flags: review- → review?(bugmail)
Assignee | ||
Updated•11 years ago
|
Attachment #799628 -
Flags: review- → review?(bugmail)
Assignee | ||
Updated•11 years ago
|
Attachment #799627 -
Flags: review- → review?(bugmail)
Assignee | ||
Comment 14•11 years ago
|
||
Oh, and one more thing: Gaia was leaking the global 'hookupInputAreaResetButtons' from input_areas.js; I see that there's a bug about widening its scope to all apps, but since it's only used in email for now I made it a proper module so that mocha doesn't complain about leaking globals.
Comment 15•11 years ago
|
||
Comment on attachment 799627 [details]
SimpleSMTP pull request
There was still the potential for startTLS bypass if the server claimed to not support EHLO. I tacked on a simple fix for this based on your existing "hey, I want startTLS!" logic. r=asuth conditional on that, which I'm landing for time reasons.
I definitely like the cleanup of just passing the crypto in!
Attachment #799627 -
Flags: review?(bugmail) → review+
Comment 16•11 years ago
|
||
Comment on attachment 799628 [details]
gelam-pull-request.html
Thanks for the extra unit test! I added another variant on it for the HELO bypass case noted in the previous review comment. I also did the console.log hookup so we can see the SMTP traffic in the pretty logs. I've landed that as an extra commit and pulling in the simplesmtp changes.
Attachment #799628 -
Flags: review?(bugmail) → review+
Updated•11 years ago
|
Attachment #799659 -
Flags: review?(bugmail) → review+
Comment 17•11 years ago
|
||
landed:
https://github.com/asutherland/simplesmtp/pull/2
https://github.com/asutherland/simplesmtp/commit/fb31cae0222c4ff7d3c7c639a9040c3f7f18bde6
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/251
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/2ebe315dab755e5c625a4507f32f8c53273985bf
https://github.com/mozilla-b2g/gaia/pull/12216
https://github.com/mozilla-b2g/gaia/commit/7596b8368097c3d5bd4c5fafd553dc6bfc2e3520
I'll prepare a v1-train uplift patch.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 18•11 years ago
|
||
triage, please leo+ this bug as i suspect it blocks bug 914520, which was reported by partner. ni? asuth to confirm.
blocking-b2g: - → leo?
Flags: needinfo?(bugmail)
Comment 19•11 years ago
|
||
tested and landed on v1-train. b2g18 fix just landed on bug 784816 so the new stuff won't work until tomorrow's builds
https://github.com/mozilla-b2g/gaia/pull/12229
https://github.com/mozilla-b2g/gaia/commit/8b20ff956f8e366a17a7ef5f64935a6a6a8cb0c0
status-b2g18:
--- → fixed
Flags: needinfo?(bugmail)
Comment 20•11 years ago
|
||
triage: leo+, raised as critical during work week
blocking-b2g: leo? → leo+
Comment 21•11 years ago
|
||
I/Gecko ( 675): TCPSocket: Host info: securesmtp.t-online.de:587
I/Gecko ( 675):
I/Gecko ( 675): TCPSocket: SSL: false
I/Gecko ( 675):
I/Gecko ( 143): TCPSocket: content process: false
I/Gecko ( 143):
I/Gecko ( 143): TCPSocket: window init: undefined
I/Gecko ( 143): TCPSocket: startup called
I/Gecko ( 143):
I/Gecko ( 143): TCPSocket: Host info: securesmtp.t-online.de:587
I/Gecko ( 143):
I/Gecko ( 143): TCPSocket: SSL: false
I/Gecko ( 143):
I/Gecko ( 675): WWAR: PROBE:SMTP sad. error: | server-problem | |
I/Gecko ( 675): WWAR: PROBE:IMAP sad. Error | BAD | Error while executing request: Invalid login | Invalid login
Comment 22•11 years ago
|
||
Only "inbox" folder.
I/Gecko ( 1139): WERR: Error: This server doesn't support the command FolderSync
I/Gecko ( 1139): WLOG: runOp(check: {"type":"syncFolderList","longtermId":"1/0","lifecycle":"do","localStatus":"done","serverStatus":"checking","tryCount":1,"humanOp":"syncFolderList"})
I/Gecko ( 1139): WLOG: runOp(check: {"type":"syncFolderList","longtermId":"1/0","lifecycle":"do","localStatus":"done","serverStatus":"checking","tryCount":6,"humanOp":"syncFolderList"})
Comment 23•11 years ago
|
||
Comment 24•11 years ago
|
||
Comment 25•11 years ago
|
||
v1.1.0hd: 8b20ff956f8e366a17a7ef5f64935a6a6a8cb0c0
v1.1.0hd: 4231e3f6d72badd3c3aa3394f37e0fa4cb5ce002
status-b2g-v1.1hd:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•