[Shield] Pref Flip Study: Trusted Recursive Resolver

RESOLVED FIXED

Status

RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: bagder, Assigned: bagder)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [trr])

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

10 months ago
Basic description of experiment: TRR is a separate and parallel way to resolve host names in the browser and the implementation allows for several different operational modes. We want to enable TRR in “shadow mode”, meaning that Firefox resolves all host names using both original native resolver mechanism as well as DNS-over-HTTPS (DOH) but the results from DOH are discarded and are only used for measuring and telemetry. For this experiment, we would use a cloudflare hosted server.

What is the preference we will be changing? network.trr.mode = 4, and network.trr.uri = “https://dns.cloudflare.com/.well-known/dns”

What are the branches of the study and what values should each branch be set to? Two branches: one using TRR, one not. (the one ‘not’ might actually be the control - it would have default prefs. Not sure of shield nomenclature.)

What percentage of users do you want in each branch? 50/50

What Channels and locales do you intend to ship to? Nightly

What is your intended go live date and how long will the study run? 7 days (?)
Are there specific criteria for participants? We want a random distribution to make it possible to assume both branches are sufficiently similar, user wise. Being able to break the data down by very rough locale would be interesting as internet topology will impact performance.

What is the main effect you are looking for and what data will you use to make these decisions? We will look at resolver timings, connection error rates and http response code changes.

Who is the owner of the data analysis for this study? Daniel Stenberg + Patrick McManus

Will this experiment require uplift? No

QA Status of your code: Green, yellow, red. Your code should be QA’d to ensure that changing the preference values has the intended effect you are looking for and does not cause obvious regressions to Firefox. All experiments must pass QA. Depending on the channel/population size a dev QA may be accepted.

Do you plan on surveying users at the end of the study? No.

Link to any relevant google docs / Drive files that describe the project. Links to prior art if it exists:



Details Section for Analysis
For each telemetry probe to be analysed in the study, find it here to determine the following:
Name of probes
DNS_LOOKUP_DISPOSITION
DNS_NATIVE_LOOKUP_TIME
DNS_TRR_RACE
DNS_LOOKUP_ALGORITHM
DNS_TRR_LOOKUP_TIME
DNS_BLACKLIST_COUNT
DNS_TRR_BLACKLISTED
DNS_CLEANUP_AGE
IPV4_AND_IPV6_ADDRESS_CONNECTIVITY
HTTP_RESPONSE_STATUS_CODE
Associated bugzilla thread URL:
e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=
(Assignee)

Updated

10 months ago
Flags: needinfo?(mgrimes)
Flags: needinfo?(isegall)

Comment 1

10 months ago
> For this experiment, we would use a cloudflare hosted server.

This will result in sending all DNS lookups in the study to Cloudflare. Given that even Mozilla does not collect this level of browsing data via telemetry/studies, I do not think it is reasonable to send this data to a third party.

Updated

10 months ago
Flags: needinfo?(mgrimes)
I think we shouldn't run this study in the proposed form.

Sending information about what is browsed to an off-path party will erode trust in Mozilla due to people getting upset about privacy-sensitive information (what they browse where "they" is identified by IP address and "what" by host name) getting sent to an off-path party without explicit consent.

The policy agreements we have in place with the off-path party won't remove this negative effect, since the way people are known to react this kind of thing isn't in our power to negotiate: people will react to this as a matter of what technically got sent and not as a matter of what the recipient promised not to do. (A browser sending information about what is browsed to an off-path party is the quintessential browser privacy no-no.)

(By off-path party, I mean a party that isn't *by necessity* on the network path between the user's computer and the site the user browses. The site can use third-party trackers or infrastructure providers, but that's an action on the site's part--not on the browser's part.)

The problem could be addressed in two ways:

 1) To study things like end-to-end reachability or round-trip time, Firefox could perform queries for a set of pre-defined names (to remove any correlation with what the user actually browses). This kind of thing has successful precedent as part of TLS 1.3 handshake studies.

 2) To study things under realistic use, we should obtain an explicit opt in specifically for sending DNS queries to Cloudflare. (We should do this even if it introduces a potential bias in the data.)
Are DoH requests made from private browsing sessions too?
Flags: needinfo?(daniel)
(Assignee)

Comment 4

