Closed Bug 831614 Opened 7 years ago Closed 7 years ago

Story - Set up Sync for both Firefoxes in desktop Firefox

Categories

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

22 Branch
x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 22

People

(Reporter: asa, Assigned: bbondy)

References

Details

(Whiteboard: feature=story c=Install_and_setup u=metro_desktop_user p=8)

Attachments

(4 files, 8 obsolete files)

5.18 KB, text/html
Details
3.67 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
7.10 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
13.20 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
No description provided.
Priority: -- → P2
Whiteboard: c=Install_and_setup u= p= → c=Install_and_setup u=metro_desktop_user p=
Summary: Set up Sync for both Firefoxes in desktop Firefox → Story – Set up Sync for both Firefoxes in desktop Firefox
Whiteboard: c=Install_and_setup u=metro_desktop_user p= → c=Install_and_setup u=metro_desktop_user feature=story
Depends on: 809171
Depends on: 798841
OS: Windows 8 → Windows 8 Metro
Whiteboard: c=Install_and_setup u=metro_desktop_user feature=story → feature=story c=Install_and_setup u=metro_desktop_user p=0
Depends on: 836393
bbondy or jimm might be able to provide an estimate for this story.
Flags: needinfo?(netzen)
Whiteboard: feature=story c=Install_and_setup u=metro_desktop_user p=0 → feature=story c=Install_and_setup u=metro_desktop_user p=13
Provided the info just forgot to reset the flag.  Apparently it doesn't auto clear the flag when you only change whiteboard tags.
Flags: needinfo?(netzen)
Summary: Story – Set up Sync for both Firefoxes in desktop Firefox → Story - Set up Sync for both Firefoxes in desktop Firefox
Component: General → Metro Operations
Product: Firefox for Metro → Tracking
Version: unspecified → ---
Assignee: nobody → netzen
Blocks: metrov1it3
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
No longer depends on: 836393
Depends on: 826396
Attached patch Patch v1 - Widget (obsolete) — Splinter Review
Attachment #721523 - Flags: review?(jmathies)
Attached patch Patch v1 - Metro sync (obsolete) — Splinter Review
Attachment #721524 - Flags: review?(mbrubeck)
Attachment #721525 - Flags: review?(neil)
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.
Flags: needinfo?(bsmith)
Flags: needinfo?(imelven)
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.
Attachment #721524 - Flags: review?(mbrubeck) → review+
Attached patch Patch v2 - Metro sync (obsolete) — Splinter Review
Implemented review nit. Carrying forward r+.
Attachment #721524 - Attachment is obsolete: true
Attachment #721698 - Flags: review+
Comment on attachment 721525 [details] [diff] [review]
Patch v1 - Browser sync UI changes

Sorry, I'm not a peer.
Attachment #721525 - Flags: review?(neil)
Attachment #721525 - Flags: review?(jAwS)
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.
Attachment #721525 - Flags: review?(jAwS) → review+
Story being returned to the Product Backlog until formal security reviews have been completed.
Blocks: metrov1backlog
No longer blocks: metrov1it3
No longer depends on: 809171
Whiteboard: feature=story c=Install_and_setup u=metro_desktop_user p=13 → feature=story c=Install_and_setup u=metro_desktop_user p=8
Talked to curtisk and he suggested to mark this as sec-review? and they will triage it for a sec code review.
Flags: needinfo?(imelven)
Flags: needinfo?(curtisk)
Flags: needinfo?(bsmith)
Flags: needinfo?(curtisk)
Details for sec review in Comment 6.
Flags: sec-review?(curtisk)
chaning flag to <blank> so it goes to security triage on 2013.03.13
Flags: sec-review?(curtisk) → sec-review?
changing flag to <blank> so it goes to security triage on 2013.03.13
Implemented review comments, carrying forward r+.
Attachment #721525 - Attachment is obsolete: true
Attachment #722858 - Flags: review+
Attachment #721698 - Attachment is obsolete: true
Attachment #722859 - Flags: review+
(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.
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 metro@mozilla.org for a discussion.
Duplicate of this bug: 826396
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.
Attachment #721523 - Flags: review?(jmathies) → review+
Attached patch Patch v2 - Widget (obsolete) — Splinter Review
Implemented review comments. Carried forward r+.
Attachment #721523 - Attachment is obsolete: true
Attachment #723311 - Flags: review+
This is ready for landing as long as security gives the OK.
(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.
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.
Attachment #724489 - Flags: review?(ywang)
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.
Blocks: metrov1it4
No longer blocks: metrov1backlog
Info from Yuan via IRC:
"Also sync with Windows 8 style $shortBrand"
Attachment #724489 - Flags: review?(ywang)
Flags: sec-review? → sec-review?(dchan+bugzilla)
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
Updated sync checkbox text as per Yuan's suggestion and associated accelerator key.
Attachment #722858 - Attachment is obsolete: true
Attachment #725054 - Flags: review+
Attachment #724489 - Attachment is obsolete: true
Attached patch Patch v3 - Widget (obsolete) — Splinter Review
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.
Attachment #723311 - Attachment is obsolete: true
Attachment #725055 - Flags: review+
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.
> 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.
(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.
Flags: sec-review?(dchan+bugzilla) → sec-review+
Implemented sec review comment of wrapper assignments in succeeded.
Attachment #725055 - Attachment is obsolete: true
Attachment #725198 - Flags: review+
Component: Metro Operations → Sync
Product: Tracking → Firefox for Metro
Target Milestone: --- → Firefox 22
Version: --- → 22 Branch
Flags: needinfo?(jbecerra)
QA Contact: jbecerra
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.
Depends on: 853182
Flags: needinfo?(jbecerra)
Depends on: 856048
Depends on: 856049
Depends on: 856052
No longer depends on: 856048
No longer depends on: 856052
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.
No longer depends on: 853182
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"
Depends on: 889707
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.
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.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.