861196 - mozTCPSocket needs to translate SSL and certificate error codes to something that can be exposed to content. [Error: Permission denied for ORIGIN to create wrapper for object of class UnnamedClass']

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

When SSL errors happen, (moz)TCPSocket currently just exposes the nsISSLStatus exception as the 'data' payload of its error object.  However, XPConnect won't let a wrapper be created for the nsISSLStatus instance.

This impacts the gaia e-mail app's ability to detect SSL/security problems and properly report them to the user.  The functionality did work at some point, so presumably either XPConnect or the nsScriptSecurityManager was less strict long ago or we only ever tested with chrome privileges.

Since nsISSLStatus is not intended to be a public/content API, it does not make sense to try and expose it.  Right now the e-mail app lacks a nuanced understanding of different SSL errors, so it would be fine if mozTCPSocket only returned a single boolean indication of there being a security problem, although it would be nice for it to eventually be able to report more specific failures and do so without losing that boolean indication.

Specific families of errors that would be useful to be able to indicate:
- certificate would be valid if the date weren't wrong (so we can say to double-check the date on the device and tell the user what we think it is)
- certificate is self-signed (and therefore no good without an exception)
- certificate is signed by a CA we don't recognize (and therefore no good)
- domain mismatch stuff with indication of the domains the cert is valid for; primarily of interest for domain probing where it could hint to us to try another valid permutation we already know about.

This needs to be tef+ because the bug it blocks is tef+.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Andrew Sutherland (:asuth) from comment #0)
> Since nsISSLStatus is not intended to be a public/content API, it does not
> make sense to try and expose it.  Right now the e-mail app lacks a nuanced
> understanding of different SSL errors, so it would be fine if mozTCPSocket
> only returned a single boolean indication of there being a security problem,
> although it would be nice for it to eventually be able to report more
> specific failures and do so without losing that boolean indication.

nsISSLStatus is ONLY intended to store state for the UI indicator in the address bar of Firefox. The information in it must not be used to make security decisions.

When you make a networking call (open a connection, send data, receive data) using the Necko APIs, you get one of various error codes when SSL errors happen. Look at nsDocShell::DisplayLoadError to see how this should be done.

> Specific families of errors that would be useful to be able to indicate:
> - certificate would be valid if the date weren't wrong (so we can say to
> double-check the date on the device and tell the user what we think it is)
> - certificate is self-signed (and therefore no good without an exception)
> - certificate is signed by a CA we don't recognize (and therefore no good)

We already do this. See nsDocShell::DisplayLoadError and TransportSecurityInfo::formatErrorMessage.

> - domain mismatch stuff with indication of the domains the cert is valid
> for; primarily of interest for domain probing where it could hint to us to
> try another valid permutation we already know about.

It should be possible to recognize domain mismatch errors. This libssl error code is SSL_ERROR_BAD_CERT_DOMAIN and you should be able to pull that out of the nsresult. See nsDocShell::DisplayLoadError for how to convert between nsresult and PRErrorCode.

Comment 2

[Johnny Stenback (:jst)]

Andrew, does Brian's comment here give you enough to go on to fix this? If not, let me know and we'll find you more help if needed.

Comment 3

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

Brian's comment is super helpful; as long as it's cool if I direct the review at Brian, I think all our ducks in a row.  If someone else should review, please let me know so I can get that right on the first try.  Thanks!

Comment 4

[Honza Bambas (:mayhemer)]

I cat take the review if needed.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Andrew Sutherland (:asuth) from comment #3)
> Brian's comment is super helpful; as long as it's cool if I direct the
> review at Brian, I think all our ducks in a row.  If someone else should
> review, please let me know so I can get that right on the first try.  Thanks!

