784816 - Implement startTLS for MozTCPSocket

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Andrew Sutherland [:asuth] (he/him)]

+++ This bug was initially created as a clone of Bug #733573 +++

Bug 733573 currently includes support for unencrypted cleartext connections and starting a connection with SSL/TLS active from the get-go, but does not include upgrade-to-TLS from unencrypted cleartext.

Both IMAP and SMTP support a variant of this; in IMAP, it is the STARTTLS command.

This was initially deferred from bug 733573 because it was not required for immediate needs (the known IMAP servers of interest that support STARTTLS also support SSL/TLS from the get-go) and there were complications related to buffering and use of the TLS upgrade.

The MozTCPSocket implementation ended up changing its buffering solution by the time it landed, so this is believed to be pretty simple.  The key implementation need is to make sure that all buffers filled/enqueued prior to the call to startTLS on MozTCPSocket get flushed before the TLS upgrade is performed.  Ideally, all buffers written to after calling startTLS will appropriately wait for the TLS negotiation to occcur as well.

In terms of configuration, the way this worked before startTLS support was removed was that 'starttls' is passed as the crypto option instead of (Boolean)true, but the gaia e-mail client is flexible about this.

NB: Since MozTCPSocket moved from b2g/ to dom/, it might be appropriate to move this bug, but since I'm cloning the bug, I'm leaving it b2g for now in the hopes someone else has a better idea of where to migrate it to.

Because there are no known servers that only support TLS upgrade (and not initial SSL/TLS), I don't think this should be a b2g blocker, but I also think that we really need to do this now so we don't end up with a partial TCP implementation and while the code is fresh in the minds of the authors/reviewers.

Comment 1

[Andrew Overholt [:overholt]]

Donovan, do you think this will make feature freeze?  Thanks.

Comment 2

[Donovan Preston [:fzzzy]]

No.

Comment 3

[Andrew Overholt [:overholt]]

I spoke with Andrew Sutherland about this and while it's low-hanging fruit that would be nice to have, it's not a blocker for the B2G mail app for v1 so I'm removing the blocker+.  As always, if anyone disagrees, feel free to re-nom.

Comment 4

[Faramarz Rashed]

We just covered this in the Gaia meeting. Just wanting to make sure that this doesn't mean we won't have a secure way of emailing; and Andrew S. assured us we would.  Andrew, can you confirm in this bug please?

Comment 5

[Donovan Preston [:fzzzy]]

SSL works without this bug; this bug is only for TLS, which is creating a socket unencrypted and then upgrading it to encrypted.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Faramarz (:faramarz) from comment #4)
> We just covered this in the Gaia meeting. Just wanting to make sure that
> this doesn't mean we won't have a secure way of emailing; and Andrew S.
> assured us we would.  Andrew, can you confirm in this bug please?

Yes.  We are establishing encrypted connections (technically TLS which has succeeded SSL, but 'SSL' in our parlance) for IMAP and SMTP using the TCP API.

Although we support unencrypted/plaintext connections in the back-end, this is primarily for testing.  We are attempting to make it impossible to create an account that is unencrypted because the risks are simply so high given that the devices may be used over suspect wi-fi networks, etc.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(ps: where I said plaintext I meant cleartext.)

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Andrew Sutherland (:asuth) from comment #6)

> Although we support unencrypted/plaintext connections in the back-end, this
> is primarily for testing.  We are attempting to make it impossible to create
> an account that is unencrypted because the risks are simply so high given
> that the devices may be used over suspect wi-fi networks, etc.

+1 awesome!

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Ben Bucksh did some fantastic digging into the Thunderbird ISP database on a post to the tb-planning list to look for servers that may require STARTTLS because of a lack of pure SSL support.  It also covers entries that appear to lack crypto support entirely or may require POP3:
https://mail.mozilla.org/pipermail/tb-planning/2013-January/002579.html

