Closed Bug 399206 Opened 13 years ago Closed 13 years ago

Remove password and form data import options for Internet Explorer 7

Categories

(Firefox :: Migration, defect, P2)

x86
Windows Vista
defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta2

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 3 obsolete files)

Remove Password and form data import options for Internet Explorer 7 profile imports on Windows Vista. See bug 355538 for details.
Attached patch import patch v.1 (obsolete) — Splinter Review
Assignee: jmathies → nobody
Component: OS Integration → Migration
QA Contact: os.integration → migration
Status: NEW → ASSIGNED
Assignee: nobody → jmathies
Status: ASSIGNED → NEW
Summary: Remove Password and form data import options for Internet Explorer 7 → Remove password and form data import options for Internet Explorer 7
Attachment #284370 - Flags: review?(cbiesinger)
Status: NEW → ASSIGNED
Attached patch import patch v.2 (obsolete) — Splinter Review
Minor update, I noticed a badly formatted if block.
Attachment #284370 - Attachment is obsolete: true
Attachment #284370 - Flags: review?(cbiesinger)
Attachment #284981 - Flags: review?(cbiesinger)
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Attachment #284981 - Flags: review?(cbiesinger) → review?(robert.bugzilla)
Shifted review to Rob, biesi's crazy busy.  Want to get this in for M10
Target Milestone: --- → Firefox 3 M10
Comment on attachment 284981 [details] [diff] [review]
import patch v.2

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v
>retrieving revision 1.65
>diff -u -p -8 -r1.65 nsIEProfileMigrator.cpp
>--- browser/components/migration/src/nsIEProfileMigrator.cpp	30 Aug 2007 03:01:07 -0000	1.65
>+++ browser/components/migration/src/nsIEProfileMigrator.cpp	15 Oct 2007 19:08:34 -0000
>...
>@@ -434,20 +436,27 @@ nsIEProfileMigrator::Migrate(PRUint16 aI
>   return rv;
> }
> 
> NS_IMETHODIMP
> nsIEProfileMigrator::GetMigrateData(const PRUnichar* aProfile, 
>                                     PRBool aReplace,
>                                     PRUint16* aResult)
> {
>-  // There's no harm in assuming everything is available.
>+  if (TestForIE7()) {
>+  // IE7 and up store form data and passwords in an unrecoverable
>+  // way, preventing us from importing this data.
>+  *aResult = nsIBrowserProfileMigrator::SETTINGS | nsIBrowserProfileMigrator::COOKIES | 
>+             nsIBrowserProfileMigrator::HISTORY | nsIBrowserProfileMigrator::BOOKMARKS;
>+  } else {
nit: style for this file is
  }
  else {
with only one exception

>   *aResult = nsIBrowserProfileMigrator::SETTINGS | nsIBrowserProfileMigrator::COOKIES | 
>              nsIBrowserProfileMigrator::HISTORY | nsIBrowserProfileMigrator::FORMDATA |
>              nsIBrowserProfileMigrator::PASSWORDS | nsIBrowserProfileMigrator::BOOKMARKS;
>+  }
nit: indentation

>+
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsIEProfileMigrator::GetSourceExists(PRBool* aResult)
> {
>   // IE always exists. 
>   *aResult = PR_TRUE;
>@@ -508,16 +517,91 @@ nsIEProfileMigrator::nsIEProfileMigrator
> {
>   mObserverService = do_GetService("@mozilla.org/observer-service;1");
> }
> 
> nsIEProfileMigrator::~nsIEProfileMigrator() 
> {
> }
> 
>+// Test used in detecting Internet Explorer 7 prior to presenting
>+// import options.
>+PRBool 
>+nsIEProfileMigrator::TestForIE7()
>+{
>+  nsCOMPtr<nsIWindowsRegKey> regKey =  
>+    do_CreateInstance("@mozilla.org/windows-registry-key;1"); 
>+  if (!regKey)  
>+    return PR_FALSE;  
>+
>+  // HKEY_CLASSES_ROOT\Applications\iexplore.exe 
>+  nsAutoString applicationsPath; 
>+  applicationsPath.AppendLiteral("Applications\\iexplore.exe\\shell\\open\\command"); 
>+
>+  nsresult rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT, 
>+                             applicationsPath, 
>+                             nsIWindowsRegKey::ACCESS_QUERY_VALUE); 
>+  if (NS_FAILED(rv)) 
>+    return PR_FALSE; 
I think the existing format used in this file is a tad simpler

  NS_NAMED_LITERAL_STRING(key,
      "Applications\\iexplore.exe\\shell\\open\\command");
  if (!regKey ||
      NS_FAILED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
                             key, nsIWindowsRegKey::ACCESS_QUERY_VALUE)))
    return PR_FALSE;

