369910 - Cookies not imported from IE 7

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Kevin Brosnan]

Go to a site where cookies are used to store data http://arstechnica.com is a good example, use the buttons to change font size and the background to black. Start up Firefox for the first time, go through the migration wizard select IE, browse to arstechnica notice that none of the font or color settings were imported.

Comment 1

[Carsten Book [:Tomcat]]

confirmed on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/2007020823 Firefox/2.0.0.2 - i see the same as reported on comment #0

Comment 2

[Daniel Veditz [:dveditz]]

Not blocking 1.8 branch, but probably so for FF3. When the fix is done for FF3 we can think about back-porting to the branch.

cc'ing mconnor to find a victim to work on IE7 migration issues

Comment 3

[Mike Connor [:mconnor]]

IE7 Migration -> jmathies

Comment 4

[Jim Mathies [:jimm]]

I'm not convinced this is a bug. The reason we don't see IE7's cookies is due to the fact that IE7 runs in protected mode, in which case cookies are stored in a different location from instances running at a medium integrity level. 

C:\Users\jim\AppData\Roaming\Microsoft\Windows\Cookies\ - medium
C:\Users\jim\AppData\Roaming\Microsoft\Windows\Cookies\Low - low

If Firefox was running at the same integrity level, the call in SpecialSystemDirectory.cpp - 

GetWindowsFolder(CSIDL_COOKIES, aFile);

would return the low integrity location. Loading low integrity cookies into a medium integrity app may constitute a security issue. I'd like to get some feedback from the security gurus on this before I add a "Low" import into things. 

Comment 5

[Jim Mathies [:jimm]]

Attachment: cookie import ie7 patch v.1

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 284183 [details] [diff] [review]
cookie import ie7 patch v.1

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

