831614 - Story - Set up Sync for both Firefoxes in desktop Firefox

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Sync, defect, P2)

Description

[Asa Dotzler [:asa]]

Attachment: UC-1-11 Set up Sync for both Firefoxes in desktop Firefox

Comment 1

[Matt Brubeck (:mbrubeck)]

bbondy or jimm might be able to provide an estimate for this story.

Comment 2

[Brian R. Bondy [:bbondy]]

Provided the info just forgot to reset the flag.  Apparently it doesn't auto clear the flag when you only change whiteboard tags.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Widget

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Metro sync

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Browser sync UI changes

Comment 6

[Brian R. Bondy [:bbondy]]

bsmith/imelvin:

Would it be possible to get a yay/nay on whether the method used in the widget patch is acceptable?

Summary:
We want to automatically setup Metro Sync when Desktop Sync is setup.

Metro and Desktop are in different profiles and their password manager values are encrypted separately.

What these patches do is save out the username, password, and key at setup time.

It uses CryptProtectData/CryptUnprotectData at it's core to store this data. Which ensures that only the same user can decrypt the data.

I mainly just need to know if that's acceptable from a security standpoint to land this.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 721524 [details] [diff] [review]
Patch v1 - Metro sync

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

::: browser/metro/base/content/sync.js
@@ +53,5 @@
> +      metroUtils.clearSyncInfo();
> +      this.connect();
> +    } catch (ex) {
> +        return false;
> +    }

Please make sure that the try/catch block includes only code where we are expecting a specific exception (just the call to loadSyncInfo?).  If we silently swallow other errors here, it'll make unexpected problems harder to debug.

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Metro sync

Implemented review nit. Carrying forward r+.

Comment 9

[[e-mail]]

Comment on attachment 721525 [details] [diff] [review]
Patch v1 - Browser sync UI changes

Sorry, I'm not a peer.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 721525 [details] [diff] [review]
Patch v1 - Browser sync UI changes

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

::: browser/locales/en-US/chrome/browser/syncSetup.dtd
@@ +33,5 @@
>  <!ENTITY setup.choosePassword.accesskey  "P">
>  <!ENTITY setup.confirmPassword.label     "Confirm Password">
>  <!ENTITY setup.confirmPassword.accesskey "m">
> +<!ENTITY setup.setupMetro.label     "Also sync Firefox on my start screen">
> +<!ENTITY setup.setupMetro.accesskey "M">

The accesskey should be the same case as the character in the label that it is referencing.

::: browser/themes/windows/syncSetup.css
@@ +126,5 @@
>    list-style-image: url("chrome://browser/skin/sync-32.png");
>  }
> +
> +#tosDesc {
> +  margin-left: -7px;

This should use -moz-margin-start as the property name so it will work as expected for RTL environments.

Comment 11

[Marco Mucci [:MarcoM]]

Story being returned to the Product Backlog until formal security reviews have been completed.

Comment 12

[Brian R. Bondy [:bbondy]]

Talked to curtisk and he suggested to mark this as sec-review? and they will triage it for a sec code review.

Comment 13

[Brian R. Bondy [:bbondy]]

Details for sec review in Comment 6.

Comment 14

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

chaning flag to <blank> so it goes to security triage on 2013.03.13

Comment 15

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

changing flag to <blank> so it goes to security triage on 2013.03.13

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Browser sync UI changes

Implemented review comments, carrying forward r+.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Metro sync

Comment 18

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

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> It uses CryptProtectData/CryptUnprotectData at it's core to store this data.
> Which ensures that only the same user can decrypt the data.
> 
> I mainly just need to know if that's acceptable from a security standpoint
> to land this.

What is the purpose of using CryptProtectData/CryptUnprotectData? Keep in mind that other applications that have access to the user's registry hive can read and/or write any of these values. In particular, malware will be able to decrypt the data and/or substitute alternative credentials. So, that means that this offers no protection against malware. I do not think that is in your threat model anyway. And, the local file system has the same issues. 

Methods like this will fail to work in the case where the user copies his user profile from one machine to another using normal file copy operations, because he/she will have a new key on the new computer.

I am very curious as to why Metro Firefox needs to run in a separate profile from normal Firefox. I am sure there is a good reason, but I thought Microsoft's exception for sandboxing for web browsers allowed us to do pretty much everything we'd normally need to do without resorting to tricks. It would be great if somebody could email me an explanation of why these tricks are necessary.