The relevant bits are:
===
comcast.net - SMTP is STARTTLS only, only POP3
email.it - SMTP is STARTTLS only
ewetel.de - SMTP is STARTTLS only
hotmail.com - SMTP is STARTTLS only, only POP3
some unheard-of Japanese ISPs - STARTTLS only
inbox.lt, inbox.lv - SMTP is STARTTLS only
kabelmail.de - POP3 only
posteo.de - STARTTLS only
telenor.dk (with lots of domains) - STARTTLS only
skynet.be - SMTP is STARTTLS only
sympatico.ca - SMTP is STARTTLS only
t-online.de - SMTP is STARTTLS only (they are huge!)
web.de - SMTP is STARTTLS only (they are huge!)
uol.com.br - SMTP is STARTTLS only (that's your target group, right?)
===

===
Please note that I generally tried to find and list all working configs, not only those that are officially supported, so if there's no SSL server listed, then most likely there simply is no working one (at least at the time when we did the config).

I just double-checked some of the important configs that have STARTTLS, but no SSL:

 * T-Online securesmtp.t-online.de works with SSL on port 465, so we
   could add that. I only checked whether the server responds, but not
   whether the certificate is OK and a login actually works.
 * ditto uol.com.br
 * smtp.web.de SSL 465 does not work. web.de is one of the biggest mail
   providers in Germany, and in the top 5 world-wide for Thunderbird.
 * smtpauth.peoplepc.com SSL 465 blackholes, no response at all
 * ditto earthlink, mindspring.com 
===

Nominating for tracking since this is not currently tracked.

Comment 10

[Alex Keybl [:akeybl]]

New feature, unless this is a blocker for a future product/version, we won't be taking in v1.x.

Comment 11

[Andreas Gal :gal]

I am adding a comment pointing to this bug to imap.js to use node-like starttls emulation for this.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Needed for competitive parity, I think.  If we can emulate it then p2.

Comment 13

[Florian Bender]

For T-Online: it's a branch of Telekom which Mozilla is partnering with. So you might be able to do something here on the provider side. 

This will probably not work for WEB.DE. It's mostly used for Freemail, though there is a Premium option (which only few people use). The Freemail product is inherently bad (e. g. they only offer 12MB Mail storage unless you install their crappy toolbar). I guess they'll have little motivation to upgrade any of their software for the free users (their Premium product provides IMAP and maybe SSL/TLS without STARTTLS). However, WEB.DE focuses on the German market iirc thus it should probably not be high prio.

Comment 14

[Honza Bambas (:mayhemer)]

I think this should not be that hard to implement with the architecture we have.

Comment 15

[Honza Bambas (:mayhemer)]

What is the priority of this bug?

Comment 16

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #15)
> What is the priority of this bug?

What do you think it should be? When this bug was first opened we decided it wasn't life or death because regular ssl worked, and doing this properly would have been difficult the way the code was originally written. Now that the socket code uses MultiplexStream, it should be straightforward to implement this by setting a flag, creating a new MultiplexStream to store new sent strings in, waiting until the old insecure MultiplexStream is empty, and then performing the TLS upgrade.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Peter, we would like to prioritize this (and the related e-mail bug 847032) for v1.2 for e-mail.  I'm abusing needinfo here for e-mail feature triage.

Comment 18

[Peter Dolanjski [:pdol]]

Nom'd for blocking 1.2 (Koi).

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Peter Dolanjski [:pdol] from comment #18)
> Nom'd for blocking 1.2 (Koi).

Thanks.  What is the 1.2 deadline or where can I find the dates (there is wiki page for this I think) ?

(In reply to Donovan Preston from comment #16)
> (In reply to Honza Bambas (:mayhemer) from comment #15)
> > What is the priority of this bug?
> 
> What do you think it should be? ... then performing the TLS upgrade.

I was asking for priority.  I know how to implement it, but the thing is that someone has to do it.  I wanted to know whether I should preserve some cycles for this soon or not.

Comment 20

[Vincent]

Could you work on it Honza Bambas? You could solve https://bugzilla.mozilla.org/show_bug.cgi?id=847032

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Is anyone working on this? If not, can I take this bug?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Patrick, I don't think anyone is working on this. But please coordinate the implementation strategy with Honza since he module peer for this code. It would also be good to keep Donovan in the loop since he wrote the original TCPSocket implementation.

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Implement startTLS in MozTCPSocket

Comment 24

[Vincent]

(In reply to Patrick Wang [:kk1fff] from comment #23)
> Created attachment 789416 [details] [diff] [review]
> Implement startTLS in MozTCPSocket

OMG, thousand thanks to you, Patrick Wang. Waited for so long time! Thank you again.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 789416 [details] [diff] [review]
Implement startTLS in MozTCPSocket

Review of attachment 789416 [details] [diff] [review]:
-----------------------------------------------------------------

That's it.

Test?

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +73,5 @@
>     */
>    nsIDOMTCPServerSocket listen(in unsigned short localPort, [optional] in jsval options,
>                                 [optional] in unsigned short backlog);
>  
> +  void startTLS();

IID change

::: dom/network/src/TCPSocket.js
@@ +157,5 @@
>    _socketBridge: null,
>  
> +  // StartTLS
> +  _waitingForStartTLS: false,
> +  _pendingData: [],

_pendingDataAfterStartTLS ?

@@ +265,5 @@
> +            if (self._pendingData.length > 0) {
> +              for (let i = 0; i < self._pendingData.length; i++) {
> +                self._multiplexStream.appendStream(self._pendingData[i]);
> +              }
> +              self._pendingData.length = 0;

clear() ?

@@ +270,5 @@
> +              self._ensureCopying();
> +            } else if (self._waitingForDrain) {
> +              self._waitingForDrain = false;
> +              self.callListener("drain");
> +            }

this is duplicated.  thinking of just 'return;' after self._ensureCopying(); and when no pending data let this method execution continue.  Up to you.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Before we removed startTLS support from the original TCPSocket landing, we only allowed for startTLS upgrade if useSSL was 'starttls'.  Is there an underlying resource overhead for asking for startTLS support for a connection that will only ever be cleartext TCP connection?

Also, should we migrate to be closer to the ongoing raw sockets spec work?  Right now, the TCPSocket constructor spec calls for the third argument to be an options dictionary: http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions

The encryption stuff isn't spec'ed well there, but presumably we can impact that.  It seems like 'useSecureTransport' there isn't thought out and you would need to explicitly indicate SSL or starttls support, presumably via a string value like 'ssl' or 'starttls'.  Or use something like 'initial' or 'upgrade' to indicate how we would go secure since we're really using TLS and it's a question of whether we fundamentally require encryption from the get-go or will negotiate up to it after a chat with the server.

Comment 27

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Andrew Sutherland (:asuth) from comment #26)
> Before we removed startTLS support from the original TCPSocket landing, we
> only allowed for startTLS upgrade if useSSL was 'starttls'.  Is there an
> underlying resource overhead for asking for startTLS support for a
> connection that will only ever be cleartext TCP connection?
> 
> Also, should we migrate to be closer to the ongoing raw sockets spec work? 
> Right now, the TCPSocket constructor spec calls for the third argument to be
> an options dictionary:
> http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions
> 
> The encryption stuff isn't spec'ed well there, but presumably we can impact
> that.  It seems like 'useSecureTransport' there isn't thought out and you
> would need to explicitly indicate SSL or starttls support, presumably via a
> string value like 'ssl' or 'starttls'.  Or use something like 'initial' or
> 'upgrade' to indicate how we would go secure since we're really using TLS
> and it's a question of whether we fundamentally require encryption from the
> get-go or will negotiate up to it after a chat with the server.

Sorry, I missed this comment.

A starttls socket in gecko is more like an SSL socket, I believe it introduces more overhead than a plain socket does. But I think it will be more straightforward for people who use MozTCPSocket to upgrade any insecure socket to secure by calling a method like startTLS. As we already use SSL socket a lot in our code, I believe the overhead should be acceptable. I suggest that we just use startTLS socket in any MozTCPSocket that isn't created with ssl.

So we have two questions:
1. Should we change the option name to 'useSecureTransport' to make our interface be closer to raw socket spec?
2. Should provide a way to create a TCPSocket that won't go secure?

Honza, do you have any thoughts?

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Patrick Wang [:kk1fff] from comment #27)
> (In reply to Andrew Sutherland (:asuth) from comment #26)
> > Before we removed startTLS support from the original TCPSocket landing, we
> > only allowed for startTLS upgrade if useSSL was 'starttls'.  Is there an
> > underlying resource overhead for asking for startTLS support for a
> > connection that will only ever be cleartext TCP connection?
> > 
> > Also, should we migrate to be closer to the ongoing raw sockets spec work? 
> > Right now, the TCPSocket constructor spec calls for the third argument to be
> > an options dictionary:
> > http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions
> > 
> > The encryption stuff isn't spec'ed well there, but presumably we can impact
> > that.  It seems like 'useSecureTransport' there isn't thought out and you
> > would need to explicitly indicate SSL or starttls support, presumably via a
> > string value like 'ssl' or 'starttls'.  Or use something like 'initial' or
> > 'upgrade' to indicate how we would go secure since we're really using TLS
> > and it's a question of whether we fundamentally require encryption from the
> > get-go or will negotiate up to it after a chat with the server.
> 
> Sorry, I missed this comment.
> 
> A starttls socket in gecko is more like an SSL socket, I believe it
> introduces more overhead than a plain socket does. But I think it will be
> more straightforward for people who use MozTCPSocket to upgrade any insecure
> socket to secure by calling a method like startTLS. As we already use SSL
> socket a lot in our code, I believe the overhead should be acceptable. I
> suggest that we just use startTLS socket in any MozTCPSocket that isn't
> created with ssl.
> 

I don't understand the concerns.  What overhead are you talking about?

> So we have two questions:
> 1. Should we change the option name to 'useSecureTransport' to make our
> interface be closer to raw socket spec?

For spec conformation ask :sicking

> 2. Should provide a way to create a TCPSocket that won't go secure?

What would be a reason??

When creating a socket you create it either as raw or ssl (i.e. ssl is setup on the socket sooner then any data go out or in).  When the socket is raw, you can - depending on protocol demands - layer it with ssl with startTLS().  I don't see any other use case.

Maybe I just don't understand what you are asking me for.

Comment 29

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #28)
> I don't understand the concerns.  What overhead are you talking about?

The question is basically: Is memoryUse(unencrypted TCP without the ability to call startTLS()) roughly the same as memoryUse(unencrypted TCP with startTLS upgrade possible but never called)?

Presumably there is some difference, and presumably the difference is small enough to not matter.  I just thought it was worth checking.

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Andrew Sutherland (:asuth) from comment #29)
> (In reply to Honza Bambas (:mayhemer) from comment #28)
> > I don't understand the concerns.  What overhead are you talking about?
> 
> The question is basically: Is memoryUse(unencrypted TCP without the ability
> to call startTLS()) roughly the same as memoryUse(unencrypted TCP with
> startTLS upgrade possible but never called)?
> 
> Presumably there is some difference, and presumably the difference is small
> enough to not matter.  I just thought it was worth checking.

There is absolutely no difference for the two cases.

Comment 31

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Jonas, based on comment 26, in raw socket spec (http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions), the option name of enabling secure is 'useSecureTransport', while the name is 'useSSL' in our implementation. Should we also change the option name to move closer to the spec?

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I suggest we keep useSSL for now, we can always file a bug to change the option name when we are ready to make this change. Andrew, what do you think?

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Fine with me.  In general we probably want to maintain backwards compatibility anyways.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The long term goal is definitely to align fully with the spec. Including removing properties that are not in the spec, and dropping support for the current API.

We can do that separately when we unprefix the API though. But of course the smaller the differences between the prefixed and unprefixed API, the easier migration will be.

So I would prefer to align with the spec as soon as we can. And we should definitely do it when we're adding new features.

Of course, if we think that our names/APIs are better than what's in the spec, please do provide feedback to the spec. We should definitely not be shy about doing that.

Comment 35

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks, Jonas. I will update the option name.

Comment 36

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Honza Bambas (:mayhemer) from comment #25)
> @@ +265,5 @@
> > +            if (self._pendingData.length > 0) {
> > +              for (let i = 0; i < self._pendingData.length; i++) {
> > +                self._multiplexStream.appendStream(self._pendingData[i]);
> > +              }
> > +              self._pendingData.length = 0;
> 
> clear() ?

I didn't find clear() method. my intention is to clear the array.

Comment 37

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: Implement startTLS in MozTCPSocket

Comment 38

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - Use useSecureTransport option in Gaia.

Change in Gaia. In this patch I just add 'useSecureTransport' in option. I think we can remove useSSL after Gecko change landed.

Comment 39

[Thinker Li [:sinker]]

I think the API worked here is too simplify what TLS is.  It seems stick to serve the requirement of the mail app.  But, by considering common use cases of TLS, we need to provide the ways to configure TLS itself.  For example, to implement an VPN protocol, it at least needs to install or select the certificate you want to use.

Comment 40

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Thinker Li [:sinker] from comment #39)
> I think the API worked here is too simplify what TLS is.

It's probably best to directly discuss this as part of the standards process.  I think https://github.com/sysapps/raw-sockets/issues/10 or another issue there is probably the right place for that.  Or on http://lists.w3.org/Archives/Public/public-sysapps/

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 796068 [details]
Patch: Part 2 - Use useSecureTransport option in Gaia.

Thanks for the patch.  Unfortunately, the files in gaia/apps/email/js/ext/ are compiled byproducts from the gaia-email-libs-and-more repo.  I've prepared an appropriate patch with an added comment for gaia-email-libs-and-more and landed it there and on gaia after testing a b2g-desktop build with your gecko patch and without it with the gaia patch applied.

landed:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/241/
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/5bb84b91e4c6cdfdc9164e6d14242b7548d4bd75

https://github.com/mozilla-b2g/gaia/pull/11796
https://github.com/mozilla-b2g/gaia/commit/d5e7d4c39dee68dc7f3558980804ce8ea0015790

We'll take care of removing the old useSSL instance after the gecko patch lands and when we implement startTLS support/usage from the e-mail app.

Comment 42

[Andrew Sutherland [:asuth] (he/him)]

Additional note: previous TCPSocket changes have broken the Firefox OS simulator's adb logic, but I just checked https://github.com/mykmelez/r2d2b2g/blob/master/addon/lib/adb/adb-socket.js and no SSL is used, so I think that covers the consumers I know of.  (I do not profess to know all consumers outside of gaia, however.)

Comment 43

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #30)
> (In reply to Andrew Sutherland (:asuth) from comment #29)
> > The question is basically: Is memoryUse(unencrypted TCP without the ability
> > to call startTLS()) roughly the same as memoryUse(unencrypted TCP with
> > startTLS upgrade possible but never called)?
> 
> There is absolutely no difference for the two cases.

I've double checked and there is a difference...  

For "startTLS" we add a ssl socket over the tcp socket, but set it up to do not start the negotiation.  So, there is more memory consumption per socket.

Based on that it would be better to have an option to distinguish between startTLS and no-security.

Other option could be to simply add an ssl socket over the tco socket in your JS implementation of startTLS().  But I'm not sure how complicated and even possible would be to do this purely in JS.

Sorry, I somehow misinterpreted the question at the first place.

Comment 44

[Honza Bambas (:mayhemer)]

Comment on attachment 796066 [details] [diff] [review]
Patch: Part 1: Implement startTLS in MozTCPSocket

Review of attachment 796066 [details] [diff] [review]:
-----------------------------------------------------------------

r+.  Comment 43 can be fixed in a followup, but since it changes the API we might want to ship consistently.  It means the startTLS option should be in the same release as this patch is, at least.

::: dom/network/src/TCPSocket.js
@@ +212,5 @@
>    set onclose(f) {
>      this._onclose = f;
>    },
>  
> +  _activeTLS: function() {

_activateTLS ?

@@ +229,1 @@
>      }

As I wrote in comment 43.

@@ +265,5 @@
> +            if (self._pendingDataAfterStartTLS.length > 0) {
> +              for (let i = 0; i < self._pendingDataAfterStartTLS.length; i++) {
> +                self._multiplexStream.appendStream(self._pendingDataAfterStartTLS[i]);
> +              }
> +              self._pendingDataAfterStartTLS.length = 0;

FYI: you can also do:

while (self._pendingDataAfterStartTLS.length)
  self._multiplexStream.appendStream(self._pendingDataAfterStartTLS.shift());

Up to you.

@@ +488,5 @@
> +      throw new Error("Socket not open.");
> +    }
> +    if (this._ssl == 'ssl') {
> +      // Already SSL
> +      return;

I have not read the spec, but shouldn't we throw?

Comment 45

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Honza Bambas (:mayhemer) from comment #44)
> @@ +488,5 @@
> > +      throw new Error("Socket not open.");
> > +    }
> > +    if (this._ssl == 'ssl') {
> > +      // Already SSL
> > +      return;
> 
> I have not read the spec, but shouldn't we throw?

Under current discussion thread (https://github.com/sysapps/raw-sockets/issues/10). The error is expected to be reported in the Promise upgradeToSecure() returns.

Comment 46

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Per talked with Jonas on IRC, I will change the method name to upgradeToSecure(), which is proposed in the discussion thread, and leave the return type void in this first implementation.

Comment 47

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Implement startTLS in MozTCPSocket v.3

Address comment 44 and comment 46. Jonas, would you take a look at the interface change?

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 797086 [details] [diff] [review]
Patch: Implement startTLS in MozTCPSocket v.3

Review of attachment 797086 [details] [diff] [review]:
-----------------------------------------------------------------

Andrew, you have not addressed comment 43.

Comment 49

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #48)
> Comment on attachment 797086 [details] [diff] [review]
> Patch: Implement startTLS in MozTCPSocket v.3
> 
> Review of attachment 797086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Andrew, you have not addressed comment 43.

(By means of what is your plan, at least).

Comment 50

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #49)
> > Andrew, you have not addressed comment 43.
> 
> (By means of what is your plan, at least).

Did you mean Patrick?  I don't have much knowledge of the network stack in terms of what's exposed to JS for crypto layering; I've always asked for 'starttls' crypto when opening a socket in XPCOM land.

I do think that the security argument can be made that you should know when opening a connection whether you are going to try and perform a TLS upgrade since "only upgrade to TLS if the insecure connection which could just be an attacker says that TLS-upgrade is available" is not safe.  Which is an argument for requiring useSecureTransport to be set to 'starttls' in that case, or something like it.  The spec's TCPOptions dictionary seems like it will need to be updated to something like that in the future.

Comment 51

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Honza Bambas (:mayhemer) from comment #49)
> (In reply to Honza Bambas (:mayhemer) from comment #48)
> > Comment on attachment 797086 [details] [diff] [review]
> 
> (By means of what is your plan, at least).

I will file a follow up to provide an interface to create a non-secure TCPSocket. We can do this through changing |useSecureTransport| to a string or using an additional boolean in TCPOptions as comment 26 mentioned.

Comment 52

[Honza Bambas (:mayhemer)]

(In reply to Andrew Sutherland (:asuth) from comment #50)
> Did you mean Patrick?  I don't have much knowledge of the network stack in

Ah!  Yes, sorry :)

Comment 53

[Honza Bambas (:mayhemer)]

(In reply to Patrick Wang [:kk1fff] from comment #51)
> I will file a follow up to provide an interface to create a non-secure
> TCPSocket. We can do this through changing |useSecureTransport| to a string
> or using an additional boolean in TCPOptions as comment 26 mentioned.

Thanks!  And again, sorry for the first confusion about this.

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 797086 [details] [diff] [review]
Patch: Implement startTLS in MozTCPSocket v.3

Review of attachment 797086 [details] [diff] [review]:
-----------------------------------------------------------------

What happens if you call send() right after calling upgradeToSecure(), but before the actual upgrade has had time to happen? Does it result in an exception? Does the data get sent once the upgrade has happened? What happens if the upgrade fails, does the data get sent unsecurely?

I sort of like what I think the spec proposes, which is that when you call upgradeToSecure() we change readystate to "upgrading". If you call send() during that state an exception is thrown. Once the upgrade is successful or fails, the readystate goes back to "open". We should also somewhere on the API expose if the current connection is secure or not. Either by making secure connections use "opensecure" as readystate, or by having a separate .secure property or some such.

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Alternatively, it might be easier to handle a failed upgrade by simply disconnecting the connection. That way no data is inadvertently sent over an insecure connection. If we do that we might not need the "upgrading" readystate.

Comment 56

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #54)
> Comment on attachment 797086 [details] [diff] [review]
> Patch: Implement startTLS in MozTCPSocket v.3
> 
> Review of attachment 797086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What happens if you call send() right after calling upgradeToSecure(), but
> before the actual upgrade has had time to happen? 

The data are written to the secure socket that starts the negotiation.  This is how PSM/NSS secure socket is written.

To start the update you actually need to write data to the socket.

> Does it result in an
> exception? 

Obviously no.

> Does the data get sent once the upgrade has happened? 

Yes.

> What
> happens if the upgrade fails, does the data get sent unsecurely?

No.  If the secure connection fails from whatever reason (often because of cert issues or cipher negotiation issues) you get an error and the socket closes.

> 
> I sort of like what I think the spec proposes, which is that when you call
> upgradeToSecure() we change readystate to "upgrading". If you call send()
> during that state an exception is thrown. Once the upgrade is successful or
> fails, the readystate goes back to "open". 

Is there a notification specified when the socket is again at "open" state?

It would mean we drive the ssl negotiation by pushing 0 bytes to the socket while getting WOULD_BLOCK.  When we get OK, the update is done.  When we ERROR update (ssl connection) has failed.

I'm not sure this is fully doable in JS, but I may check.  If not, it might just complicate the TCPSocket code.

> We should also somewhere on the
> API expose if the current connection is secure or not. Either by making
> secure connections use "opensecure" as readystate, or by having a separate
> .secure property or some such.

Agree.

(In reply to Jonas Sicking (:sicking) from comment #55)
> Alternatively, it might be easier to handle a failed upgrade by simply
> disconnecting the connection. 

That is the behavior.


> That way no data is inadvertently sent over an
> insecure connection. If we do that we might not need the "upgrading"
> readystate.

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Current approach sounds good then. Would be good to expose if we are on a secure connection or not through a property separate from readyState. But we might already do that? Either way that can be a separate bug/patch

Comment 58

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jonas Sicking (:sicking) from comment #57)
> Current approach sounds good then. Would be good to expose if we are on a
> secure connection or not through a property separate from readyState. But we
> might already do that? Either way that can be a separate bug/patch

Current readyState doesn't contain the secure state, I will file a bug for reporting the state of secure connection.

Comment 59

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Implement startTLS in MozTCPSocket v.checkin

Comment 60

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/1000c23df772

Comment 61

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1000c23df772

Comment 62

[Donovan Preston [:fzzzy]]

Woohoo! Awesome that this is fixed! Thanks Patrick and everyone who helped!

Comment 63

[Andrew Sutherland [:asuth] (he/him)]

This works in single-process SSL operation but regressed multi-process SSL operation.  This caused bug 912147.

The problem is in TCPSocket.prototype.open:

    if (this._inChild) {
      that._socketBridge = Cc["@mozilla.org/tcp-socket-child;1"]
                             .createInstance(Ci.nsITCPSocketChild);
      that._socketBridge.open(that, host, port, !!that._ssl,
                              that._binaryType, this.useWin, this.useWin || this);
      return that;
    }

Since that._ssl will be set to 'ssl', and !!'ssl' becomes `true`, then in the child, _createTransport will receive an sslMode of `true`, which is not `=== 'ssl'`, so it will create the socket in startTLS mode.


It's unclear to me if this patch should be backed out and re-landed or a new follow-up bug filed.  It is pretty clear that we'll need to get SSL tests in the tree for this (bug 466524).  I'm speculatively reopening.

Comment 64

[Andrew Sutherland [:asuth] (he/him)]

Sorry, jet-lag problem analysis fail.

The problem looks to be in TCPSocketParentIntermediary.  

That looks like:

  open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType) {
    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
    let socket = baseSocket.open(aHost, aPort, {useSSL: aUseSSL, binaryType: aBinaryType});

The problem there is that useSSL should have been changed to useSecureTransport.  Probably.  Jet-lag, you know?  But definitely we are not establishing an SSL connection at the other end.  Definitely.

Comment 65

[Andrew Sutherland [:asuth] (he/him)]

Attachment: [interdiff between v.checkin and atchm 801231] fix-tcpsocket-v1.diff

Here's the patch that fixes things for me.

Comment 66

[Andrew Sutherland [:asuth] (he/him)]

Backed out per advice from Fabrice (and since this is a weekend so a review of my suggested fix might not be fast enough for us to have working Firefox OS builds for this work week):
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf2b6cf8d73

Comment 67

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 801213 [details] [diff] [review]
[interdiff between v.checkin and atchm 801231] fix-tcpsocket-v1.diff

Review of attachment 801213 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I've missed that when changing the option name. Thanks for catching it, Andrew!

Comment 68

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Implement startTLS in MozTCPSocket

Having grep-ed for all 'useSSL' in tcpsocket's implementation, the code I actually modified is only the line that Andrew had mentioned. I have tested SSL connections in OOP and inproc, they both works.

Comment 69

[Honza Bambas (:mayhemer)]

Comment on attachment 801231 [details] [diff] [review]
Patch: Implement startTLS in MozTCPSocket

If this fixes the problem, then go ahead (no time and resources to test this my self).

This is just a prove that tests must go in WITH the patch and NOT AFTER.

Comment 70

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

re-land: https://hg.mozilla.org/integration/b2g-inbound/rev/9edc229b7d09

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9edc229b7d09

Comment 72

[Andrew Sutherland [:asuth] (he/him)]

Bug 914520 is a leo+'d blocker to support German ISPs that only support startTLS SMTP upgrades on port 587 and do not support (initial) SSL on port 465.  This bug should be marked leo+ for clarity.

I have prepared an uplift patch and am testing it now; the existing patch does not apply cleanly because mozilla-b2g18 does not have the TCP server socket changes.

The changes of the uplift patch from the mozilla-central landing are:
- New UUIDs have been generated so anyone using the UUIDs directly would not be confused.
- Sticking with useSSL instead of useSecureTransport.  My rationale is that since this is such a late-landing change for v1.1, the de facto interface should not be changed.

Comment 73

[Andrew Sutherland [:asuth] (he/him)]

Attachment: b2g18 proposed uplift patch v1

xpcshell tests for a b2g-desktop build work.  Having trouble getting gaia to work in b2g at all.  Will spin an unagi build and verify on the unagi.

Comment 74

[Andrew Sutherland [:asuth] (he/him)]

I performed testing on an unagi device using the patched version of the e-mail app (in addition to the xpcshell-tests).

Landed (on the authority of blocking-b2g=leo+ bug 914520):
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bebaca30ea30

Comment 75

[Dylan Oliver [:doliver]]

triage: leo+, raised as high priority during work week (already landed)

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bebaca30ea30

Comment 77

[Jeremie Patonnier :Jeremie]

Documented here:
https://developer.mozilla.org/en-US/docs/Web/API/TCPSocket.upgradeToSecure