455008 - EnableLUA=1 on XP causes IE cookie migration to fail

Status: NEW

(Firefox :: Migration, defect)

Description

[Mark Smith [:mcs]]

Attachment: proposed fix: only append "\Low" on Vista systems.

On most Windows XP systems, the following registry key is not present (as far as I know, it is only relevant on Vista systems):

  HKLM\Software\Microsoft\Windows\CurrentVersion\Policies\System\EnableLUA

If this key is set, the IE migration code looks for cookies in the ...\Cookies\Low directory.  On one of my XP systems, EnableLUA was set to 1 for some reason, which caused import to fail.

I am attaching a proposed fix.

Comment 1

[Mark Smith [:mcs]]

Attachment: -w version of patch, for reference

As you can see from this -w diff, the only change is to add a Windows version check.

Comment 2

[Mark Smith [:mcs]]

FYI, I cannot find any conclusive information on the net that says what the EnableLUA registry key means in XP (it seems to always be mentioned in relation to Vista).  I do not know how it got set on my system (it is not present on two other XP systems I checked).

The code I bracketed with the OS version check was introduced by the fix for bug 369910.

Comment 3

[Robert Strong (they/them - no direct email)]

It might be better to just fallback to using the non EnableLUA location when a failure occurs when EnableLUA is set to 1 since GetVersionEx will return the OS set via compatibility for the binary which could for example return XP on a Vista system.

http://msdn.microsoft.com/en-us/library/ms724451(VS.85).aspx

Comment 4

[Mark Smith [:mcs]]

(In reply to comment #3)
> It might be better to just fallback to using the non EnableLUA location when a
> failure occurs when EnableLUA is set to 1 since GetVersionEx will return the OS
> set via compatibility for the binary which could for example return XP on a
> Vista system.

I did not realize that (makes sense though).  Do you mean look for the Cookies\Low directory, and if it does not exist, use Cookies?

Comment 5

[Robert Strong (they/them - no direct email)]

(In reply to comment #4)
> I did not realize that (makes sense though).  Do you mean look for the
> Cookies\Low directory, and if it does not exist, use Cookies?
For the case EnableLUA is 1 Yes though I'd like Jim Mathies' input on this since I believe he's worked on this code.

Comment 6

[Jim Mathies [:jimm]]

EnableLUA might indicates UAC is enabled (it could be set on XP through group policy as well though) but I don't think it's a reliable indicator that the current user is operating ie under protected mode. UAC and ie's protected mode are two different things. I can have UAC turned on while protected is off.

I'm not sure we should migrate by default from both locations, mixing cookies from both modes might represent a user privacy issue. If I remember correctly I had a discussion with Daniel about this a long time ago. We may have decided it was safe to do. I'll dig up the mail on it and post back.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to comment #6)
> EnableLUA might indicates UAC is enabled (it could be set on XP through group
> policy as well though) but I don't think it's a reliable indicator that the
> current user is operating ie under protected mode. UAC and ie's protected mode
> are two different things. I can have UAC turned on while protected is off.
> 
> I'm not sure we should migrate by default from both locations, mixing cookies
> from both modes might represent a user privacy issue. If I remember correctly I
> had a discussion with Daniel about this a long time ago. We may have decided it
> was safe to do. I'll dig up the mail on it and post back.
I wasn't suggesting that we migrate from both locations but if it is safe / a decent user experience then sure.

Comment 8

[Jim Mathies [:jimm]]

>I wasn't suggesting that we migrate from both locations but if it is safe / a
>decent user experience then sure.

Yeah sry that was just my thinking out loud. 

I remember working on this stuff last spring. The EnableLUA thing was a concession since we didn't have the ability to figure out what ie was running under. I misread your first patch thinking that was something you were adding.

So, all the way around again, I'm fine with using the GetVersionEx stuff to test 
for vista vs. XP. I think the compat stuff is going to be pretty minor.

Comment 9

[Jim Mathies [:jimm]]

>I did not realize that (makes sense though).  Do you mean look for the
>Cookies\Low directory, and if it does not exist, use Cookies?

The original conclusion was that we should not do this due to privacy concerns. Pretty much do our best to find the right directory as best we can, for the most common cases.

Comment 10

[Robert Strong (they/them - no direct email)]

How about we try to use the Low directory when EnableLUA is 1 and if the directory doesn't exist fallback to the Cookies directory. This would be possible without using GetVersionEx which may return XP on a Vista system for example.

http://msdn.microsoft.com/en-us/library/ms724451(VS.85).aspx

Comment 11

[Jim Mathies [:jimm]]

It's a more complex patch but doesn't change the behavior on vista. Is the compat stuff really that big an issue?

Comment 12

[Robert Strong (they/them - no direct email)]

I'm not sure how it would be more complex? It seems like it would be possible to just use the Cookies directory when EnableLUA the Low sub-directory to the Cookies directory doesn't exist? It seems that using GetVersionEx would be more complex and that they are both rather trivial and the one case where this would be better is when the user sets compatibility mode on the binary. Either way is fine with me since users seldom set compat on the binary but it seems like it would be simpler to do it as I just outlined.

Comment 13

[Mark Smith [:mcs]]

Is it always safe from a security point of view to look in Cookies if Cookies\Low does not exist?  In other words, does Vista ensure that Cookies\Low exists when running IE in protected mode (even if no cookies have ever been saved)?  Probably it does, but I don't know for sure.

Comment 14

[Jim Mathies [:jimm]]

> Either way is
>fine with me since users seldom set compat on the binary but it seems like it
>would be simpler to do it as I just outlined.

I'm fine with either. So basically 

- create a the low dir string
- check exists, if it dows, import, exit
- if not, fall back on the regular dir string, import, exit

 
>Is it always safe from a security point of view to look in Cookies if
>Cookies\Low does not exist? 

Good question, we'll have to check a fresh  install of ie7 to see...

Comment 15

[Robert Strong (they/them - no direct email)]

(In reply to comment #13)
> Is it always safe from a security point of view to look in Cookies if
> Cookies\Low does not exist?  In other words, does Vista ensure that Cookies\Low
> exists when running IE in protected mode (even if no cookies have ever been
> saved)?  Probably it does, but I don't know for sure.

Just launching IE in protected mode on Vista will create a Cookies\Low directory with an index.dat file.

I think that any security issues that come up in relation to cookie migration would be due to using EnableLUA to determine if protected mode is on but there doesn't seem to be a reliable way to determine if it is.

(In reply to comment #14)
> > Either way is
> >fine with me since users seldom set compat on the binary but it seems like it
> >would be simpler to do it as I just outlined.
> 
> I'm fine with either. So basically 
> 
> - create a the low dir string
> - check exists, if it dows, import, exit
> - if not, fall back on the regular dir string, import, exit
While keeping the check for EnableLUA?

> >Is it always safe from a security point of view to look in Cookies if
> >Cookies\Low does not exist? 
> 
> Good question, we'll have to check a fresh  install of ie7 to see...
btw: I didn't check this with a fresh Vista install or with a fresh profile... just deleted the low dir and launched IE.

Comment 16

[Jim Mathies [:jimm]]

>> - create a the low dir string
>> - check exists, if it dows, import, exit
>> - if not, fall back on the regular dir string, import, exit
>While keeping the check for EnableLUA?

yep, wfm. Mark are you interested in tooling this up?

Comment 17

[Marco Bonardo [:mak]]

the new migrator uses userCanElevate to determine if using /Low or not, generally users are discouraged from running without UAC or to run apps with admin rights, so my assumption is that it may be enough.
I'm leaving this open just in case someone wants to comment whether that's enough or we need stricter checks.