10 months ago
Yes, DOH requests are made in PB as well. DOH is after all by design likely to be more private and secure than native resolves.

- The name resolve results are like "native" resolves in PB cached separately from non-PB resolves
- The TRR blacklist is never stored on disk for PB sessions
Flags: needinfo?(daniel)

Comment 5

10 months ago
Science review: R+, though I have some questions about the analysis that we can discuss when I have access to the phd
Flags: needinfo?(isegall)
These DoH requests definitely leak private browsing history to the (3rd-party) DoH provider.

Are the PB-cached DoH resolve results cleared when the user exits private browsing?
Flags: needinfo?(daniel)
Based on our risk matrix I'm adding a few more folks for sign off.
Flags: needinfo?(merwin)

Updated

10 months ago
Flags: needinfo?(nnguyen)
Flags: needinfo?(dcamp)
Since this is happening in Nightly, I think this is fine to launch.
Flags: needinfo?(nnguyen)
(Assignee)

Comment 9

10 months ago
(In reply to Luke Crouch [:groovecoder] from comment #6)
> These DoH requests definitely leak private browsing history to the
> (3rd-party) DoH provider.

Name resolving means asking a 3rd party (in all typical cases). It is often your ISP and it is often Google's DNS (8.8.8.8) or similar. In the DOH case it is *also* a 3rd party, that is correct. Probably not the same 3rd party though.

(and for this study, we're suggesting we leak to both 3rd parties for the purpose of getting data and metrics on how it fares)

Name resolving leaks info to 3rd parties. Both DOH and ordinary native resolving do.

> Are the PB-cached DoH resolve results cleared when the user exits private
> browsing?

doh-resolved names in PB are treated exactly the same as natively resolved names in PB in the cache. They're kept separate from the normal DNS cache (and they're only stored in memory). The DNS cache is in fact working the same way, doh or not.

I believe PB-resolved DNS cache entries are not cleared on "PB exit". The DNS cache isn't really aware of the concept of entering or exiting the mode.
Flags: needinfo?(daniel)

Comment 10

10 months ago
I also support this time-limited Nightly trial.
Flags: needinfo?(dcamp)
(In reply to Daniel Stenberg [:bagder] from comment #9)
> Name resolving leaks info to 3rd parties. Both DOH and ordinary native resolving do.

Short feedback on this:
The user's DNS provider (even it's the regular ISP) is the user's decision.

https://www.mozilla.org/en-US/privacy/firefox/#telemetry
It may be illegal in the EU to process parts of surf data without further consent. With agreeing to basic telemetry the Nightly user does not expect to transmit domains from his surf activity to any host defined by Mozilla. That's far more than a search request being routed to a search engine.

You have https://addons.mozilla.org/en-US/firefox/addon/firefox-pioneer/ ("specially marked SHIELD studies") for these types of experiments. https://support.mozilla.org/en-US/kb/about-firefox-pioneer

Comment 12

10 months ago
Tagging Selena too, my support is contingent on her go-ahead.
Flags: needinfo?(sdeckelmann)
(In reply to Daniel Stenberg [:bagder] from comment #9)
> I believe PB-resolved DNS cache entries are not cleared on "PB exit". The
> DNS cache isn't really aware of the concept of entering or exiting the mode.

Actually we do clear the DNS cache when the last PB is closed.
https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/netwerk/dns/nsDNSService2.cpp#1129-1130,1133
(Assignee)

Comment 14

10 months ago
(In reply to Valentin Gosu [:valentin] from comment #13)

> Actually we do clear the DNS cache when the last PB is closed.

Ah, yes there it is! Thanks for correcting me!
Thanks for the info. Since this does not leak any additional Private Browsing history to local adversaries, it seems fine to me. The new additional 3rd party receiving name resolution requests is still concerning. But I like the random_padding feature of DoH for some extra privacy. Are we planning to use the random_padding feature?

Comment 16

10 months ago
I am signing off, conditional on Selena giving the final go ahead. This experiment is testing a feature that could add valuable privacy and security protections for our users. We have in the past used Nightly for tests that have similar privacy properties.

We do need to be transparent about this is and to communicate clearly what prospective privacy and security benefits this feature will provide. We are working on a blog post that will add that transparency.
Flags: needinfo?(merwin)
(Assignee)

Comment 17

10 months ago
(In reply to Luke Crouch [:groovecoder] from comment #15)

> I like the random_padding feature of DoH for some extra privacy. Are we
> planning to use the random_padding feature?

Are you referring to HTTP/2 padding here? DOH itself, which really is a very basic protocol which mostly just encapsulate DNS-pakets as they look over UDP but over HTTP/2 and HTTPS instead, doesn't have any particular padding specified. I don't think we use padding in HTTP/2 (even if we of course support receiving it), but I'm not the expert on that.
(In reply to Merwin from comment #16)
> We have in the past used Nightly for tests that
> have similar privacy properties.
> 

Can you point to examples and how we communicated privacy leaking for those?

(In reply to Valentin Gosu [:valentin] from comment #13)
> (In reply to Daniel Stenberg [:bagder] from comment #9)
> > I believe PB-resolved DNS cache entries are not cleared on "PB exit". The
> > DNS cache isn't really aware of the concept of entering or exiting the mode.
> 
> Actually we do clear the DNS cache when the last PB is closed.
> https://searchfox.org/mozilla-central/rev/
> 877c99c523a054419ec964d4dfb3f0cadac9d497/netwerk/dns/nsDNSService2.cpp#1129-
> 1130,1133

The point here is to NOT SEND DNS requests to an additional 3rd party in PB, not to ensure we don't keep them persistent.

Does the study ensure that?


In general, I think the main concern here is to make sure people explicitly opt-in to this study, since this study (unlike other studies we run - fix me if I'm wrong) leaks private data to a 3rd party of our choice.


(Note that this TRR study made me to turn off running all studies on all my Nightly profiles)
(Assignee)

Comment 19

10 months ago
(In reply to Honza Bambas (:mayhemer) from comment #18)

> The point here is to NOT SEND DNS requests to an additional 3rd party in PB,
> not to ensure we don't keep them persistent.
> 
> Does the study ensure that?

I responded to a question about the currently implemented functionality, and it does not make any difference in Private Browsing.

The T in TRR stands for Trusted: this resolver mechanism is designed for you to select a trusted resolver to send your requests to and thus avoiding privacy-leaking and vulnerable plain-text resolves as is otherwise done natively. In such a setup I don't think it makes a lot of sense to use the non-trusted resolver when you switch to PB. Do you?

If something in the existing implementation should be changed to make a study better or more likely to be acceptable, that's a slightly different question but I'm listening and I certainly personally am not against modifications for that purpose.

As the discussion goes however, I don't think changing behavior during PB would make a very big difference.

Comment 20

10 months ago
PB is by design only private on the users end, not on the web end. How could it be?
To have true privacy regarding browsing, the user would need to use Tor or similar technologies.

I'm in favor of this study.

Comment 21

10 months ago
> PB is by design only private on the users end, not on the web end. How could it be?

Private Browsing enables Tracking Protection by default, which is supposed to keep the user safe(r) from third-parties.

In the context of this study, at least, Cloudflare is a third party that some of us don't necessarily want to trust with our browsing history [1]. As roc pointed out on the mailing list, a recent Mozilla blog post says:

> The headlines speak for themselves: Up to 50 million Facebook users had their information used by Cambridge Analytica, a private company, *without their knowledge or consent*. That’s not okay.
> [...]
> At Mozilla, our approach to data is simple: no surprises, and user choice is critical.

Do you really feel that opt-out transmission to a third-party of the users' browsing history is a good way to improve their trust in Mozilla? The community and media provided quite some push-back against recent initiatives like Pocket, the RAPPOR collection of browsing history proposal, Cliqz, and others.

This does respect https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories, but I think running Nightly isn't a free pass for Mozilla to silently collect such information. And that's without mentioning the tendency of the "Allow Studies" check box to get re-enabled by itself [2].

All we're asking for is making this an opt-in instead.

[1] While I respect their work, they had a major data leak in the past
[2] Bug #1425663
(In reply to i from comment #20)
> PB is by design only private on the users end, not on the web end. 

Note: the design goals of Private Browsing were updated at the beginning of this year to include protecting the private session data from online tracking. [1] E.g., enabling Tracking Protection, and stripping path information from 3rd-party requests. [2]

[1] https://wiki.mozilla.org/Private_Browsing
[2] https://blog.mozilla.org/security/2018/01/31/preventing-data-leaks-by-stripping-path-information-in-http-referrers/

> How could
> it be?
> To have true privacy regarding browsing, the user would need to use Tor or
> similar technologies.

Bugzilla comments of a single study are a poor channel for defining "true privacy". :) Suffice it to say - Private Browsing should try to match users' expectations as much as possible, not necessarily aim for "true privacy".

> 
> I'm in favor of this study.

I'm also in favor of this study.
(In reply to Daniel Stenberg [:bagder] from comment #17)
> (In reply to Luke Crouch [:groovecoder] from comment #15)
> 
> > I like the random_padding feature of DoH for some extra privacy. Are we
> > planning to use the random_padding feature?
> 
> Are you referring to HTTP/2 padding here? DOH itself, which really is a very
> basic protocol which mostly just encapsulate DNS-pakets as they look over
> UDP but over HTTP/2 and HTTPS instead, doesn't have any particular padding
> specified. 

Oh, sorry ... I was looking at the random_padding parameter of https://developers.google.com/speed/public-dns/docs/dns-over-https. But I suppose that's a Google-specific implementation.

Does our implementation try to use any HTTP/2 padding to mitigate traffic analysis of the DNS queries? Seems like it could potentially be a nice bit of extra protection?
wrt padding in h2 or tls:

it turns out that padding is ridiculously hard to do in a way that's effective; at least in the general case. It would be an interesting thing we could analyze (and because h2 and tls define it well we can make at least 1/2 the change unilaterally) for DNS. Its possible the nature of DNS could be more suitable to this than general web traffic is.

could you maybe open a bug/request on the topic? We don't need to discuss it in the shield bug.

But we should pursue that separately as resources accommodate; whatever the merits and demerits of TRR as a whole might be TRR is already a strict improvement vs the passive observer adversary.
(Assignee)

Comment 25

10 months ago
(In reply to Luke Crouch [:groovecoder] from comment #23)

> Oh, sorry ... I was looking at the random_padding parameter of ... But I
> suppose that's a Google-specific implementation.

Correct, that's a different protocol. The DNS-over-HTTPS protocol we are working on here is documented here: https://tools.ietf.org/html/draft-ietf-doh-dns-over-https-04

Comment 26

10 months ago
Could not you at least just running a Mozilla server for that, instead of using a (untrusted) third-party?

I agree that leaking domains to a third-party is not what you want to do (and yes, it can erode trust in Mozilla, remember the Mr. Robot thing, whcih has happened –  I know, this here is different, but I am just saying…)
Especially when others stumble upon this bug and now complain. (Hint: it has already happened, German text: https://www.kuketz-blog.de/firefox-nightly-uebermittlung-von-besuchten-domains/)
This experiment is currently on hold as we incorporate feedback provided by our community. I'm working with our networking team to include a more people-friendly explanation of the vision, intent and what the experiment entails before we move forward. 

Thanks everyone for participating.
Flags: needinfo?(sdeckelmann)
See Also: → bug 1455425
The DNS over HTTPS addon has been developed, and we would like to request that the study be launched on May 21st. We're ready for any necessary reviews for approval. The following has already taken place:

CONTENT
-------

Legal has signed off on all content used within the addon and the two blogs. Here are applicable links to our content:

* Content in shared google folders: https://drive.google.com/drive/folders/1SLIbS-KeNH8oOi9Vv4C-OuVAWwB43ebK?usp=sharing

* Improving DNS Privacy in Firefox blog to be posted on the Nightly blog: https://docs.google.com/document/d/1meeHOAbi4bJxra1-ocmjPW0AS23fEa9ynTy0Dln6wJQ/edit

* A Cartoon intro to DNS over HTTPS: https://medium.com/@linclark/cee3af19f10a

PRESS RELEASE
-------------

Erica Terry-Derryck will issue a press release. She's been notified that May 21st is the study's target release date. The team is waiting to hear back that her press release can go out a couple business days in advance of the study's release. 

QA TESTING
----------

The team has requested QA testing of the Addon to make sure everything works as expected. 

LOCALIZATION
------------

The addon content has been localized to US-EN, UK-EN, German, and French. Russian is pending but it not a blocker for the study's release. An DNS over HTTPS SUMO page is being localized in these same languages.

UX FLOW
-------

The UX flow for users interacting with the study is documented in: https://docs.google.com/document/d/1gUG8J7tKuNoaHzZ3F28U425dQUUb1nch_Q_FLuMl8NE/edit

ADDITIONAL INFORMATION
----------------------

In addition to the description provided in this bug, additional information about the study can be found in the team's meetings notes: https://drive.google.com/drive/u/1/folders/1C0krCr4omlR488gHpTFeYzXQ4unbKGek
Flags: needinfo?(mgrimes)
(In reply to Julie McCracken (:julie) from comment #28)
> The addon content has been localized to US-EN, UK-EN, German, and French.
> Russian is pending but it not a blocker for the study's release. An DNS over
> HTTPS SUMO page is being localized in these same languages.

Correction to the list of locales: we currently have de, en-US, en-GB, it. fr and ru are partially translated, but should be ready well before the deadline.
Has a Firefox peer been assigned to review the add-on?
Flags: needinfo?(jmccracken)
Is there an end date for this study? Or should it complete when 62 goes beta?

Also, we'll need the Intent to Ship sent to release-drivers prior to the planned study launch date of May 21.
(In reply to Marnie Pasciuto-Wood [:marnie] from comment #31)
> Is there an end date for this study? Or should it complete when 62 goes beta?
> 
> Also, we'll need the Intent to Ship sent to release-drivers prior to the
> planned study launch date of May 21.

We don't currently have an end date. It will depend on the different things we want to test as part of this study and the outcomes. Is there any problem with this?

And yes, we'll make sure to let release-drivers know.

I'm still awaiting confirmation that the press release about this study can go out by the end of the week to provide advance notice. That's our dependency for launching.
Flags: needinfo?(mpasciutowood)
Flags: needinfo?(mgrimes)
Flags: needinfo?(jmccracken)
Flags: needinfo?(jmccracken)
Assuming we get permission to proceed with launching the study May 21st, according to Erica Terry Derryck the pre-announcement will not be a press release but rather Jen Boscacci's pointing the press to our two blogs on Thursday, May 17th, and Friday, May 18th, respectively. We will also need to alert product legal to be on standby to talk to the press if the need arises. In addition, we'll need to have either Matt Grimes or Dave Camp be available to answer questions from the press since Selena is out on leave. All this needs to be coordinated in the next couple days and is dependent upon approval to launch. Where are we at with the approval?
Flags: needinfo?(jmccracken)
Hey Julie. Before we flag Dave for an approval we're still going to need QA sign off and sign off from the Firefox peer that reviewed the study. Do you have these two signoffs lined up? They'll need to be documented in the bug.
Can you recommend someone to peer review?
Flags: needinfo?(dtownsend)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Created attachment 8976102 [details] [diff] [review]
Implementation PR
Attachment #8976100 - Attachment is obsolete: true
Attachment #8976100 - Flags: review?(jhofmann)
Attachment #8976102 - Flags: review?(jhofmann)
Flags: needinfo?(dtownsend)
Flags: needinfo?(mpasciutowood)
Comment on attachment 8976102 [details] [diff] [review]
Implementation PR

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

Gave some feedback on the whole patch and getting this off my list for now.

I think we're pretty close to getting this done, though. Let me know when there's a new revision on GitHub. :)
Attachment #8976102 - Attachment is patch: true
Attachment #8976102 - Attachment mime type: text/x-github-pull-request → text/plain
Attachment #8976102 - Flags: review?(jhofmann) → review-
Fixed the feedback I will flag again on Monday. Thanks!
Comment on attachment 8976102 [details] [diff] [review]
Implementation PR

I think I covered all your feedback mentioned so flagging you again.
Attachment #8976102 - Flags: review- → review?
* As a reminder, we are now planning to launch on June 4th. (as noted in Slack last week)
* QA testing was completed, and issues are being discussed with Jonathan and Michelle. We're getting clarification re. Disable and Remove buttons.
* The other pending requirement is the addon peer review, discussed in comments 40-42.
I'm going to give this another look, I just wanted to mention that previous Shield studies have been doing try runs with the add-on enabled to catch leaks and/or potential breakage with the default behavior:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cfc7d8ccd8fea10513089b7bd8dafb8b920f621
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1c2b190169506dc82a47e7d7af13fea85418261

In one case it was actually effective, so maybe it wouldn't hurt just doing a quick try run for this add-on, too.
Attachment #8976102 - Flags: review? → review?(jhofmann)
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ac7ffe857b11804dc3f69987837e8f0e91d6737

Johann mentioned he is blocked on the decision based on the remove/disable and restoring of preferences before he can r+ etc.
Updated try run with the latest changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cee45c3edc7e47a4cba53ad5f898ebe3142ff65

It should be good to review again Johann with the agreed upon changes from the meeting.
Comment on attachment 8976102 [details] [diff] [review]
Implementation PR

Thanks! Just had a few more questions.
Attachment #8976102 - Flags: review?(jhofmann) → review-
Comment on attachment 8976102 [details] [diff] [review]
Implementation PR

Flagging again as answered one question and made two changed based on the comments.
Attachment #8976102 - Flags: review- → review?
Attachment #8976102 - Flags: review? → review?(jhofmann)
Comment on attachment 8976102 [details] [diff] [review]
Implementation PR

Looks good, thank you!
Attachment #8976102 - Flags: review?(jhofmann) → review+
We have finished testing the "DNS Over HTTPS" Shield Study experiment.

QA’s recommendation: GREEN - Ship it

Reasoning: All the reported issues are fixed and verified.

Testing Summary:
Test Cases: https://testrail.stage.mozaws.net/index.php?/plans/view/9852
Tested Platforms: Windows 10 x64, Ubuntu 16.04 x64, macOS 10.13
Tested Firefox versions: Firefox Nightly 62.0a1
We should be ready for :dcamp to check this over now that we have QA and peer review signed off.
Flags: needinfo?(jmccracken)
Hey :francois, we don't have any probes in the code at all for this study, but instead the telemetry is in Central. Does this still need a review? (I feel like I have asked this somewhere else but couldn't find it).

Thanks
Flags: needinfo?(francois)
(Adding this here just for posterity)

I had some additional concerns that I just chatted with jkt about. Our code might not be correctly avoiding overwriting custom set user prefs. I could be wrong about this so I asked jkt to double check.
(In reply to Jonathan Kingston [:jkt] (on PTO) from comment #52)
> Hey :francois, we don't have any probes in the code at all for this study,
> but instead the telemetry is in Central. Does this still need a review? (I
> feel like I have asked this somewhere else but couldn't find it).

If you're not actually adding new data collection and are just looking at the data we already collect in telemetry, then you don't need to do a data review.
Flags: needinfo?(francois)
(In reply to Jonathan Kingston [:jkt] (on PTO) from comment #51)
> We should be ready for :dcamp to check this over now that we have QA and
> peer review signed off.

TRR Go/No GO has been scheduled by Matt Grimes for May 30th at 1:30 PDT/9:30 GMT (UK time) and invitation sent.
Just wrapped up the Go/No GO meeting. Flagging dcamp for a VP signoff.
Flags: needinfo?(dcamp)
Approved, thanks for making the changes.
Flags: needinfo?(dcamp)
(In reply to Julie McCracken (:julie) from comment #55)
> (In reply to Jonathan Kingston [:jkt] (on PTO) from comment #51)
> > We should be ready for :dcamp to check this over now that we have QA and
> > peer review signed off.
> 
> TRR Go/No GO has been scheduled by Matt Grimes for May 30th at 1:30 PDT/9:30
> GMT (UK time) and invitation sent.
Stephanie, if this study passes your security review, please indicate your sign-off in this bug.
Flags: needinfo?(jmccracken) → needinfo?(stephouillon)
From relman point of view this is low risk and a GO.
Paul and I did a security review of the Shield Study, complete notes can be read at https://docs.google.com/document/d/1_wyqoZww7g_zmZgSzSuPp0kBV10CCqCl9d3C6RuxKmw/edit#heading=h.3ovbbnpelc3.

Two identified risks could get stronger mitigations:

* Risk: User’s DNS entries are tracked without their knowledge. 

The user might ignore or miss the warning without choosing an option.
In these cases the users DNS is being tracked without their explicit approval.
=> Business decision needed about whether the warning alone is sufficient and what behavior is necessary.

* Risk: Experiment Setting API of app extension can modify any preference.

Experimental APIs are bound to extensions (one extension can’t call anothers APIs in regular usage).
However in a compromise scenario, an attacker who manages to control the extension process can likely call this experimental API
=> One way to make this API more secure might be to validate the pref names being set against the state.json (file in the extension package)
Flags: needinfo?(stephouillon)
I just updated the code at: https://github.com/jonathanKingston/http-dns/pull/4 to move the pref manipulation outside of the regular extension code.
I mostly was concerned about causing different security issues there by having arbitrary file access on the OS however I think I have hopefully prevented "../" in the path and am safely reading the JSON file.

Other changes:
- I changed the URL to the hacks post.
- Merged the updated eslint config
- Removed the extension when the user doesn't match the pref criteria, :johannh suggestion.


Could all three of you check that nothing has broken in these changes?


Thanks so much!
Flags: needinfo?(stephouillon)
Flags: needinfo?(jhofmann)
Flags: needinfo?(carmen.fat)
Depends on: 1462082
I couldn't find anything obvious, it's fine with me.
Flags: needinfo?(stephouillon)
This seems good from a cursory look. Modified user prefs are now also treated correctly (IMO).
Flags: needinfo?(jhofmann)
This is also looking good from our side.
Flags: needinfo?(carmen.fat)
Created attachment 8983083 [details]
http-dns.zip

This code has been verified by all of the interested parties etc.
Are there any further steps to be taken?
Flags: needinfo?(mcooper)
I can't sign this zip file. Both the signing tool and Firefox say that it is not a valid extension. Firefox gives the error "There was an error during installation: File /home/mythmon/Downloads/http-dns.zip does not contain a valid manifest".

Additionally, I recommend including the version number of the extension in the filename. In the past we've used the pattern "{id}-{version}.{ext}", like "httpdns@shield.mozilla.org-1.0.zip". This helps avoid confusion if we iterate on the add-ons after signing.
Flags: needinfo?(mcooper)
Created attachment 8983104 [details]
dns_over_https-1.0.zip

Sorry that was the whole codebase. This is the result of running web-ext build on the reviewed code.
Attachment #8983083 - Attachment is obsolete: true
Flags: needinfo?(mcooper)
Flags: needinfo?(mcooper)
For clarification: 

The study has been localized in 6 languages: 

us english
gb english
german
french
russian
italian

All other locales should get EN-US.

Comment 72

8 months ago
We're live.  Thanks everyone!
Nitpick: The description of the study is so long that about:studies ellipsizes it. It's complete in the tooltip but that makes the buglink at the end hard to follow. Maybe put the link at the start next time.
Created attachment 8983485 [details] [diff] [review]
Update to 1.1.0 version

So it turns out the URL Github uses has become more strict and we should be instead using: "https://mozilla.cloudflare-dns.com/dns-query" I also changed to account for users who have clicked OK to hide the banner.
Attachment #8976102 - Attachment is obsolete: true
Attachment #8983485 - Flags: review?(jhofmann)
Attachment #8983485 - Flags: feedback?(carmen.fat)
Sorry: s/Github/Cloudflare/
Comment on attachment 8983485 [details] [diff] [review]
Update to 1.1.0 version

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

Note that I'm traveling and only had a brief look at the changes, but it's not a lot and makes sense to me.
Attachment #8983485 - Attachment is patch: true
Attachment #8983485 - Attachment mime type: text/x-github-pull-request → text/plain
Attachment #8983485 - Flags: review?(jhofmann) → review+
Comment on attachment 8983485 [details] [diff] [review]
Update to 1.1.0 version

In order to test the latest changes, we created a custom build from the study's Github repo.
Here are the results:
- we verified that the Cloudflare URL is correctly updated ("about:config" page);
- we confirmed that websites are loaded properly and there were no error connection warnings;
- checked that the pref values specific to the study (install, disable and remove scenarios) were not affected by the changes.
These changes don't seem to have broken anything of the study.

Please note that we weren't able to test the change for the users who already clicked "OK" button from the banner since this would've meant that we were already enrolled in the study.
Attachment #8983485 - Flags: feedback?(carmen.fat) → feedback+
Created attachment 8986472 [details]
dns_over_https-1.1.zip

Hey :mythmon,
Could you sign this newer version of the addon?

Thanks!
Attachment #8983104 - Attachment is obsolete: true
Flags: needinfo?(mcooper)
Flags: needinfo?(mcooper)
From a Shield Operator to many interested parties around TRR:

1.  Enrolling TRR version 1.1.  (see Launch details below) in Nightly.
2.  Analysis:  We are working with the Data Engineering team to resolve issues around analysis, and will have better answers within the week.


Gregg Lind


Launch Details
- current users keep their (patched) 1.0.
- previous users will not enroll
- any enrolling Nightly will get 1.1.

Recipes:
https://normandy-admin.prod.mozaws.net/recipe/484/
https://normandy-admin.prod.mozaws.net/recipe/485/
https://normandy-admin.prod.mozaws.net/recipe/486/
https://normandy-admin.prod.mozaws.net/recipe/487/
https://normandy-admin.prod.mozaws.net/recipe/488/
During a bit of knowledge sharing with the Addons QA team, looking over the TRR shield recipe, more exactly after recipe/484/, we've noticed a weird inconsistency, which also got reflected into a short test run.

(:cfat/:cmuresan) The study sets the pref. network.trr.experimentalRollout.
But the filter for normandy recipe to run has this: !('network.trr.experimentalRollout'|preferenceExists) - which reads that pref. network.trr.experimentalRollout shouldn't exist for the study to be executed. The pref. initially doesn't exist, therefore the study is ran, the add-on installed and then the add-on sets the above preference, so on its next run, Normandy unenrolls the user, since the filter is not matched anymore.


app.normandy.recipe-runner	INFO	Executing recipe "Addon Study: Trusted Recursive Resolver 2, Nightly, en [Bug1446404" (action=opt-out-study)
app.normandy.normandy-driver.actions	DEBUG	Starting study for recipe 484

app.normandy.recipe-runner	DEBUG	Fetched 3 actions from the server: opt-out-study, show-heartbeat, preference-experiment
app.normandy.recipe-runner	INFO	Executing recipe "Pref Flip Study: Hotfix Trusted Recursive Resolver, Nightly [Bug 1446404]" (action=preference-experiment)
app.normandy.preference-experiments	DEBUG	PreferenceExperiments.has(prefflip-hotfix-trr-1446404)
app.normandy.preference-experiments	DEBUG	PreferenceExperiments.start(prefflip-hotfix-trr-1446404, fix)
app.normandy.preference-experiments	DEBUG	PreferenceExperiments.startObserver(prefflip-hotfix-trr-1446404)
app.normandy.normandy-driver.actions	DEBUG	Stopping study for recipe 484.
[Show/hide message details.] this.box.getNotificationWithValue(...) is null api.js:58
app.normandy.preference-experiments	DEBUG	PreferenceExperiments.stop(prefflip-hotfix-trr-1446404, {resetValue: false, reason: user-preference-changed})
app.normandy.preference-experiments	DEBUG	PreferenceExperiments.hasObserver(prefflip-hotfix-trr-1446404)
app.normandy.preference-experiments	DEBUG	PreferenceExperiments.stopObserver(prefflip-hotfix-trr-1446404)


To sum-up, for new profiles on Nightly the TRR add-on study stays enrolled ~10' minutes (due to bug 1446421) and on a normal profile ~6 hours, after-which it gets unrolled.




Not entirely sure how the TRR pref. experiment and TRR add-ons studies are supposed to work or to integrate with eachother, but the above behaviour doesn't seem right. Need info'ing Greg and Mike on the subject, might be I'm missing something.
Flags: needinfo?(mcooper)
Flags: needinfo?(glind)
I meant "Experiments QA" team instead of "Addons QA" team in the beginning of the above comment.
We believe we have fixed the unenrollment issue and pushed that update live. Josh Gaunt will now be assigned as the fulltime analyst for this study. Any additional requests for data can be funneled to him directly.
Flags: needinfo?(mcooper)
Flags: needinfo?(glind)
(Assignee)

Updated

6 months ago
Blocks: 1475321

Updated

6 months ago
See Also: → bug 1475528
this bug was for study 1 (and study 2 to fix the deploy issue). It is complete.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Blocks: 1485398
You need to log in before you can comment on or make changes to this bug.