Closed Bug 1322748 Opened 8 years ago Closed 6 years ago

Web extensions: SSL (TLS) status API request

Categories

(WebExtensions :: Request Handling, enhancement, P3)

Product:
WebExtensions
Component:
Request Handling
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed
Blocking Flags:
webextensions ?

People

(Reporter: 5i13ghzt462u, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][triaged])

Attachments

(4 files, 5 obsolete files)

example of extended webRequest object
32.36 KB, text/plain
Details
Cert override XPCOM API 2018-01-29.diff
31.00 KB, patch
franziskus
: feedback-
Details | Diff | Splinter Review
Bug 1322748 add ability to get registered channelwrappers,
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1322748 add securityInfo to webRequest listeners,
59 bytes, text/x-review-board-request
rpl
: review+
Details 
TL;DR: Web extensions should get the ability to access ssl/tls related data, which is provided for other add-ons via the nsISSLStatus Interface. Also extend the API.

Many add-ons for advanced and/or privacy-conscious users[1] somehow integrate into the ssl/tls API of Firefox. They do useful things such as blocking certs, showing the fingerprint and making it more visible for the user or showing details such as ciphers etc.

Personally I am particular interested in the add-on SSleuth[2] by Sibi Antony.
This add-on makes use of the SSLSTatus API[3] according to a statement of the developer[4].
Based on the fact that web extensions "are the future"[5] we need to have this API in Firefox for web extensions.

And when you are at it, please also expose the used elliptical curves (original issue: [6]), so add-ons can display/evaluate/… this information too.
And I mean of course both ECs used in the certificate and in the key exchange.

[1] https://addons.mozilla.org/en-US/firefox/collections/ivanristic/ssl/
[2] https://github.com/sibiantony/ssleuth
[3] http://doxygen.db48x.net/mozilla-full/html/da/db2/interfacensISSLStatus.html
[4] https://github.com/sibiantony/ssleuth/issues/78#issuecomment-265587468
[5] https://blog.mozilla.org/addons/2016/11/23/add-ons-in-2017/
[6] https://github.com/sibiantony/ssleuth/issues/45
Priority: -- → P5
Whiteboard: design-decision-needed
Depends on: 1312195
It would be great to have a concrete list of the specific use-cases and overall aims of add-ons that would use such an API. That way we can design an API that provides something meaningful.
I will give an example from the my extension. I think the bug author is interested in the same.

https://addons.mozilla.org/firefox/addon/http-useragent-cleaner/

1. (Now the extension does not this). To assess the restriction of the address to the DNS resolve. Bug 1251002 . Synchronously.
2. To assess whether loading via a secure protocol (wss or https). Synchronously.
3. The assess is carried out synchronously. If the Protocol is not https or wss is either blocking or http redirect. Synchronously.
4. (Now the extension does not this). Set possible TLS version, cipher suite and certificates to connect. In real time the extension uses a artificial created HPKP for the valid certificates filtration. Synchronously.
5. For each request, not just the main request at the tab, to assess the strength of the encryption.
Synchronously. But, if the resistance of the connection is insufficient, the extension must doing to prevent the establishment of such a connection. Prior to transfer a cookies to the server.
That is, it must be done, in part, between onBeforeSendHeaders and onHeadersReceived.
5.1. To evaluate the connection for the validity of the TLS version and the ciphers ( bug 1312195 ). Synchronously
5.2. To evaluate the connection for the certificates (a certificates life time, an encryption strength). Synchronously
5.3. To evaluate the certificates trust. That is implemented at the extenssion level this the extension public key pinning. Synchronously
5.4. Terminate connection, if the connection is not trust.
5.4. To evaluate the durability of HPKP. Asynchronously.

6. Asynchronously. Display the connection rating for a user.
I suppose I should have thought through comment 2 a bit more before posting. What would really be best would be to discuss the needs and what sort of API(s) would address them on a public mailing list. Then, we can come back to this bug and implement whatever results from that.
To be discussed at January 24 WebExtensions Triage mtg. 

Agenda: https://docs.google.com/document/d/1add-6FL8mzksvzbyB83HUmEkVmKERd-nt740AYr-4PE/edit#
My AddOn
https://addons.mozilla.org/en-US/firefox/addon/pubkeypin/
depends (besides others) on nsISSLStatus, too.
Flags: needinfo?(g.maone)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> I suppose I should have thought through comment 2 a bit more before posting.
> What would really be best would be to discuss the needs and what sort of
> API(s) would address them on a public mailing list. Then, we can come back
> to this bug and implement whatever results from that.

Where's the best place to start this discussion? dev-platform, maybe?
Flags: needinfo?(g.maone) → needinfo?(dkeeler)
dev-platform or firefox-dev, maybe. There might also be a web extensions-specific mailing list. If there's a more appropriate mailing list, I'm sure someone will let you know.
Flags: needinfo?(dkeeler)
You might also cross-post to dev-security and/or dev-security-policy.
Just cross-posted on dev-platform, dev-security and dev-addons:

"In https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c4 David Keeler suggested to bring this issue up in a public forum in order to decide how and how much to expose of the nsISSLStatus interface and its dependencies to WebExtensions, considering that many Firefox add-ons use it either to provide enhanced security UIs  or to enforce stricter security policies tailored on specific use cases.

Additionally, exposing also ECDHE/DHE parameters has been asked for the same reasons ( https://bugzilla.mozilla.org/show_bug.cgi?id=1312195 ).

The most natural place to provide WebExtensions with this data is, IMHO, in webRequest.onBeforeSendHeaders or in an ad-hoc event (onConnect?) which needs anyway to be called before any HTTPS payload is actually exchanged on the wire.

Personally (i.e. for the purposes of the Tails Download and Verify Extension which I maintain) I would be fine with a thin wrapper over nsISSLStatus and nsIX509Cert, but platform developers, security guys and other add-ons authors likely have different but hopefully reconcilable views on this matter, therefore I'm cross-posting to dev-platform, dev-security and dev-addons hoping for the best outcome."
Status: UNCONFIRMED → NEW
Ever confirmed: true
I would like to see extensions given access to all the fields of each certificate in the tab's current certificate chain, both as some sort of documented data structure and as a raw binary blob. That would generally cover all the use cases of "do I like/trust this certificate, or its root", and the use cases of "I've detected a cert problem here; let me send the full chain off to some logging server". I'm not sure we could say with certainty that a particular subset of the fields would be "enough" - people are always finding new ways they want to evaluate certificates.

My particular use case is the expiry (a.k.a. notAfter) date; my extension, Expiry Canary, warns you if the cert for the site you are accessing is about to expire.

A different set of extensions might want info about the negotiated parameters of the SSL connection (cipher suite, curves etc.) but I'd say that's a different request.

Gerv
> A different set of extensions might want info about the negotiated parameters of the SSL connection (cipher suite, curves etc.) but I'd say that's a different request.


No, not at all. IMHO this is most important as it is the thing I (as a user, not as a dev here) would use most. I've already linked to SSLLeuth and as you can see in this screenshot https://camo.githubusercontent.com/9bec1ce009642f1e8a0ebe2a9a5f2ed46c77e8bf/68747470733a2f2f6164646f6e732e63646e2e6d6f7a696c6c612e6e65742f757365722d6d656469612f70726576696577732f66756c6c2f3137362f3137363936332e706e67 it uses these features quite much.
So all the statistics about the connection should also be exposed.
"A different request" is not the same thing as "not important".

Gerv
Well... I created this issue with exactly these features in mind, so I'd say it certainly belongs to this request. However of course it does not matter to me, in how requests we'll split this as long as it is implemented in the end. :)
> The most natural place to provide WebExtensions with this data is, IMHO, in webRequest.onBeforeSendHeaders or in an ad-hoc event (onConnect?) which needs anyway to be called before any HTTPS payload is actually exchanged on the wire.