Note: I did not review the patch.

Comment 19

[Brian R. Bondy [:bbondy]]

Thanks for the feedback bmisth.

> What is the purpose of using CryptProtectData/CryptUnprotectData? 
1. Data will not be human readable.
2. Data will not be able to be decrypted from another user accounts such as admin accounts.

> In particular, malware will be able to decrypt the data and/or substitute alternative credentials. 

Only malware running as the same user account. This is only used to auto-setup metro after a new account is blank sync account is created.  They can substitute invalid values in if they want, there is no harm.  It will be ignored and cleared. 

If a malware process is already running within the computer they can already read all of the data we're syncing anyway. So they could themselves copy it out wherever they wanted.  I'm not sure why this is a concern to us.

> So, that means that this offers no protection against malware. 
> I do not think that is in your threat model anyway. And, the local file 
> system has the same issues. 

As above I don't think there is any threat we need to protect against from that.


> Methods like this will fail to work in the case where the user copies 
> his user profile from one machine to another using normal file 
> copy operations, because he/she will have a new key on the new computer.

If the user copies a metro profile from one machine to the other it will work. The registry entries created are only used when the metro browser starts up and then they are stored in the normal way in password manager. 

> I am very curious as to why Metro Firefox needs to run in a separate 
> profile from normal Firefox.

The same reasons we don't allow 2 instances of Firefox at the same time running on the same profile.  There has been a lot of discussion about profile sharing in the past and we decided against it.  This bug is not the place to continue the discussion on profile sharing, but feel free to email [e-mail] for a discussion.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 721523 [details] [diff] [review]
Patch v1 - Widget

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

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +187,5 @@
> +  passwordIn = { 
> +    (aPassword.Length() + 1) * 2,
> +    (BYTE *)aPassword.BeginReading()},
> +  keyIn = { 
> +    (aKey.Length() + 1) * 2,

nit - splinter shows some end of line white space here.

@@ +209,5 @@
> +                 NS_ConvertUTF8toUTF16(regPath),
> +                 nsIWindowsRegKey::ACCESS_SET_VALUE);
> +
> +  nsresult rv1 =
> +    regKey->WriteBinaryValue(NS_LITERAL_STRING("weave-e"),

nit - I think we've dropped the internal 'weave' name. something like 'sync' would be better. Also I'd move these into const strings up top.

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Widget

Implemented review comments. Carried forward r+.

Comment 23

[Brian R. Bondy [:bbondy]]

This is ready for landing as long as security gives the OK.

Comment 24

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> bsmith/imelvin:
> 
> Would it be possible to get a yay/nay on whether the method used in the
> widget patch is acceptable?
> 
> Summary:
> We want to automatically setup Metro Sync when Desktop Sync is setup.
> 
> Metro and Desktop are in different profiles and their password manager
> values are encrypted separately.
> 
> What these patches do is save out the username, password, and key at setup
> time.
> 
> It uses CryptProtectData/CryptUnprotectData at it's core to store this data.
> Which ensures that only the same user can decrypt the data.
> 
> I mainly just need to know if that's acceptable from a security standpoint
> to land this.

It's fine with me - bsmith already made a couple of good points, including that malware running as the user is probably not something we're trying to protect against here. It's probably better to use the CryptProtect/Unprotect approach than using the local file system and doing encryption, or writing a plaintext file which we then have to worry about cleaning up.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: New checkbox in Desktop Sync Setup Window

Hey Yuan, could you let me know which text you would like for the checkbox which currently reads: "Also sync Firefox on my start screen"?

The rest of the dialog is beyond the scope of this bug. But feel free to comment on the positioning of the checkbox and especially text of the checkbox.

Comment 26

[Brian R. Bondy [:bbondy]]

Thanks for taking a look Ian and bsmith. I heard that the security triage meeting is today so I think we'll gt more feedback on it soon, or at least someone assigned to it today.

Comment 27

[Brian R. Bondy [:bbondy]]

Info from Yuan via IRC:
"Also sync with Windows 8 style $shortBrand"

Comment 28

[David Chan [:dchan]]

Comment on attachment 723311 [details] [diff] [review]
Patch v2 - Widget

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

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +269,5 @@
> +      !CryptUnprotectData(&passwordIn, nullptr, nullptr, nullptr,
> +                          nullptr, 0, &passwordOut) ||
> +      !CryptUnprotectData(&keyIn, nullptr, nullptr, nullptr,
> +                          nullptr, 0, &keyOut)) {
> +    return NS_ERROR_FAILURE;

The code should LocalFree() any decrypted data if an expression fails

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Browser sync UI changes

Updated sync checkbox text as per Yuan's suggestion and associated accelerator key.

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Widget

Always LocalFree now as per dchan in Comment 28.

Also fixed this review nit I forgot to implement before:
> Also I'd move these into const strings up top.

Comment 31

[David Chan [:dchan]]

Comment on attachment 725055 [details] [diff] [review]
Patch v3 - Widget

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

The rest of the code / patches look good. Barring any major code changes, I'll change the sec-review flag to +

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +278,5 @@
> +                                      nullptr, 0, &passwordOut) &&
> +                   CryptUnprotectData(&keyIn, nullptr, nullptr, nullptr,
> +                                      nullptr, 0, &keyOut);
> +
> +  aEmail = reinterpret_cast<wchar_t*>(emailOut.pbData);