>+
>+  nsAutoString iePath;
>+  if (NS_FAILED(regKey->ReadStringValue(EmptyString(), iePath)))
>+    return PR_FALSE; 
>+
>+  // Replace embedded environment variables. 
>+  PRUint32 bufLength =  
>+    ::ExpandEnvironmentStringsW(iePath.get(), 
>+                               L"", 0); 
>+  if (bufLength == 0) // Error 
>+    return PR_FALSE; 
>+
>+  nsAutoArrayPtr<PRUnichar> destination(new PRUnichar[bufLength]); 
>+  if (!destination) 
>+    return PR_FALSE;
nit: add a blank line here

>+  if (!::ExpandEnvironmentStringsW(iePath.get(), 
>+                                   destination, 
>+                                   bufLength)) 
>+    return PR_FALSE; 
>+
>+  iePath = destination; 
>+
>+  if (StringBeginsWith(iePath, NS_LITERAL_STRING("\""))) {
>+    iePath.Cut(0,1);
>+    PRUint32 index = iePath.FindChar('\"', 0);
>+    if (index > 0)
>+      iePath.Cut(index,iePath.Length());
>+  }
>+
>+  nsCOMPtr<nsILocalFile> lf; 
>+  NS_NewLocalFile(iePath, PR_TRUE, getter_AddRefs(lf)); 
>+
>+  if (!lf)
>+   return PR_FALSE;
nit: you don't add an extra line when you use an if statement to check the previous call.

r=me with these nits picked. Thanks Jim!
Attachment #284981 - Flags: review?(robert.bugzilla) → review+
Priority: P3 → P2
Attached patch import patch v.3 (obsolete) — Splinter Review
Attachment #284981 - Attachment is obsolete: true
Attachment #288566 - Flags: review?(gavin.sharp)
Comment on attachment 288566 [details] [diff] [review]
import patch v.3

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

>+nsIEProfileMigrator::TestForIE7()
>+{
>+  nsCOMPtr<nsIWindowsRegKey> regKey =  
>+    do_CreateInstance("@mozilla.org/windows-registry-key;1"); 
>+  if (!regKey)  
>+    return PR_FALSE;  
>+
>+  NS_NAMED_LITERAL_STRING(key,
>+      "Applications\\iexplore.exe\\shell\\open\\command");
>+  if (!regKey ||
>+      NS_FAILED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                             key, nsIWindowsRegKey::ACCESS_QUERY_VALUE)))

The !regKey is redundant, already checked it above.

>+  nsCOMPtr<nsILocalFile> lf; 
>+  NS_NewLocalFile(iePath, PR_TRUE, getter_AddRefs(lf)); 
>+  if (!lf)
>+   return PR_FALSE;
>+
>+  nsCOMPtr<nsILocalFileWin> lfw = do_QueryInterface(lf); 
>+  if (!lfw)
>+   return PR_FALSE;

do_QueryInterface is null-safe, so you can remove the first null check here.

Seems kinda icky to have to do string parsing on the FileVersion field, but I guess there isn't really a better way? I haven't tested this on a machine with IE6, I'm assuming you have. r+ with those addressed.
Attachment #288566 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need final patch for landing]
Jim, when can we expect a new patch?
sorry, this one slipped under the radar. I'll get to it today.
>Seems kinda icky to have to do string parsing on the FileVersion field, but I
>guess there isn't really a better way?

Seems crazy but I've actually had this method rec'd to me by MS at a previous job. Should be reliable. 

> I haven't tested this on a machine with
> IE6, I'm assuming you have. r+ with those addressed.

Yes, checked in IE6.
Attached patch import patch v.4Splinter Review
Attachment #288566 - Attachment is obsolete: true
Whiteboard: [has patch][need final patch for landing] → checkin-needed
Please note that checkin-needed is a keyword, not part of the status whiteboard. :)
Keywords: checkin-needed
Whiteboard: checkin-needed
Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v  <--  nsIEProfileMigrator.cpp
new revision: 1.68; previous revision: 1.67
done
Checking in browser/components/migration/src/nsIEProfileMigrator.h;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.h,v  <--  nsIEProfileMigrator.h
new revision: 1.15; previous revision: 1.14
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Couldn't we recover IE7 passwords by pulling URLs out of IE's history and using them in a dictionary attack against the hashes?
Maybe, although between that only partially working (if history isn't complete) and the mess described in http://www.passcape.com/recovering_ie7_passwords.htm, I'm not sure how well that would work. :/
At least the passwords that are used often are likely to be in the history.
The other thing we could do, which would be considerably more work, is to consult the IE7 database ourselves as the user visits pages.
>Couldn't we recover IE7 passwords by pulling URLs out of IE's history and using
>them in a dictionary attack against the hashes?

That's really the only way these applications are doing it. It looks like the process is not exact, so some sort of UI would be needed for the user to confirm the imports.
We should move discussion on possible alternatives for IE7 password import to another bug.
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120405 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.