1. I think this place (onBeforeSendHeaders ) is not suitable.
I assume this handler is called before define a specific tcp connection. It is even possible that this connection is already established (However, I'm not sure.).



2. Best of all, if it will be added 3 events.
OnPreTLSConnection
OnTLSConnection
OnHPKPProcess

I write most of what I need. I understand that the developers will make less.

-----
OnPreTLSConnection:
Must contain the IP address, domain name. after onBeforeSendHeaders. Is called only before establishing a new TLS connection.
Perfect, allows I to enable (switch by extension) the specific cipher suites that are permitted for this connection and TLS version and allow to change an expected Public key pinned certificates.
Allows to cancel the connection before the first TCP request.

-----
OnTLSConnection
After the establishment of the TLS connection but before sending the http headers to server.
Allows to cancel the connection before the first TCP request.

Indicative list of fields.
Ciphersuite
Cipher length of key exchange without (! Now prepared different, I think) no change in equivalents between RSA and EC. That is, for RSA it is, for expample, 4096, and for EC - 256.
Symmetric cipher key length.
TLS version

--
Full certificates tree with informaton for all certificates:


RSA/ECDSA key length and algorithm (RSA/ECDSA)
Status of domain match, date expired and trust (true/false)
Status of ExtendedValidation and CertificateTransparency
Status of the OCSP check (preferably)

sha256SubjectPublicKeyInfoDigest
SHA1 and SHA2-256 of certificate
commonName  windowTitle  organization  serialNumber 
notAfter
notBefore

-----
OnHPKPProcess
After receive http headers from server, but before process of HPKP.
Allows to change or set information about HPKP for this site.
> OnTLSConnection
> Allows to cancel the connection before the first TCP request.

error. Before send http headers
Please let me note one more related extension here:

Cipherfox allows users to inspect the current certificate chain and cipher suite. Currently it seems impossible to implement this via WebExtensions, and there's no alternative.

Links to the addon:
https://github.com/gavinhungry/cipherfox
https://addons.mozilla.org/en-US/firefox/addon/cipherfox/

As a side note: It would be a nice feature, if add-ons could add some ui items to the site identification dialog, besides accessing the SSL/TLS parameters and the certificate chain. See the current Chipherfox UI.

Another add-on worth considering is Certificate Patrol:
https://addons.mozilla.org/en-US/firefox/addon/certificate-patrol/
Blocks: webextensions-additional-apis
Severity: normal → enhancement
There wasn't any objection to Giorgio's email, just questions about how we should implement this. For the purpose of our team I think we are all on board that this should be implemented. The next part is figuring out what to implement, what the API should like and possible finding someone from security to help us do this.

For that reason marking as approved.
Whiteboard: design-decision-needed → [design-decision-approved][triaged]
Hi all! This will be discussed at the April 17 WebExtensions Triage meeting. 

Agenda: https://docs.google.com/document/d/1zKqhDqXoH9vi88q4DGtOHm1hsCF8ZwLNvCrrCE5htbc/edit#

Call-in info: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Details_.26_How_to_Join

Rugk, would you be able to join this meeting?
Sorry, that was my fault, we've already discussed this. Until we get onto the implementation details I don't think there's anything to discuss in the meeting, taking off the agenda.
When designing the new "security" API I'd really like to see the inclusion of (at least non-blocking read) access to the complete available data (raw and pre-parsed certificates, ciphers, ...) of system connections, too.
It shouldn't be hidden from the user what the browser (or other extensions) is doing in the background on the network, as currently with WebRequests.


If this new API doesn't become integrated into the WebRequest API directly, they should be connected by a unique ID.

For example, each connection with its security settings gets a unique "securityID".
As the channel must be encrypted before any "real" data exchange happens and multiple WebRequests will propably use the same network connection, each WebRequest should contain a reference to the used "securityID".

Multiple "requestID"s will share the same "securityID".
WebRequests might be blocked from within the WebRequest API, because the extension discovered something "bad", "suspicious" or similar in the security of the connected "securityID"s data - for example a "weak" cipher, not pinned public key or a used root certificate from honest Achmed.

Without available security data the "securityID" in the WebRequest might be set to "unencrypted", "cache", "none" (if the extension does not have the "security" permission),...
Hi,

Namecoin is interested in this API as well.  Some background on our use case:

Namecoin websites use the ".bit" TLD and use different certificate trust rules compared to typical websites.  Specifically, the trust root for a Namecoin website is specified in that website's record in the Namecoin blockchain.  This means 2 things:

1. Self-signed certs for a .bit domain, or certs for a .bit domain signed by a CA that Firefox doesn't know of, must be trusted by Firefox if the cert matches the Namecoin blockchain record.
2. Certs for a .bit domain that are signed by a CA that Firefox trusts, must be rejected by Firefox if the cert doesn't match the Namecoin blockchain record.

The Namecoin blockchain would preferably specify a certificate by its public key hash, so a Namecoin TLS WebExtension wouldn't possess a full certificate until the server certificate is received in the TLS handshake.  

For privacy reasons, we don't want any information about what certificates have been checked to stay in Firefox's state after the TLS handshake has finished, so simply adding HPKP pins isn't really a good option here.

It probably makes sense to have separate API permissions for expanding the set of allowed certificates and restricting them.  This is because a malicious extension with the former permission could MITM traffic, while a malicious extension with the latter permission could only block traffic.  It might also make sense to allow the API permissions to specify a domain suffix; there's no reason that a Namecoin extension would need to change how TLS works for any domain that doesn't end in .bit.

It is particularly important for the extension to be able to block a connection before any private data is sent (e.g. HTTP cookies or TLS client certificates).  That means that the extension should be consulted before the TLS handshake proceeds beyond the server certificate validation step.  It would also be useful for the extension to be notified as soon as it is known what domain name will be connected to for a given TLS connection; this is because looking up a Namecoin record can take some time, and it will be faster if that process can begin before the server certificate validation step of the TLS handshake is reached.

A similar use case to Namecoin's would be DANE / TLSA validation (with the difference that DANE / TLSA would apply to all domain names rather than just Namecoin's .bit domains).

It would also be beneficial for this API to make available information about what proxy settings are in use for the TCP connection used for a TLS handshake.  This is relevant primarily for use cases like Tor Browser, which uses different SOCKS authentication parameters for different TCP connections; we'd want to pass those SOCKS authentication parameters through to whatever external connections our extension makes in the process of verifying a certificate.  (I suppose this could be done as a part of Tor Browser's patchset against Firefox, but it seems like a good idea to address the use case in Firefox's code if possible.)

Cheers!
FWIW, some folks from EFF, Chrome, and myself have been working through a pretty narrow proposal to add more TLS metadata to the webRequest API here: https://github.com/EFForg/webrequest-tlsinfo-api/blob/master/proposal.md with some predecsesor work on Chrome here: https://codereview.chromium.org/2156763003/. I think everyone would love it if what gets implemented was uniform between Chrome and us!
Thanks Alex, at first scan through that looks pretty good, very happy to see other parties involved. I note that this is P5, that's probably a bit low so bumping up, but isn't really on our radar for Firefox 57.

If anyone is interested in working on this, we can definitely find a mentor and provide the WebExtensions support.
Priority: P5 → P3
I've been working on this from the EFF side and did the initial Chromium implementation that got the ball rolling for Chrome. Once we have a stable proposal (it'd be great if people from Mozilla/Firefox who have strong opinions on it to chime in!) I'll be happy to pick up the implementation work for Firefox as well if somebody doesn't beat me to it.
Probably worth flagging, lest anyone be confused, that I'm not qualified to represent the perspective of the necko/NSS team, my involvement has been as someone who very much wants to consume this API :-)
Reading through the proposal, it seems the security information should only be available after all data is already transmitted (onComplete).

This read-only non-blocking place might be appropriate for system connections, but not for standard WebRequests.
As proposed in comment #10 (onBeforeSendHeaders or onConnect) users should be able to withhold their data, if their security demands are not fullfilled.

For example they might accept a "weaker" cipher while browsing example.com, but only want to send a password to login.bank, if a specific public key is used.

With WebRequests blocking depending on the used url or request type is possible.
Why shouldn't the user (with the help of an extension) be allowed to make the same but finer grained decision based on some security info, he sees as important for his use case?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #23)
> FWIW, some folks from EFF, Chrome, and myself have been working through a
> pretty narrow proposal to add more TLS metadata to the webRequest API here:
> https://github.com/EFForg/webrequest-tlsinfo-api/blob/master/proposal.md

Do we really want every certificate-handling extension implementing their own DER decoder?

Also, the idea that this information is only available after the connection completes rules out a whole class of security addons. Perhaps we need a new event between onBeforeRequest and onBeforeSendHeaders - onSSLCompleted - which fires whether or not the SSL connection completes successfully. This would allow people to terminate the connection if necessary, or display or store information about it.

Gerv
The Chrome team was very concerned about offering a too-broad API, which they'd need to be backwards compatible with forever, making it difficult for them to make larger changes, so the APIs proposed there are very much "the narrowest thing that could actually meet the usecases of the folks asking for it".
I can understand the DER thing then, although I still think it will lead to misery.

Is it really true that no-one who wanted the ability to take over or modify trust decisions "was asking"?