wrap these assignments in a
if (succeeded) check?

I'm assuming CryptUnprotectData doesn't change the DATA_BLOB on failure. This may result in a null dereference at some point after cast.

Comment 32

[Brian R. Bondy [:bbondy]]

> Barring any major code changes, I'll change the sec-review flag to +

Thanks that sounds great, I don't anticipate any major changes but if I make any I'll re-request sec-review? from you.  Just to make sure you seen the discussion in Comment 18 and Comment 19 right?

> wrap these assignments in a
> if (succeeded) check?

Will do.

> I'm assuming CryptUnprotectData doesn't change the DATA_BLOB on failure. 
> This may result in a null dereference at some point after cast.

From my testing ya, if you want I can explicitly NULL it on each failure though just in case.

Comment 33

[David Chan [:dchan]]

(In reply to Brian R. Bondy [:bbondy] from comment #32)

> From my testing ya, if you want I can explicitly NULL it on each failure
> though just in case.

No need to perform an explicit NULL check if you put in the succeeded check. LocalFree works when passed NULL.

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Widget

Implemented sec review comment of wrapper assignments in succeeded.

Comment 35

[Brian R. Bondy [:bbondy]]

Updated component because I couldn't select the target milestone field.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c75172789a3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/19eed0fb290e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b855b1ac3f14

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b855b1ac3f14
https://hg.mozilla.org/mozilla-central/rev/19eed0fb290e
https://hg.mozilla.org/mozilla-central/rev/c75172789a3c

Comment 37

[juan becerra [:juanb]]

Tested on 2013-03-20 using Nightly built from http://hg.mozilla.org/mozilla-central/rev/8156df33b757
- I've tested basic setup on both Firefox for desktop and Metro and the information I tested like bookmarks, history, passwords, some simple forms, history and open tabs, and an add-on, and it seems to be working. I will spend a lot more time making sure everything that can be sync'ed is in sync before verifying it, however.
- Filed bug 853182 for the case where simple Bookmarks are not listed in the other Firefox after sync'ing.

Comment 38

[Brian R. Bondy [:bbondy]]

Juan this story is only relating to the setup experience from desktop firefox. It is not related to the actual syncing functionality. I'll move bug 853182 to the bookmarks syncing story.

Comment 39

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Story" for iteration #8 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-06-17-03-11-12-mozilla-central/

- Went through the original "Story" that has been attached above without any issues
- Paired a Desktop Firefox on Windows 8 and Firefox Metro on a second Windows 8 machine without any issues
- Paired two different Firefox Metro's on two different machines without any issues
- Ensured that all the paired devices are under the same account
- Ensured that the experience was smooth and all the progress bars worked without any issues
- Ensured the correct information was being listed under the "Sync" flyout for both Firefox Metro machines

Note:

Didn't test syncing as those are separate "Story's"

Comment 40

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Story" for iteration #9 testing with some issues. Used the following build:

- Went through the test cases added in comment 39 without any issues

Notes:

Found Bug 889707 when going through the following Story.

Comment 41

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in user story and got expected result.