(In reply to Honza Bambas (:mayhemer) from comment #4)
> I cat take the review if needed.

I am also happy to review.

Comment 6

[Chris Lee [:clee]]

It's not clear to me exactly what the use impact is if we don't resolve this issue?

Also, looking to get the latest status here.  Thanks.

Comment 7

[Dylan Oliver [:doliver]]

Status: working through some difficulties related to b2g18 but should have a patch up for review later today.

Comment 8

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

Attachment: map and report errors via DOMError, add empty TCPError stub for future enhancement v1

This just changes TCPSocket to send its errors using a DOMError.  I tried ever so hard to implement TCPError so it would work on b2g18 by aping DOMError, but quickly ran into problems that begged the question of why I was trying to do that.  I think the ability to add JS-implemented bindings on mozilla-central makes it (more) practical, but maybe DOMError and regexes are okay?  Either way, I think proper TCPError would be a future work type of thing given that this bug is tef, not trunk.

I go pretty all out in this patch in terms of mapping many of the known error types across since I think it would be useful for a UI that wanted to provide more information about failures.

Comment 9

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

Attachment: map and report errors via DOMError, add empty TCPError stub for future enhancement v2 (test fix)

(make our limited unit tests still work...)

Comment 10

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

Comment on attachment 742049 [details] [diff] [review]
map and report errors via DOMError, add empty TCPError stub for future enhancement v2 (test fix)

Josh, your thoughts on this error strategy?  (Especially as you are probably the person to make TCPError happen if it's gonna happen.)

Comment 11

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

bsmith had a busy Friday, but conveyed over IRC that we will probably want to express the existence of a class of override-able errors such as strict transport security errors (which exist on a per-host basis and are port/protocol agnostic.)

The right course of action may be for me to split the patch into a v1.0.1/v1-train version that is basically the current patch, and a mozilla-central version.  For mozilla-central I can make TCPError be a real bona fide custom interface type with extra fields orthogonal to the 'name' attribute, such as the type attribute I define in the IDL.

Having said all that, I'm not really clear on how STS would affect TCPSocket, at least as it relates to IMAP.  Currently, we always want to establish a full SSL connection in all non-unittest cases.  Once TCPSocket supports startTLS we would want to support establishing a cleartext connection and upgrading that, but we already indicate our startTLS desires when we request the socket, so STS shouldn't really come into play there either.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 742049 [details] [diff] [review]
map and report errors via DOMError, add empty TCPError stub for future enhancement v2 (test fix)

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

I reviewed the error decoding logic, and it seems reasonable to me. Please have a DOM peer review the patch.

Note that the test doesn't test any of the SSL/certificate related things. Do we have any tests for TCPSocket that use SSL certificates? We probably should.

It would be best to put this logic somewhere where TCPSocket, XHR, and nsDocShell.cpp can all make use of it. That can be done in a follow-up bug. (I am a little surprised that we're doing this for TCPSocket but not XHR.)

Note that in the desktop browser, we do not notify users of certificate/SSL/network failures that occur with WebSockets, XHR, or other subrequests like <img> or even <iframe>; they just silently fail like a 404. In particular, we don't have any UI for certificate error overrides for subrequests, and we specifically disabled the certificate error override UI for <iframe> because of clickjacking concerns. Also note that certificate overrides are done on a per-profile basis, not a per-app basis. That means that if you allow an app to control certificate error overrides, then they can compromise other apps, and even gecko-internal security-critical components (IIRC, not automatic updates, but similar things).

Also note that the information returned is not sufficient to determine whether or not to show a certificate error override UI. In particular, because the HSTS state isn't returned, there's no way to know whether or not it is OK to show the cert error override UI.

Note that we are regularly expanding and changing the error codes returned by PSM (and thus through Necko), especially for SSL- and certificate-related errors. It is likely that the switch statements in this code will become stale over time. If it is important to have specific error names returned for specific scenerios, then we should have automated tests for those scenerios.

::: dom/network/src/TCPSocket.js
@@ +573,5 @@
> +              errName = 'SecurityUntrustedCertificateError';
> +              break;
> +            case 36: // SEC_ERROR_CA_CERT_INVALID, sec(36)
> +              errName = 'SecurityIssuerCertificateInvalidError';
> +              break;

I understand, from discussion in IRC, that the goal here is to use names that would be reasonable for other implementations to use, if/when TCPSocket goes through standarization.

If that is the case, then I recommend collapsing case 13, case 20, case 21, and case 36 into a single "SecurityUntrustedCertificateIssuerError" error. I cannot guarantee that Firefox will be able to discern these as distinct cases going forward; in fact, I can almost definitely guarantee that we won't be able to discern these as distinct cases in the near future.

Anyway, the idea of us being in a rush to fix this bug is counter to the idea that we need to make good choices here that are reasonable for other browsers to return. So, I think we should just be resigned to the idea that we will have to change these error names in the future as part of the standardization of the feature.

@@ +600,5 @@
> +            case 8: // SSL_ERROR_UNSUPPORTED_CERTIFICATE_TYPE, ssl(8)
> +              errName = 'SecurityUnsupportedCertificateTypeError';
> +              break;
> +            case 9: // SSL_ERROR_UNSUPPORTED_VERSION, ssl(9)
> +              errName = 'SecurityUnsipportedVersionError';

Unsupported is spelled wrong. Also, there are various versions of various things involved, so I think we should indicate that this is specifically the *TLS* version that is the problem.

I think this should be "SecurityUnsupportedTLSVersionError";

@@ +603,5 @@
> +            case 9: // SSL_ERROR_UNSUPPORTED_VERSION, ssl(9)
> +              errName = 'SecurityUnsipportedVersionError';
> +              break;
> +            case 12: // SSL_ERROR_BAD_CERT_DOMAIN, ssl(12)
> +              errName = 'SecurityDomainMismatchError';

I suggest "SecurityCertificateDomainMismatchError"

@@ +750,2 @@
>      return true;
>    },

You do not need to implement nsIBadCertListener2 at all. The default behavior is now to silently fail the connection, which is what you are asking for here.

Comment 13

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

(In reply to Brian Smith (:bsmith) from comment #12)
> Note that the test doesn't test any of the SSL/certificate related things.
> Do we have any tests for TCPSocket that use SSL certificates? We probably
> should.

No SSL test.  Yes, we should.  I have filed bug 866931.

> It would be best to put this logic somewhere where TCPSocket, XHR, and
> nsDocShell.cpp can all make use of it. That can be done in a follow-up bug.
> (I am a little surprised that we're doing this for TCPSocket but not XHR.)

I have filed bug 866924 to this end and am establishing a see-also relationship.

I have integrated your suggestions and will put up a revised patch for a DOM peer to review shortly.  I understand from his bugzilla name that :jdm is on vacation until May 7th, so I will need someone else to step up for review.  I am tentatively going to point it at :jst since he's a DOM owner and has already involved himself on this bug :)

Comment 14

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

Attachment: map and report errors via DOMError, add empty TCPError stub for future enhancement v3 (review changes, IPC fixes)

This has the review fixes, plus I had apparently broken the IPC case.  Both the IPC and non-IPC xpcshell tests now pass happily.

Comment 15

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

Comment on attachment 743384 [details] [diff] [review]
map and report errors via DOMError, add empty TCPError stub for future enhancement v3 (review changes, IPC fixes)

:jst, please review or delegate the review to someone since :jdm is on vacation.  Thanks! (Carried r=bsmith for the error semantics bits.)

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 743384 [details] [diff] [review]
map and report errors via DOMError, add empty TCPError stub for future enhancement v3 (review changes, IPC fixes)

Looks good, with a couple of comments, and a few take it or leave it nits. r=jst

 [scriptable, uuid(fff9ed4a-5e94-4fc0-84e4-7f4d35d0a26c)]
 interface nsITCPSocketInternal : nsISupports {
-  // Trigger the callback for |type| and provide an Error() object with the given data
-  void callListenerError(in DOMString type, in DOMString message, in DOMString filename,
-                         in uint32_t lineNumber, in uint32_t columnNumber);
+  // Trigger the callback for |type| and provide a DOMError() object with the given data
+  void callListenerError(in DOMString type, in DOMString name);

Since the signature changes here, bump the interface uuid please.
 
- In TCPSocket.js:

+function createTCPError(aErrorName, aErrorType) {
+  let error = Cc["@mozilla.org/dom-error;1"]
+                .createInstance(Ci.nsIDOMDOMError);
+  error.wrappedJSObject.init(aErrorName);
+LOG(':::: creating tcp error: ' + aErrorName);

Fix indentation.

- In TCPSocket.prototype.callListenerError:

+    // XXX we're not really using TCPError at this time, so there's only a name
+    // attribute to pass.
+LOG('callListenerError', type, name);

Fix indentation

- In _maybeReportErrorAndCloseIfOpen:

+        // NSS_SEC errors (happen below the base value because of negative vals)
+        if ((status&0xffff) < Math.abs(nsINSSErrorsService.NSS_SEC_ERROR_BASE)){

Add spaces around the '&' to match style of surrounding code.

+          // The bases are actually negative, so in our positive numeric space, we
+          // need to subtract the base off our value.
+          let nssErr = Math.abs(nsINSSErrorsService.NSS_SEC_ERROR_BASE) -
+                         (status&0xffff);

ditto.

+          let sslErr = Math.abs(nsINSSErrorsService.NSS_SSL_ERROR_BASE) -
+                         (status&0xffff);

... and here too.

- In dom/webidl/TCPError.webidl:

+/*
+ * NOTE! This file does not currently do anything on mozilla-b2g18; it wants a
+ * JS implementation on mozilla-central where it can get a JS implementation.
+ */

I can go either way here, but would it not be better to include this file when the work gets done to actually make use of it on m-c, rather than just adding the file now.

And finally, sorry for the delay, vacation got in the way...

Comment 17

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

Attachment: v4 patch as landed on mozilla-inbound

landed on mozilla-inbound:
https://tbpl.mozilla.org/?tree=Try&rev=36edcbc1e65a

Comment 18

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/987235c0ba5e

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/514f49e853fa

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4036016ee651

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am clarifying the summary since nsISSLStatus isn't involved in the solution.