>   // find the cookies directory
>   NS_GetSpecialDirectory(NS_WIN_COOKIES_DIR, getter_AddRefs(cookiesDir));
>-  if (cookiesDir)
>+  if (cookiesDir) {
>+    // Check for Vista's UAC, if so, tack on a "Low" sub dir
>+    nsCOMPtr<nsIWindowsRegKey> regKey =
>+      do_CreateInstance("@mozilla.org/windows-registry-key;1");
>+    if (regKey) {
>+      nsAutoString registryPath;
>+      registryPath.AppendLiteral("Software\\Microsoft\\Windows\\CurrentVersion\\Policies\\System");

Use NS_NAMED_LITERAL_STRING?

>+      if (NS_SUCCEEDED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE,
>+                                    registryPath,
>+                                    nsIWindowsRegKey::ACCESS_QUERY_VALUE))) {
>+        PRUint32 value;
>+        if (NS_SUCCEEDED(regKey->ReadIntValue(NS_LITERAL_STRING("EnableLUA"),
>+          &value)) && value == 1) {

Wrapping is a bit strange... How about something like:
if (NS_SUCCEEDED(regKey->ReadIntValue(NS_LITERAL_STRING("EnableLUA"),
                                      &value)) &&
    value == 1) {

>+          if (path.Length() > 3) {
>+            dir.Append(Substring(path, path.Length()-3));
>+            if (dir.Equals(NS_LITERAL_STRING("Low")) == PR_FALSE) {
>+                cookiesDir->Append(NS_LITERAL_STRING("Low"));
>+            }

Why not just use leafName instead of GetPath+Substring?

Should we be trying to import from both Cookies\ and Cookies\Low? I suppose that'd only be useful if we're "privileged" while LUA is enabled...

Comment 7

[Jim Mathies [:jimm]]

> Use NS_NAMED_LITERAL_STRING?
> Wrapping is a bit strange...
Will do.

> Why not just use leafName..
Ok, didn't realize that would return the ending dir.  

>Should we be trying to import from both Cookies\ and Cookies\Low? I suppose
>that'd only be useful if we're "privileged" while LUA is enabled...

There was a discussion about this. There are some merge issues and really, users aren't going to be switching back and forth between the two states. Most will have it on, some will have it off, so going with what is currently configured made better sense, while also keeping things simple.

Comment 8

[Jim Mathies [:jimm]]

Attachment: cookie import ie7 patch v.2

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 285008 [details] [diff] [review]
cookie import ie7 patch v.2

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

> nsIEProfileMigrator::CopyCookies(PRBool aReplace) 
> {
>   // IE cookies are stored in files named <username>@domain[n].txt
>   // (in <username>'s Cookies folder. isn't the naming redundant?)
>-  PRBool rv = NS_OK;
>+  nsresult rv = NS_OK;

I noticed this earlier, this code is definitely confused! :)

>+  if (cookiesDir) {
>+    // Check for Vista's UAC, if so, tack on a "Low" sub dir
>+    nsCOMPtr<nsIWindowsRegKey> regKey =
>+      do_CreateInstance("@mozilla.org/windows-registry-key;1");
>+    if (regKey) {
>+      nsAutoString registryPath;
>+      NS_NAMED_LITERAL_STRING(regPath,"Software\\Microsoft\\Windows\\CurrentVersion\\Policies\\System");
>+      registryPath.Append(regPath);

You can use regPath directly in the Open() call and remove registryPath entirely. 

>+      if (NS_SUCCEEDED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE,
>+                                    registryPath,
>+                                    nsIWindowsRegKey::ACCESS_QUERY_VALUE))) {
>+        nsresult rv;

Redeclaring rv... Intentionally? Makes the code hard to read. Given that the rest of this file doesn't obey the 80 character line limit, might as well just do NS_SUCCEEDED(...->ReadIntValue...) and not worry about line length, or use wrapping from my previous comment.

>+        PRUint32 value;
>+        rv = regKey->ReadIntValue(NS_LITERAL_STRING("EnableLUA"), &value);
>+        if (NS_SUCCEEDED(rv) && value == 1) {
>+          nsAutoString dir;
>+          // For cases where we are running under protected mode, check
>+          // cookiesDir for the Low sub directory. (Simpler than using
>+          // process token calls.)
>+          cookiesDir->GetLeafName(dir);
>+          if (dir.Equals(NS_LITERAL_STRING("Low")) == PR_FALSE) {

I'm not sure I understand... When would dir be equal to "Low"? I think you should clone cookiesDir, append "Low" unconditionally, then check Exists() to determine whether to use the clone or the original cookiesDir.

>+              cookiesDir->Append(NS_LITERAL_STRING("Low"));
>+          }

Nit: no brackets around single-line then-clause.

Comment 10

[Jim Mathies [:jimm]]

>You can use regPath directly in the Open() call and remove registryPath
>entirely.

doh!

>Redeclaring rv... Intentionally?

I didn't want to mess with it's default value. I'll remove it.

>I'm not sure I understand... When would dir be equal to "Low"?

The point here is that cookiesDir may already point to the Low subdir, if and when Firefox, or some derivative is running at low integrity. The system will return the appropriate cookies dir depending on the integrity of the app that makes the call in the special system directories code. This is basically a safety catch, insuring that when and if we implement protected mode, the code doesn't break. If we just check the registry flag and tack on Low as a result, we would end up with .../Low/Low. Maybe there's a better way to do it but essentially this patch follows the logic:

check the protected mode flag
-> True (ie7 cookies are in Low)
   get the cookies dir we are given, ends with Low? 
   -> True (we are running at low integrity, path is good)
   -> False (tack on Low sub dir to get ie7's cookies)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> The point here is that cookiesDir may already point to the Low subdir, if and
> when Firefox, or some derivative is running at low integrity.

Ah, right, of course. I wish there was a better way to detect that than to do a string compare on the directory name we get back...

Comment 12

[Jim Mathies [:jimm]]

Attachment: cookie import ie7 patch v.3

updated

Comment 13

[Jim Mathies [:jimm]]

>Ah, right, of course. I wish there was a better way to detect that than to do a
>string compare on the directory name we get back...

Well, there is, but it's complex process token type stuff. Probably 10-15 lines of code. afaict, just checking Low is going to work ok too. I'm not aware of a way to change the cookies location. There might be some obscure roaming profiles related thing that breaks this, but I would think we could address that down the road when and if it arises.

Comment 14

[Mike Connor [:mconnor]]

can wait till M10 to get this if it doesn't get in.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 285128 [details] [diff] [review]
cookie import ie7 patch v.3

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

> nsIEProfileMigrator::CopyCookies(PRBool aReplace) 

>+            if (dir.Equals(NS_LITERAL_STRING("Low")) == PR_FALSE)

nit: if (!dir.EqualsLiteral("Low"))

>+  if (cookiesDir) {

...

>+    if (!cookieFiles)
>+      return NS_ERROR_FAILURE;
>+  }

Just noticed this: leaving this inside the "if (cookiesDir)" block will result in a null deref of cookieFiles later on in the function if getting cookiesDir somehow fails. Just leave it outside the block like it was before.

r=me with those two comments addressed.

Comment 16

[Jim Mathies [:jimm]]

Attachment: cookie import ie7 patch v.4

ok, nits addressed, recompiled and tested one more time to be sure everything's correct.

Comment 17

[Reed Loden [:reed]]

Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v  <--  nsIEProfileMigrator.cpp
new revision: 1.66; previous revision: 1.65
done

Comment 18

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102407 Minefield/3.0a9pre. I tested by clearing my Firefox cookies and then building up a cookie history in IE, and then importing.