Gerv
I don't want to speak for anyone else, but that wasn't in my use case (I only care about data collection). It's entirely possible that the solution here is that Firefox has a broader set of webExtension APIs for TLS than Chrome does. As a consumer of these APIs, I'd be happiest if the intersection of the functionality both expose used the same APIs though.
The EFF's use case for this API is purely passive data collection and analysis. That said there has been interest in implementing an active API that allows modifying network requests based on TLS data which has prompted considerable discussion. Chromium developers have routinely shot down suggestions for an active API for a variety of code complexity and security related issues (a subset of which are expressed in the tickets linked below). While I think there is some value in designing an API that provides this functionality I suspect that unless it was designed with cooperation from both Firefox and Chromium devs it is unlikely to gain parity among browsers.

From my admittedly limited discussions with other extension developers the majority of interest I've heard of is in a limited TLS inspection API. It seems that landing a limited onComplete version would satisfy _most_ developers needs, be relatively easy to implement + ship, and provide a baseline for developers to think about how they might use the provided information in a safe and constructive manner if it was provided further up the event chain.

Relevant Chromium tickets:
https://bugs.chromium.org/p/chromium/issues/detail?id=598123
https://bugs.chromium.org/p/chromium/issues/detail?id=107793
https://bugs.chromium.org/p/chromium/issues/detail?id=49469
Let me note that for example the Certificate Patrol extension (mentioned above) is actively blocking connections with insecure certs. That will not be possible with the stripped-down API functionality.

From a security point of view, I see very little advantage having the certificate details API without the possibility to block a request based on that. My opinion is that if a request is sent to a rouge server, it's already too late to protect the user.
(In reply to Herczeg Zsolt from comment #33)
> Let me note that for example the Certificate Patrol extension (mentioned
> above) is actively blocking connections with insecure certs. That will not
> be possible with the stripped-down API functionality.
> 
> From a security point of view, I see very little advantage having the
> certificate details API without the possibility to block a request based on
> that. My opinion is that if a request is sent to a rouge server, it's
> already too late to protect the user.

This is an interesting use case but it seems like a slippery slope. If the past has tiught us anything it's that TLS chain building and validation is a complicated subject with a number of significant pitfalls. While it is entirely possible extension developers could build a system that provided further validation on top of the browsers validation engine it would open up users to both unintentionally buggy and actively malicious implementations as well. That said I think the best argument against this is that browser implementations are currently the best possible examples of validation engines, if there are possible improvements that an extension could make why aren't we just implementing them at the browser level where they can be properly reviewed and maintained so that the entire community gains the benefit they provide.

Without compelling arguments for why this extremely complicated functionality should be provided I'm not sure what the purpose of jumping into the deep end is.
I would certainly vote for an API where blocking is allowed for addons, but bypassing is not. That would mean, that the worst thing what a malicious addon can do is denial of service, which is already possible (for example by the adblock API.) A single hook _after_ a successful browser-based validation, but _before_ sending actual request to the server would be OK I think.

On the use case side, I see tons of situations, which is not useful for the general public, but could be important for smaller groups. These things are usually best addressed by add-ons. A few examples:

 - I'd like to be notified when a certificate on certain pages change, even if the chain is correct. (CertPatrol) That's mostly useful for crypto-nerds only.
 - I might want to hand-pick a subset of certificates for a website, like "user-configured HSTS". That's useful in a typical university environment, where cert changes usually announced beforehand offline.
 - I might want to ship region/organization-specific or just "too-strict" blacklists, which might not be useful for the general public.
 - An add-on might want to let the user know something, and give them a choice cancel, before they disclose their request to the server.

A completely different - but just as important - reason would be experimenting, which is made possible by add-ons. If for example someone would come up with a community/dns/whatever-based certificate validation, it's highly unlikely to build a new browser around that, or that any browser would include an experimental way of certificate verification. It's quiet possible though to build an extension, and experiment with volunteers. In case the solution is proven to be good and useful, it could make it to the browsers.
(In reply to Roland Bracewell Shoemaker from comment #34)
[...]
> engines, if there are possible improvements that an extension could make why
> aren't we just implementing them at the browser level where they can be
> properly reviewed and maintained so that the entire community gains the
> benefit they provide.
[...]

Copy of my response to a similar question on dev-addons@mozilla.org a few month ago:

>>>>

Giving an example for my add-on (other developers will have their own
use cases):

Last time i looked, system requests to versioncheck.addons.mozilla.org
were secured by HPKP. Pinned are the public keys of two root certificate
owners (DigiCert Global Root CA and DigiCert High Assurance EV Root CA).

Therefore, if i'm right, Firefox itself won't warn or abort the
connection if the public key of the intermediate certificate or the
public key of this mozilla domain changes.

If an attacker can compromise DigiCerts root key or
trick/force/persuade/bug the issuing authority (DigiCert SHA2 Secure
Server CA) to sign a certificate for versioncheck.addons.mozilla.org, at
least HPKP won't detect this directly.

If a user of "PubKeyPin" has pinned mozillas own public key for this
domain before, he will be warned.

> is there a
> reason why that shouldn't be a part of security in Firefox itself?

I'm no expert, but I will try to answer with my limited knowledge:

From a security perspective alone, this perhaps should be part of
Firefox itself.

But then mozillas host keys must be pinned directly and i can imagine
the developers were afraid of bricking the update system, because after
burning two of their public keys consecutively, especially browsers not
regularly online can't connect to this domain anymore via https.

This is a typical use case, where Firefox and an add-on can work together:
Make it work out of the box for the majority of users with sane and not
too restrictive defaults, but give interested security aware users the
possibility to apply a higher standard.

<<<<

Supporting Gerv in comment #28 :
As Firefox already has a DER decoder included and as this probably won't change
in the future, developers as me will prefer to take advantage of it instead of
having to invent and/or ship another one.
webextensions: --- → ?
I might be interested in working on the certificate validation aspect of this API (both the expanding and restricting aspects).  I've been looking through the Mozilla source code; it looks to me like expanding the set of permitted certs would involve adding a little bit of code to CertErrorRunnable::CheckCertOverrides [1], and restricting the set of permitted certs would involve adding a little bit of code to AuthCertificate [2].

However, it's unclear to me how I can obtain a WebRequest context from within those functions (it would be useful to provide that context to WebExtensions that use this API).  Is that accessible via nsNSSSocketInfo, possibly?  Am I on the right track here, or should I be looking at a totally different area of the code?

Cheers!

[1] https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/SSLServerCertVerification.cpp#527
[2] https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/SSLServerCertVerification.cpp#1377
To keep the sslinfo part available to my addon when running as a webextension, at least for firefox installations under my control, I've now extended the webRequest API locally.

Some results:

- With current webRequest events, the security info is only available in onHeadersReceived, onResponseStarted and onCompleted => For blocking requests depending on this information, implementing an onTLSestablished would be necessary.

- Changing the security values on the fly is not possible with this approach.

- Additional values from certificates should be made available from further upstream (e. g. Subject Public Key Info,  Certificate Subject Alt Name, ...) => Certificate parsing and transmitting will be less costly.

- Without some kind of caching for already seen certificate chains, the computation power needed to gather these bits will increase, especially if it has to be done for each request again, although the used tls-channel and security properties are shared with many webrequests. =>  This might support my arguments for implementing the security info with a link to webRequest (securityID and requestId), but in its own domain.


Attached is an example of a webRequest object offered to my webextension while connecting to addons.mozilla.org 

Compared to current webRequest the variables "host", "isprincipal", "query", "scheme", "securitycontext", "sslinfo" and "certificates" were added.
This seems similar to what I was thinking.  Could you post the patch?

Initial thoughts:

- that's a lot of data added to the request object, it might be better to have an API call to retrieve some of it.
- at least the request could be canceled or redirected in onHeadersReceived
Flags: needinfo?(pubkeypin)
Seems like only part of the patch is in comment 40.
I'm new to firefox coding and being overwhelmed with what is needed to create a correctly formated patch, I've attached two simple diffs, based on these files and versions:

https://hg.mozilla.org/mozilla-central/file/f2a208d9d4bc/toolkit/modules/addons/WebRequest.jsm

https://hg.mozilla.org/mozilla-central/file/a149344cbbf6/toolkit/components/extensions/ext-webRequest.js


Locally are more changes implemented, but I tried to limit the patches to the scope of this bug.
Therefore the result does not exactly match what was uploaded in my example.

By far, not everything asked for in this bug is addressed.
Hopefully this can be a starting point to be extended by more code and features.
Flags: needinfo?(pubkeypin)
The EFF proposal also includes elliptical curves now (https://github.com/EFForg/webrequest-tlsinfo-api/pull/18). It is optional (don't know why), but I would really like if it is included in Firefox too, as it is needed for the add-on SSLeuth (https://github.com/sibiantony/ssleuth/issues/45).
So how is the current progress? Will it be included in Firefox 57?
(In reply to rugk from comment #44)
> So how is the current progress? Will it be included in Firefox 57?

It will not.
status-firefox57: --- → wontfix
(In reply to rugk from comment #44)
> So how is the current progress? Will it be included in Firefox 57?

I'm still hacking around with the certificate verification (positive and negative override) part of things, but I don't yet have a patch to share.
By the way, from talking to David Keeler on IRC, it sounds like using the WebRequest API for certificate verification stuff isn't a great approach, because there's not a 1-to-1 correspondence between HTTP requests and TLS handshakes.  As a result, the approach I'm taking is to define a new API, rather than adding stuff to the WebRequest API.

It also looks like the biggest immediate technical barrier is keeping the extra latency introduced by the API as low as possible.
Sorry to say that, but it would be nice if it could be fix, SSleuth was more than usefull (this addon should be a basic feature of firefox in fact)
status-firefox58: --- → ?
Hey, just want to check if this will be included in FF58?
It will not. This is not implemented, and 58 is already in beta so new features won't land there.
+1 for SSleuth (made the account just to support this)

Security and privacy are more important than ever, so this is important stuff.
Hope the nice folks @Mozilla will take it into consideration.

Best regards.
I just want to mention Calomel SSL Validation addon depends on this too:
https://addons.mozilla.org/firefox/addon/calomel-ssl-validation/

From https://calomel.org/firefox_ssl_validation.html:
>IMPORTANT: Development of the Calomel SSL Validation addon has been put on hold. Mozilla is disabling XUL and XPCOM in Firefox which means the addon is no longer able to query the current browser tab for the TLS certificate and cipher information.
>Mozilla is introducing a new framework called WebExtensions which all new addons must use, but WebExtensions are not allowing addons to query the TLS certificate and cipher at this time.


It also seems as if the SSL Observatory feature of HTTPS Everywhere was removed due to missing this API:
https://github.com/EFForg/https-everywhere/issues/9958#issuecomment-302888864
>wrt the observatory, there is no way to do certificate introspection within the WebExtensions API currently

https://github.com/EFForg/https-everywhere/issues/7389#issuecomment-277539629
>Also for anyone that's interested it looks like Mozilla Bug 1322748 - Web extensions: SSL (TLS) status API request is the bug that needs to be fixed to support submissions to the Decentralized SSL Observatory.
Adding a few more Addons to the list of those which have explicitly mentioned the need for this API:

Certificate Patrol:
https://addons.mozilla.org/en-GB/firefox/addon/certificate-patrol/

From http://patrol.psyced.org/:
>As Firefox is in the process of becoming a Google product, we are no longer granted an API that lets us protect you from men in the middle. The new versions of Firefox simply cannot provide Certificate Patrol.

>...since Mozilla is deprecating XUL & XPCOM, it does not make much sense anymore to work on the original add-on. 


DNSSEC/TLSA Validator:
https://addons.mozilla.org/en-GB/firefox/addon/dnssec-validator/

From https://addons.mozilla.org/en-GB/firefox/addon/dnssec-validator/reviews/906134/:
>At the moment the add-on uses a binary core performing the validation which communicates over js-ctypes. Our survey indicates that the WebExtensions interface doesn't provide sufficient functionality. If we decided to rewrite our extension using WebExtensions then it would cripple its current functionality.
>I'm not sure but I think that we are likely to stop developing and supporting the add-on for FF57+.
CipherFox
https://addons.mozilla.org/en-GB/firefox/addon/cipherfox/

https://github.com/gavinhungry/cipherfox/issues/47
>As far as I know, it's not possible to implement Cipherfox via WebExtensions currently, because there's no WebExtensions API to access certificate information. Here's a related bugzilla feature request: https://bug623317.bugzilla.mozilla.org/show_bug.cgi?id=1322748


I disagree with characterising these references as 'metoo', they help describe both the impact and more importantly, the use cases for this API, which should be an important part of its design. Please respect these additions and remove the tags.
Is there any progress?

I'm currently on Firefox 56 because of SSLeuth which, given the Meltdown and Spectre vilnerabilities, it's not the best.

I also disagree with caracterising the bugcon's references as "metoo" as they show the importance of this feature.

Thanks
Somebody "just" needs to propose and/or implement those APIs and get them reviewed. I see there are already patches attached here that might be possible to be updated. It might make sense to work on this bug incrementally instead of adding every conceivable feature, especially if such a feature requires changes to other code. Any of the previous mention add-on authors could potential do that. This bug is already marked design-decision-approved, so any reasonable implementation is likely to be added to Firefox.

At this point we probably have enough examples, thank you.
status-firefox58: ? → ---
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Comment on attachment 8945650 [details]
Bug 1322748 add securityInfo to webRequest listeners

Kris: In addition to general feedback, two items specifically I want to point out.  First, not placing all this into ChannelWrapper.  Second, using a new option "securityInfo" to bypass gathering all this data for the common webrequest use cases.
Attachment #8945650 - Flags: feedback?(kmaglione+bmo)
This looks like a great start, Shane!

It's still missing the subject altname, which contains all domain names the cert intends to cover.
Unfortunately, this isn't exposed in the nsIX509Cert interface :/
See here on how we get this in our certificate viewer: https://searchfox.org/mozilla-central/source/security/manager/ssl/TransportSecurityInfo.cpp#604

I'm CCing Franziskus from crypto engineering, in case he can think of more things that'd be useful.
Any chance that we could just expose that on nsITransportSecurityInfo?

Definitely appreciate feedback on items we should expose.
Flags: needinfo?(fbraun)
The subject altname is something that should probably go into nsIX509Cert if it gets exposed. I think no cert extensions are exposed by nsIX509Cert (other than keyUsage). All extensions should be exposed if this API should be useful for extensions to make trust assumptions.
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #62)
> All extensions should be exposed if this API should
> be useful for extensions to make trust assumptions.

I'm sure this is supposed to read:
All *cert extensions* should be exposed if this API should be useful for *web extensions* to make trust assumptions.


Talking about assumptions: It would be nice if we would let webextensions know exactly about the level of trust we as a browser currently assume.
Flags: needinfo?(fbraun)
This (very WIP) patch adds an XPCOM API that facilitates cert verification overrides by WebExtensions.  It supports both positive overrides (making a cert be accepted that would normally be rejected) and negative overrides (making a cert be rejected that would normally be accepted).

Known issues:

1. Lots of debug printf's littered everywhere.
2. Some of the debug printf's show performance info, which has the side effect of making it (probably) only build on GNU/Linux.
3. Some memory leaks (mainly related to the cert override cache not yet being cleared periodically).

That said, I'm hoping that people can at least provide some feedback on the general approach I'm taking here, even though the code is horribly messy (and certainly not yet in a state where I'd ask people to consider it for merging).  I have a WebExtensions Experiment that exposes this XPCOM API to WebExtensions, which I'll post later.  I will try to spend the next couple weeks cleaning up this patch so that it's a bit less horrifying in terms of messiness.
I should also note that my patch is mainly orthogonal in scope to Shane's patch.  Shane's patch is mostly focused on exposing cert details to extensions who want to read that info; my patch is mostly focused on allowing extensions to override the cert verification decisions.
(Note that anyone who wishes to wait for me to clean it up further before reviewing will certainly not be blamed.  :) )
I tend to think you should open a new bug for this.
(In reply to Tom Schuster [:evilpie] from comment #67)
> I tend to think you should open a new bug for this.

Unfortunately this bug seems to ambiguously involve 4 different use cases, all of which involve fairly separate parts of the codebase:

1. WebRequest allowing retrospective access to cert verification info.
2. WebRequest allowing retrospective access to other TLS handshake info (e.g. ciphersuites).
3. Modifying result of cert verification, during the TLS handshake.
4. Modifying result of other TLS handshake checks (e.g. ciphersuites), during the TLS handshake.

I certainly wouldn't object to splitting these out into separate bugs if there's a consensus to do so -- I believe I've privately complained myself about the ambiguity of this bug's scope in recent months.  That said, I'm new enough here that I'm hesitant to do this myself unless it's clear that there's a consensus do so do.

Is the consensus that we should split out this bug into more narrowly-scoped bugs?
Comment on attachment 8948228 [details] [diff] [review]
Cert override XPCOM API 2018-01-29.diff

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

This should go into a new bug.
That said, I don't think this is something we want. Firefox shouldn't allow WebExtensions to override trust decisions. Adding additional information or indicators is fine but overriding is a different issue.
Attachment #8948228 - Flags: feedback-
Unless WebExtensions are going to be totally tame and weak, there will always be APIs which extensions could use to disadvantage the user. Presumably we have some sort of review or permissions system to address that as best we can, that we use for multiple APIs of this "dangerous" type. We should use that here too.

Gerv
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #69)
> This should go into a new bug.

Moved this use case to Bug 1435951.
Comment on attachment 8899173 [details] [diff] [review]
apply new data from WebRequest.jsm to ext-webRequest.js

nobody seems interested
Attachment #8899173 - Attachment is obsolete: true
Comment on attachment 8899174 [details] [diff] [review]
gather security data from channel

nobody seems interested
Attachment #8899174 - Attachment is obsolete: true
Nice to see progress. A patch from inside mozilla seems to attract more interest and might be more likely to get merged.

(In reply to Shane Caraveo (:mixedpuppy) from comment #61)
[...]
> Definitely appreciate feedback on items we should expose.

sha256SubjectPublicKeyInfoDigest
for example is already included in nsIX509Cert (and needed by me).

Strictly speaking not "items", but if I understand the new patch and compare it to my withdrawn proposal,
the raw certificate, the parsed certificate tree (to get all items indirectly) and the other certificates in the chain with their "items" seem to be missing, but should be exposed (for my use case) as well.


(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #69)
[...]
> That said, I don't think this is something we want. Firefox shouldn't allow
> WebExtensions to override trust decisions. Adding additional information or
> indicators is fine but overriding is a different issue.

Not that I agree, but if this is taken as given, what about the (blocking) "onTLSestablished" event that was (with similar names) mentioned several times?
It should fire when securityInfo is available but before actual data is transmitted to the server.

This would allow extension to base their blocking decision on more context before submitting/requesting data without overriding the browsers trust decision.

This is less invasive than blocking onBeforeRequest by (only) matching an url. It would allow extensions to tighten but not relax the browsers view about the security of the connection.
(In reply to pubkeypin from comment #73)
> Comment on attachment 8899174 [details] [diff] [review]
> gather security data from channel
> 
> nobody seems interested

To the contrary, your patches were informative.  I choose to do something slightly different, but this patch in particular has items I'm still considering (which you just mentioned).
Attachment #8945650 - Attachment is obsolete: true
Attachment #8945650 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8951096 [details]
Bug 1322748 add securityInfo to webRequest listeners

I've added the cert list, asn1 object trees and rawDER.  Frankly, due to the amount of repetitive data and the size of the DER I highly doubt I will include them in the final version.  Lets see what Kris thinks about it.
Attachment #8951096 - Flags: feedback?(kmaglione+bmo)
(In reply to Shane Caraveo (:mixedpuppy) from comment #77)
> Comment on attachment 8951096 [details]
> Bug 1322748 add securityInfo to webRequest listeners
> 
> I've added the cert list, asn1 object trees and rawDER.  Frankly, due to the
> amount of repetitive data and the size of the DER I highly doubt I will
> include them in the final version.  Lets see what Kris thinks about it.

to be clear, I think the cert list is probably ok.  The ASN1 objects and rawDER concern me.
Thank you for extending your patch!
At first glance it seems to include all the functionality (and more) compared to my withdrawn proposal. As it is written by an experienced mozilla developer in a much cleaner and comprehensive way, my patch is really obsolete.

Of course I'd like to see more things added (as mentioned by others and me before in this bug). But this should not be a reason to keep this code from merging. Hopefully this is not the limit for webextension capabilities in this area.


(In reply to Shane Caraveo (:mixedpuppy) from comment #78)
> to be clear, I think the cert list is probably ok.  The ASN1 objects and
> rawDER concern me.

Regarding the ASN1 objects I completely agree, that this increases the generated amount of data. It is a work around. The superior solution would be to expose all items/extensions of each certificate in nsIX509Cert (or elsewhere) and make them all directly available to webextensions (as proposed in comment #62). Only if this doesn't happen in the near future, the ASN1 part is useful (from my point of view).

While I don't agree to restrict webextensions to the raw certs (comment #23), to omit them is not better. Some cases of use require access to raw certs.
Speaking for my Addon, it is (was) able to save all seen certificates. This way it is possible to walk through a large number of really used certs with the help of a script in order to find pieces of interest. This might be helpful to prove some fishy certs were used years ago without relying on external databases or Certificate Transparency logs from the big players, that might vanish some day.

Would you have less concerns to include the raw certificates, if it is guarded by an additional "opts" or/and if it is transformed into another format (string instead of array) before making them available to the webextension?

In line 231 of SecurityInfo.jsm, isn't there the name or value (rawDER: rawDER) missing?
(In reply to pubkeypin from comment #79)

> Would you have less concerns to include the raw certificates, if it is
> guarded by an additional "opts" or/and if it is transformed into another
> format (string instead of array) before making them available to the
> webextension?

It's not a concern about exposing the data (rather I think this is an area with lots of utility).  I'm worried about the size of data, it's a lot to pass around, especially for every request.  I'm inclined towards not including these for now, and having another bug to add some api to provide deep introspection if necessary.  So a first step gets us the easy/lightweight information with less concern over message size and performance, a later step for that.

> In line 231 of SecurityInfo.jsm, isn't there the name or value (rawDER:
> rawDER) missing?

You don't need that any longer.  {foobar} is the same as {foobar:foobar}.
Comment on attachment 8951096 [details]
Bug 1322748 add securityInfo to webRequest listeners

https://reviewboard.mozilla.org/r/220352/#review239922

::: toolkit/components/extensions/schemas/web_request.json:229
(Diff revision 1)
> +          },
> +          "certType": {
> +            "type": "string",
> +            "enum": [ "unknown", "ca", "user", "email", "server", "any" ]
> +          },
> +          "sha256SubjectPublicKeyInfoDigest": {

This name is kind of wierd... Can we make it something like `subjectPublicKeyInfoDigest.sha256`?

::: toolkit/components/extensions/schemas/web_request.json:280
(Diff revision 1)
> +            "type": "array",
> +            "items": { "$ref": "CertificateInfo" },
> +            "optional": true
> +          },
> +          "isDomainMismatch": {
> +            "type": "boolean",

Could use a description.

::: toolkit/components/extensions/schemas/web_request.json:288
(Diff revision 1)
> +          "isExtendedValidation": {
> +            "type": "boolean",
> +            "optional": true
> +          },
> +          "isNotValidAtThisTime": {
> +            "type": "boolean",

Could probably use a description.

::: toolkit/components/extensions/schemas/web_request.json:295
(Diff revision 1)
> +          },
> +          "isUntrusted": {
> +            "type": "boolean",
> +            "optional": true
> +          },
> +          "certificateTransparencyStatus": {

Could definitely use a description.

::: toolkit/components/extensions/schemas/web_request.json:302
(Diff revision 1)
> +            "enum": [
> +              "not_applicable",
> +              "policy_compliant",
> +              "policy_not_enough_scts",
> +              "policy_not_diverse_scts"
> +            ]

This should probably be a named type.

::: toolkit/components/extensions/schemas/web_request.json:316
(Diff revision 1)
> +            "description": "True if host uses Public Key Pinning and state is \"secure\".",
> +            "optional": true
> +          },
> +          "weaknessReasons": {
> +            "type": "array",
> +            "items": { "type": "string" },

Should be a named enum type.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_hsts.html:31
(Diff revision 1)
> +      browser.test.assertEq(details.url.startsWith("https"),
> +                            details.securityInfo && details.securityInfo.state == "secure",
> +                            "security info reflects https");

Please make this something like `if (url.startsWith("https")) assertEq("secure", state); else assertEq(...)` This is going to be a nightmare to reason about when it inevitably fails.

::: toolkit/modules/addons/SecurityInfo.jsm:95
(Diff revision 1)
> +    const NSSErrorsService = Cc["@mozilla.org/nss_errors_service;1"]
> +                               .getService(Ci.nsINSSErrorsService);

Please cache this.

::: toolkit/modules/addons/SecurityInfo.jsm:102
(Diff revision 1)
> +      if (channel.URI) {
> +        uri = channel.URI;
> +      }

No need for the `if`.

::: toolkit/modules/addons/SecurityInfo.jsm:110
(Diff revision 1)
> +      if (uri && !uri.schemeIs("https") && !uri.schemeIs("wss")) {
> +        // it is not enough to look at the transport security info -
> +        // schemes other than https and wss are subject to
> +        // downgrade/etc at the scheme level and should always be
> +        // considered insecure
> +        info.state = "insecure";

`info.state` is already "insecure" here.

::: toolkit/modules/addons/SecurityInfo.jsm:150
(Diff revision 1)
> +      info.protocolVersion =
> +        this.formatSecurityProtocol(SSLStatus.protocolVersion);
> +
> +      let certificates = [];
> +      let certChain = SSLStatus.serverCert.getChain();
> +      for (let i = 0; i < certChain.length; i++) {

`for (let cert of XPCOMUtils.IterSimpleEnumerator(certChain.enumerate(), Ci.nsIX509Cert))`

::: toolkit/modules/addons/SecurityInfo.jsm:158
(Diff revision 1)
> +        const sss = Cc["@mozilla.org/ssservice;1"]
> +                      .getService(Ci.nsISiteSecurityService);

Please cache.

::: toolkit/modules/addons/SecurityInfo.jsm:165
(Diff revision 1)
> +
> +        // SiteSecurityService uses different storage if the channel is
> +        // private. Thus we must give isSecureURI correct flags or we
> +        // might get incorrect results.
> +        channel.QueryInterface(Ci.nsIPrivateBrowsingChannel);
> +        let flags = (channel.isChannelPrivate) ?

Nit: Unnecessary parens.

::: toolkit/modules/addons/SecurityInfo.jsm:268
(Diff revision 1)
> +      case Ci.nsISSLStatus.TLS_VERSION_1:
> +        return "TLSv1";
> +      case Ci.nsISSLStatus.TLS_VERSION_1_1:
> +        return "TLSv1.1";
> +      case Ci.nsISSLStatus.TLS_VERSION_1_2:
> +        return "TLSv1.2";
> +      case Ci.nsISSLStatus.TLS_VERSION_1_3:
> +        return "TLSv1.3";
> +      default:
> +        return "unknown";

Please be consistent. Either always return on the same line as the `case` or always on the next line.

::: toolkit/modules/addons/SecurityInfo.jsm:301
(Diff revision 1)
> +      let isCipher = state & wpl.STATE_USES_WEAK_CRYPTO;
> +
> +      if (isCipher) {

Nit: Temporary variable isn't helpful.

::: toolkit/modules/addons/SecurityInfo.jsm:327
(Diff revision 1)
> +   *           }
> +   */
> +  getCertASN1Structure(sequence) {
> +    // Get the ASN1Sequence below the current one, bail if it is invalid.
> +    try {
> +      sequence.QueryInterface(Ci.nsIASN1Sequence);

`sequence instanceof Ci.nsIASN1Sequence`

::: toolkit/modules/addons/SecurityInfo.jsm:334
(Diff revision 1)
> +    if (!sequence || !sequence.isValidContainer) {
> +      return [];
> +    }
> +
> +    let ASN1Objects = [];
> +    for (let i = 0; i < sequence.ASN1Objects.length; i++) {

`for (let element of XPCOMUtils.IterSimpleEnumerator...)`

::: toolkit/modules/addons/WebRequest.jsm:752
(Diff revision 1)
>          if (!commonData) {
>            commonData = this.getRequestData(channel, extraData);
>            if (this.STATUS_TYPES.has(kind)) {
>              commonData.statusCode = channel.statusCode;
>              commonData.statusLine = channel.statusLine;
> +            // TODO: Should we require a opts.securityInfo before we retreive it?

Yes. This is hella expensive.
Attachment #8951096 - Flags: feedback?(kmaglione+bmo) → feedback+
Comment on attachment 8951096 [details]
Bug 1322748 add securityInfo to webRequest listeners

https://reviewboard.mozilla.org/r/220352/#review240682

::: toolkit/modules/addons/WebRequest.jsm:752
(Diff revision 1)
>          if (!commonData) {
>            commonData = this.getRequestData(channel, extraData);
>            if (this.STATUS_TYPES.has(kind)) {
>              commonData.statusCode = channel.statusCode;
>              commonData.statusLine = channel.statusLine;
> +            // TODO: Should we require a opts.securityInfo before we retreive it?

I should have expanded that comment.  I placed it there wondering if we should further limit what we do without explicit calls from the extension:

- is opts.securityInfo enough?
- should we have a details.getSecurityInfo() api?
- should we remove the cert tree and have a details.securityInfo.getCertTree() api?
Comment on attachment 8951096 [details]
Bug 1322748 add securityInfo to webRequest listeners

https://reviewboard.mozilla.org/r/220352/#review239922

> Please make this something like `if (url.startsWith("https")) assertEq("secure", state); else assertEq(...)` This is going to be a nightmare to reason about when it inevitably fails.

This test should always be https here.  Changing it to make it slightly better.
Attachment #8951096 - Attachment is obsolete: true
Comment on attachment 8966747 [details]
Bug 1322748 add securityInfo to webRequest listeners,

https://reviewboard.mozilla.org/r/235442/#review241134


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/modules/addons/WebRequest.jsm:1010
(Diff revision 1)
>    onErrorOccurred: onErrorOccurred,
> +
> +  getSecurityInfo: (details) => {
> +    let channel = ChannelWrapper.getRegisteredChannel(details.id, details.extension, details.tabParent);
> +    return SecurityInfo.getSecurityInfo(channel.channel, details.options);
> +  }

Error: Missing trailing comma. [eslint: comma-dangle]
Attachment #8966747 - Attachment is obsolete: true
Attachment #8966747 - Flags: review?(lgreco)
Attachment #8966752 - Flags: review?(lgreco)
Assignee: nobody → mixedpuppy
Comment on attachment 8966746 [details]
Bug 1322748 add ability to get registered channelwrappers,

https://reviewboard.mozilla.org/r/215786/#review242702

::: toolkit/components/extensions/schemas/web_request.json:180
(Diff revision 4)
>              }
>            }
>          }
>        },
>        {
> +        "id": "nsIASN1Object",

how about renaming this type to just "ASN1Object" instead of "nsIASN1Object"? 
is there any reason to prefer to keep the "nsI" prefix on this type? (it feels like a "internal" implementation details that is not needed to be exposed in the schema type names)

::: toolkit/components/extensions/schemas/web_request.json:444
(Diff revision 4)
> +                "optional": true
> +              }
> +            }
> +          }
> +        ],
> +        "returns": {

This method is asyncronous and so it actually returns a promise which resolves to the security info object (not sure if we have other cases like this in the current schemas, I recall that most of the `async: true` methods doesn't currently provide any informations about the type of the resolved value).

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_hsts.html:35
(Diff revision 4)
>  
> +      // We exepect all requests to have been upgraded at this point.
> +      browser.test.assertTrue(details.url.startsWith("https"), "connection is https");
> +
> +      let securityInfo = await browser.webRequest.getSecurityInfo(details.requestId, {
> +        certificateTree: true,

Nit, it could be reasonable to also call `webRequest.getSecurityInfo` with these properties set to false here, to check that they have been actually doing what they are expected to (or even better we could test the helper function exposed by SecurityInfo.jsm at a more "unit tests" level in separate xpcshell tests like I also mention in a comment below).

::: toolkit/modules/addons/SecurityInfo.jsm:19
(Diff revision 4)
> +                                   "nsINSSErrorsService");
> +XPCOMUtils.defineLazyServiceGetter(this, "sss",
> +                                   "@mozilla.org/ssservice;1",
> +                                   "nsISiteSecurityService");
> +
> +// NOTE: SecurityInfo is largly copied from the devtools NetworkHelper with some

Nit, typo: largly => largely

::: toolkit/modules/addons/SecurityInfo.jsm:20
(Diff revision 4)
> +XPCOMUtils.defineLazyServiceGetter(this, "sss",
> +                                   "@mozilla.org/ssservice;1",
> +                                   "nsISiteSecurityService");
> +
> +// NOTE: SecurityInfo is largly copied from the devtools NetworkHelper with some
> +// minor differences.

I'm wondering: is there any of these differences that may be worth to mention in this comment?

(I guess that the most important one is that our version should return a result which match the type defined in the related schema file, it could be reasonable to mention it explicitly here)

::: toolkit/modules/addons/SecurityInfo.jsm:53
(Diff revision 4)
> +   *            - hpkp: true if host uses Public Key Pinning, false otherwise
> +   *          If state == weak: Same as state == secure and
> +   *            - weaknessReasons: list of reasons that cause the request to be
> +   *                               considered weak. See getReasonsForWeakness.
> +   */
> +  getSecurityInfo(channel, options = {}) {

It would be nice to also have some additional tests for this helper function (besides the additional assertions added to the existent toolkit/components/extensions/test/mochitest/test_ext_webrequest_hsts.html by this patch).

We could add some at a "unit test" level using xpcshell tests that import and call this function directly and assert that the returned results have the format expected by the webRequest schema file for the SecurityInfo type.
(e.g. similarly to the original network-helper.js module that has a similar set of tests here: https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/test/unit)

::: toolkit/modules/addons/SecurityInfo.jsm:54
(Diff revision 4)
> +    const info = {
> +      state: "insecure",
> +    };
> +
> +    if (!channel) {
> +      return info;
> +    }

I'm not sure that a null channel is actually something that we expect to happen for the webRequest.getSecurityInfo API, my guess is that it is not expected. 

Maybe it could be reasonable to differ from the "devtools NetworkHelper" also for this scenario and throw an error when the channel is null?

::: toolkit/modules/addons/SecurityInfo.jsm:154
(Diff revision 4)
> +
> +    info.certificateTransparencyStatus = this.getTransparencyStatus(SSLStatus.certificateTransparencyStatus);
> +
> +    // Protocol version.
> +    info.protocolVersion =
> +      this.formatSecurityProtocol(SSLStatus.protocolVersion);

Nit, I'm wondering why this is splitted into two lines while there are other statements that are longer.

::: toolkit/modules/addons/SecurityInfo.jsm:167
(Diff revision 4)
> +    // HSTS and HPKP if available.
> +    if (uri && uri.host) {
> +      // SiteSecurityService uses different storage if the channel is
> +      // private. Thus we must give isSecureURI correct flags or we
> +      // might get incorrect results.
> +      let flags = channel instanceof Ci.nsIPrivateBrowsingChannel && channel.isChannelPrivate ? Ci.nsISocketProvider.NO_PERMANENT_STORAGE : 0;

This line is pretty long, how about splitting it into two lines? e.g.

let isPrivate = channel instance of ... && channel.isChannelPrivate;
let flags = isPrivate ? ... : ...;

::: toolkit/modules/addons/SecurityInfo.jsm:255
(Diff revision 4)
> +      certData.ASN1Objects = this.getCertASN1Structure(cert.ASN1Structure);
> +    }
> +    return certData;
> +  },
> +
> +  // Bug 1355903 Transparency is currently disbled using security.pki.certificate_transparency.mode

Nit, typo: disbled => disabled

(I guess that this typo is coming from the original devtools NetworkHelper version of this comment)

::: toolkit/modules/addons/WebRequest.jsm:18
(Diff revision 4)
>  ChromeUtils.defineModuleGetter(this, "ExtensionUtils",
>                                 "resource://gre/modules/ExtensionUtils.jsm");
>  ChromeUtils.defineModuleGetter(this, "WebRequestCommon",
>                                 "resource://gre/modules/WebRequestCommon.jsm");
>  ChromeUtils.defineModuleGetter(this, "WebRequestUpload",
>                                 "resource://gre/modules/WebRequestUpload.jsm");
> +ChromeUtils.defineModuleGetter(this, "SecurityInfo",
> +                               "resource://gre/modules/SecurityInfo.jsm");

Nit, given that this module already import XPCOMUtils, we could rewrite this group of `ChromeUtils.defineModuleGetter` calls into a single `XPCOMUtils.defineLazyModuleGetters`.
Comment on attachment 8966746 [details]
Bug 1322748 add ability to get registered channelwrappers,

https://reviewboard.mozilla.org/r/235440/#review251866

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:86
(Diff revision 1)
> +already_AddRefed<ChannelWrapper>
> +ChannelWrapper::GetRegisteredChannel(const GlobalObject& global, uint64_t aChannelId, const WebExtensionPolicy& aAddon, nsITabParent* aTabParent)
> +{
> +  TabParent* parent = static_cast<TabParent*>(aTabParent);
> +  nsIContentParent* contentParent = nullptr;
> +  if (parent) {

`if (TabParent* parent = ...)`
Attachment #8966746 - Flags: review?(kmaglione+bmo) → review+
certificate tree retrieval will be removed for this bug due to bug 867473.  We can post a followup to provide an additional api to fetch the chain if possible.  I'll also be requesting an additional review to make sure that the APIs we're touching for securityInfo are ok to use (ie. long-lived).
Comment on attachment 8966752 [details]
Bug 1322748 add securityInfo to webRequest listeners,

keeler to look at SecurityInfo.jsm (primarily) for any issues we may run into with cert apis.
Attachment #8966752 - Flags: review?(dkeeler)
Comment on attachment 8966752 [details]
Bug 1322748 add securityInfo to webRequest listeners,

https://reviewboard.mozilla.org/r/235444/#review251954

These APIs seem mostly fine. I noted the ones that aren't necessary to expose or are likely to be removed. I also noted some potential pitfalls with other APIs.

::: toolkit/modules/addons/SecurityInfo.jsm:152
(Diff revision 2)
> +    info.certificateTransparencyStatus = this.getTransparencyStatus(SSLStatus.certificateTransparencyStatus);
> +
> +    // Protocol version.
> +    info.protocolVersion = this.formatSecurityProtocol(SSLStatus.protocolVersion);
> +
> +    info.certificate = this.parseCertificateInfo(SSLStatus.serverCert, options);

If you want the whole chain, you should be able to use SSLStatus.succeededCertChain (or failedCertChain if the connection failed).

::: toolkit/modules/addons/SecurityInfo.jsm:196
(Diff revision 2)
> +      return {};
> +    }
> +
> +    let certData = {
> +      subject: {
> +        name: cert.subjectName,

I believe this is redundant - the subjectName field is the entire subject distinguished name, whereas commonName, organization, organizationalUnit, etc. are all potential parts of the distinguished name. Note that while most certificates will have exactly one common name, organization name, organizational unit name, etc. each in the subject DN, a certificate could have none or all or multiple or even other types of fields. So, while listing the CN, O, and OU is a common way of representing a DN, it is a simplification that may be inaccurate.

::: toolkit/modules/addons/SecurityInfo.jsm:201
(Diff revision 2)
> +        name: cert.subjectName,
> +        commonName: cert.commonName,
> +        organization: cert.organization,
> +        organizationalUnit: cert.organizationalUnit,
> +      },
> +      issuer: {

Same caveat that this doesn't necessarily accurately represent the issuer distinguished name.

::: toolkit/modules/addons/SecurityInfo.jsm:215
(Diff revision 2)
> +      fingerprint: {
> +        sha1: cert.sha1Fingerprint,
> +        sha256: cert.sha256Fingerprint,
> +      },
> +      serialNumber: cert.serialNumber,
> +      isBuiltInRoot: cert.isBuiltInRoot,

This is always going to be false unless this API exposes the whole chain.

::: toolkit/modules/addons/SecurityInfo.jsm:216
(Diff revision 2)
> +        sha1: cert.sha1Fingerprint,
> +        sha256: cert.sha256Fingerprint,
> +      },
> +      serialNumber: cert.serialNumber,
> +      isBuiltInRoot: cert.isBuiltInRoot,
> +      certType: this.getCertType(cert.certType),

This is a poorly-defined attribute. I wouldn't bother exposing it (it may also be removed in the future).

::: toolkit/modules/addons/SecurityInfo.jsm:217
(Diff revision 2)
> +        sha256: cert.sha256Fingerprint,
> +      },
> +      serialNumber: cert.serialNumber,
> +      isBuiltInRoot: cert.isBuiltInRoot,
> +      certType: this.getCertType(cert.certType),
> +      isSelfSigned: cert.isSelfSigned,

This field is not accurate as implemented. I would just remove it.

::: toolkit/modules/addons/SecurityInfo.jsm:227
(Diff revision 2)
> +    };
> +    if (options.rawDER) {
> +      certData.rawDER = cert.getRawDER({});
> +    }
> +    if (options.ASN1Sequence) {
> +      certData.ASN1Objects = this.getCertASN1Structure(cert.ASN1Structure);

This is another API I would recommend against exposing. It may be removed in the future. There are plenty of lightweight DER-decoding JS libraries extensions can incorporate - all we really need to expose is the raw DER.
Attachment #8966752 - Flags: review?(dkeeler)
Comment on attachment 8966752 [details]
Bug 1322748 add securityInfo to webRequest listeners,

If you can take another peak at SecurityInfo.jsm, I've incorporated all your feedback.  I also re-introduced the tree support.
Attachment #8966752 - Flags: review?(dkeeler)
Comment on attachment 8966752 [details]
Bug 1322748 add securityInfo to webRequest listeners,

https://reviewboard.mozilla.org/r/235444/#review252260

This looks good. My one overall comment would be that in terms of terminology, "certificate chain" is probably more common than "certificate tree" (while it is true that there may be multiple paths from an end-entity to trust anchors, we tend to only deal with one particular path, hence "chain").

::: toolkit/components/extensions/schemas/web_request.json:386
(Diff revisions 2 - 3)
>            {
>              "name": "options",
>              "optional": true,
>              "type": "object",
>              "properties": {
> -              "rawDER": {
> +              "certificateTree": {

I think 'chain' would be more widely used than 'tree' in this context.
Attachment #8966752 - Flags: review?(dkeeler) → review+
Comment on attachment 8966752 [details]
Bug 1322748 add securityInfo to webRequest listeners,

https://reviewboard.mozilla.org/r/235444/#review243716
Attachment #8966752 - Flags: review?(lgreco) → review+
Keywords: dev-doc-needed
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f66654437b7b
add ability to get registered channelwrappers, r=kmag
https://hg.mozilla.org/integration/autoland/rev/346ec4590599
add securityInfo to webRequest listeners, r=keeler,rpl
No longer depends on: 1312195
Depends on: 1464481
https://hg.mozilla.org/mozilla-central/rev/f66654437b7b
https://hg.mozilla.org/mozilla-central/rev/346ec4590599
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to 709922234 from comment #102)
> No API document?
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

It says "dev-doc-needed" :)
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Product: Toolkit → WebExtensions
Hi there! I see 

  status-firefox62: --- → fixed

But attempting to use tlsInfo on Firefox Nightly 62.0a1 (2018-06-21) (64-bit)


Returns the following:

  Error: Type error for parameter extraInfoSpec (Error processing 0: Invalid enumeration value "tlsInfo") for webRequest.onCompleted.

Code below:

	var filter = { urls: ['<all_urls>'] }

	var extraInfoSpec = ['tlsInfo', 'responseHeaders']

	// https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onCompleted
	browser.webRequest.onCompleted.addListener(function(details){
		log(`\n\nGot a request for ${details.url}`)
		log(JSON.stringify(details, null, 2))

	}, filter, extraInfoSpec)
status-firefox57: wontfix → ---
(In reply to mike.maccana from comment #105)
> Hi there! I see 
> 
>   status-firefox62: --- → fixed
> 
> But attempting to use tlsInfo on Firefox Nightly 62.0a1 (2018-06-21) (64-bit)

That's not how the api works  You call getSecurityInfo[1] that returns a SecurityInfo object[2].

let securityInfo = await browser.webRequest.getSecurityInfo(requestId, options);

requestId is received via the details object in any webRequest listener, although you likely wont have securityInfo before onHeadersReceived.

options has two boolean values: certificateChain and rawDER.

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/toolkit/components/extensions/schemas/web_request.json#372
[2] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/toolkit/components/extensions/schemas/web_request.json#240
(In reply to Shane Caraveo (:mixedpuppy) from comment #106)

Thanks Shane. I was using the EFF spec mentioned earlier (https://github.com/EFForg/webrequest-tlsinfo-api) but I've made the changes based on the links you provided (much apreciated).

	// https://developer.chrome.com/extensions/match_patterns
	var ALL_SITES = { urls: ['<all_urls>'] }

	// Mozilla doesn't use tlsInfo in extraInfoSpec //['tlsInfo', 'responseHeaders', 'blocking']
	var extraInfoSpec = null; 

	// https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onHeadersReceived
	browser.webRequest.onHeadersReceived.addListener(async function(details){
		log(`\n\nGot a request for ${details.url} with ID ${details.requestId}`)

		var requestId = details.requestId
		
		var securityInfo = await browser.webRequest.getSecurityInfo(requestId, {
			certificateChain: true,
			rawDER: false
		});

		log(JSON.stringify(securityInfo, null, 2))

	}, ALL_SITES, extraInfoSpec) 

The promise returned from getSecurityInfo() is undefined in 62.0a1 (2018-06-21) (64-bit)
Are the `keyUsages` and `validity` values in that returned data structure supposed to be easily parsed by addon code? (Is the localisation intentional or an oversight?)
(In reply to Will Elwood (:Will) from comment #109)
> Are the `keyUsages` and `validity` values in that returned data structure
> supposed to be easily parsed by addon code? (Is the localisation intentional
> or an oversight?)

I doubt this is intentional, please open a bug. You can use https://discourse.mozilla.org/c/add-ons/development for discussing API usage.
I've opened bug 1470516 regarding localised values, thanks.
If `await browser.webRequest.getSecurityInfo()` works for anyone else on the thread in 62 please let me know! My understanding is that this should be functioning in nightly now, but trying it (see previous comment above) returns undefined as the value.
(In reply to mike.maccana from comment #112)
> If `await browser.webRequest.getSecurityInfo()` works for anyone else on the
> thread in 62 please let me know! My understanding is that this should be
> functioning in nightly now, but trying it (see previous comment above)
> returns undefined as the value.

For me it works!
(In reply to mike.maccana from comment #107)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #106)
> 
> Thanks Shane. I was using the EFF spec mentioned earlier
> (https://github.com/EFForg/webrequest-tlsinfo-api) but I've made the changes
> based on the links you provided (much apreciated).
> 
> 	// https://developer.chrome.com/extensions/match_patterns
> 	var ALL_SITES = { urls: ['<all_urls>'] }
> 
> 	// Mozilla doesn't use tlsInfo in extraInfoSpec //['tlsInfo',
> 'responseHeaders', 'blocking']
> 	var extraInfoSpec = null; 
> 
> 	//
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/
> onHeadersReceived
> 	browser.webRequest.onHeadersReceived.addListener(async function(details){
> 		log(`\n\nGot a request for ${details.url} with ID ${details.requestId}`)
> 
> 		var requestId = details.requestId
> 		
> 		var securityInfo = await browser.webRequest.getSecurityInfo(requestId, {
> 			certificateChain: true,
> 			rawDER: false
> 		});
> 
> 		log(JSON.stringify(securityInfo, null, 2))
> 
> 	}, ALL_SITES, extraInfoSpec) 
> 
> The promise returned from getSecurityInfo() is undefined in 62.0a1
> (2018-06-21) (64-bit)

You need in extraInfoSpec "blocking".
> var extraInfoSpec = ["blocking"];
(In reply to kernp25 from comment #114)

Rock and roll. The TLS spice is now flowing. Thanks.

I've added demo code to https://stackoverflow.com/questions/2402121/within-a-web-browser-is-it-possible-for-javascript-to-obtain-information-about for those who come after me.
See Also: → 1471959
Depends on: 1470516
Some docs:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/getSecurityInfo
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/SecurityInfo
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/CertificateInfo

I also added a bit to the main page:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest#Accessing_security_information

I've also tried to account for bug 1470516.

A couple of things worth mentioning:

* Certificate.validity.start and Certificate.validity.end seem to be microseconds since the epoch, while the other WebExtension  APIs tend to use milliseconds for time values. I can't see a reason to use microseconds here - AIUI the certificate standard defines just seconds. Is it just that the underlying implementation uses microseconds?

* I couldn't get state==="broken" or, correspondingly, SecurityInfo.errorMessage to work: if I visit a bad site (e.g. https://expired.badssl.com/) then I get the Firefox "Your connection is not secure" error page, which blocks the connection unless I add a security exception.

Browser compat data is merged but not deployed yet, so won't appear in MDN pages yet. I will add an example to https://github.com/mdn/webextensions-examples, but didn't finish it yet.

Please let me know if I've missed or misrepresented anything :).
Flags: needinfo?(mixedpuppy)
> * Certificate.validity.start and Certificate.validity.end seem to be
> microseconds since the epoch, while the other WebExtension  APIs tend to use
> milliseconds for time values. I can't see a reason to use microseconds here
> - AIUI the certificate standard defines just seconds. Is it just that the
> underlying implementation uses microseconds?

:(  Bug 1474626
Flags: needinfo?(mixedpuppy)
Here is what I have found:

* browser.webRequest.onHeadersReceived won't fire on an HTTPS error, because no headers are received

* browser.webRequest.onErrorOccurred *does* fire, and in the request details, you have request.error === "Peer’s Certificate has expired". There is no error code, only the (presumably) internationalized error text.

* If you attempt to call browser.webRequest.getSecurityInfo() during onErrorOccurred, it should probably still return as much data as possible. Instead it simply returns undefined. I've opened bug 1474657 to address this.
Thanks, that's helpful. I've updated the docs to:

* use milliseconds instead of microseconds for validity, on the understanding that bug 1474626 will be uplifted

* say that at the moment you won't see `errorMessage` or `state==="broken"` because you can only get this info in onHeadersReceived, which doesn't fire when there is a TLS error. This is on the understanding that bug 1474657 will probably not be uplifted, but we can update the docs when it is resolved anyway.

I'm marking this dev-doc-complete, but please let me know if you see anything else that needs to be done here :).
Keywords: dev-doc-needed → dev-doc-complete
Dear all,
  
   How can I use the rawDER object that webRequest.CertificateInfo returns? I realized it's an array of numbers but I don't see the way to decoded it thorugh webextensions API.

Thank you very much!
I've done that in my certificate decoder here, using PKI.js:
https://github.com/april/certainly-something

There is nothing native in the WebExtensions API that parses the DER for you.
Thank you sooo much. But i thought u can't use modules, like a js in another js. Everything from the webextensions API or js API.

Thank you.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: