Closed Bug 682069 Opened 13 years ago Closed 9 years ago

Password Import from IE not available for HTML form authentication

Categories

(Firefox :: Migration, defect)

Product:
Firefox
Component:
Migration
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
major
Points:
8

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 43
Iteration:
43.1 - Aug 24
Tracking Flags:
Tracking Status
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 --- verified

People

(Reporter: asa, Assigned: rchtara)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

MozReview Request: Bug 682069 - Password Import from IE not available
40 bytes, text/x-review-board-request
rchtara
: review+
rchtara
: review+
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
Details 
Firefox nightly build does not offer to import my passwords from IE during first-run migration or from the Import Wizard. 

We used to do this and not doing this (if that's what's happened here) is kind of scary. A smooth migration is one of the key requirements to converting IE users to Firefox. 

Tested with today's nightly build on XP importing from IE8. Juan, can you have someone investigate this and bug 591884? These seem like unacceptable regressions to me. Am I missing something? 

Have we also regressed on importing other browsers? On other platforms?
Seems cookies fail to import here as well. So, as far as I can tell we're only importing history from IE when we should be importing Passwords, Cookies, Bookmarks, Settings, and History.
Broken in Firefox 6 as well (presumably further back too)
Keywords: regression, regressionwindow-wanted
OS: Windows XP → All
Version: 8 Branch → Trunk
I just tested this quickly in a Win XP VM. If I kill all my profiles and run the migration wizard I see the same thing Asa does. But if go to Bookmarks | Show Bookmarks | Import and Backup | Import Data from another browser I get some of the data, but not all. I see cookies, bookmarks, history at least, but no passwords.

Using Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110825 Firefox/9.0a1 and testing with IE 8.
See bug 399206. We do not import passwords for IE7 & up. Might be worth doing some additional research, somebody might have found a way around the the password store problem.
We're seeing a big jump over the past day on this bug and other associated issues with IE migration. Here's the input search for only nightly users...it's not that big, but there is someone/some_people commenting on it:

 http://input.mozilla.com/en-US/?q=import&product=firefox&version=9.0a1&date_start=2011-09-13&date_end=
Whiteboard: [Input]
Also, reported by a user on input and bugzilla - https://bugzilla.mozilla.org/show_bug.cgi?id=677446
I believe password can decrypt using CryptUnprotectData from IE7 or later.
Summary: Password Import from IE not available → Password Import from IE (7 or later) not available
(In reply to Makoto Kato from comment #7)
> I believe password can decrypt using CryptUnprotectData from IE7 or later.

Found this for HTTP basic authentication passwords - 

http://securityxploded.com/iepasswordsecrets.php
http://www.securityfocus.com/archive/1/458115/30/0/threaded

Auto-complete still required the uri to decrypt the data.
(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Makoto Kato from comment #7)
> > I believe password can decrypt using CryptUnprotectData from IE7 or later.
> 
> Found this for HTTP basic authentication passwords - 
> 
> http://securityxploded.com/iepasswordsecrets.php
> http://www.securityfocus.com/archive/1/458115/30/0/threaded
> 
> Auto-complete still required the uri to decrypt the data.

We cannot import it since URI is hashed (SHA1?).  Browser migrator needs URI.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.26) Gecko/20120128 Firefox/3.6.26

Firefox 3.6.26 fails to import cookies from IE 8 on Windows XP.
Keywords: regression, regressionwindow-wanted
Summary: Password Import from IE (7 or later) not available → Password Import from IE not available
Whiteboard: [Input]
Depends on: 538654
Hardware: x86 → All
Blocks: iemigratewin
Assignee: nobody → rchtara
Status: NEW → ASSIGNED
Iteration: --- → 42.3 - Aug 10
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
OS: All → Windows
Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review?(MattN+bmo)
Could you please have a look to the patch.
Thaks
Summary: Password Import from IE not available → Password Import from IE not available for autocomplete passwords
Summary: Password Import from IE not available for autocomplete passwords → Password Import from IE not available for HTML form authentication
See Also: → 1191175
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review?(MattN+bmo)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review13693

::: browser/components/migration/IEProfileMigrator.js:92
(Diff revision 1)
> -  finalize: function CH_finalize() {
> +CtypesHelpers.prototype.finalize = function CH_finalize() {

Can you leave these methods as properties inside a CtypesHelpers.prototype object as it's more common for us (over CtypesHelpers.prototype.foo = …). Having the methods in a prototype object is much less verbose, especially with new method syntax and the indentation helps IMO.

::: browser/components/migration/IEProfileMigrator.js:379
(Diff revision 1)
> -    CtypesHelpers.initialize();
> +    this.helpers = new CtypesHelpers();

Nit: `helpers` is a bit vague so include ctypes in the name e.g. ctypesHelpers

::: browser/components/migration/IEProfileMigrator.js:505
(Diff revision 1)
> +    let nsIWindowsRegKey = Components.interfaces.nsIWindowsRegKey;

Nit: use `Ci`

::: browser/components/migration/IEProfileMigrator.js:501
(Diff revision 1)
> +    let historyEnumerator = Cc["@mozilla.org/profile/migrator/iehistoryenumerator;1"].
> +                            createInstance(Ci.nsISimpleEnumerator);
> +
> +    let crypto = new OSCrypto();
> +    let nsIWindowsRegKey = Components.interfaces.nsIWindowsRegKey;
> +    let key = Cc["@mozilla.org/windows-registry-key;1"].
> +              createInstance(nsIWindowsRegKey);
> +    key.open(nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kLoginsKey,
> +             nsIWindowsRegKey.ACCESS_READ);
> +    let logins = [];
> +    while (historyEnumerator.hasMoreElements()) {

Nit: move the historyEnumerator declaration/initialization right above the use in the while loop then add a comment above it explaining why we're looking through history.

::: browser/components/migration/IEProfileMigrator.js:515
(Diff revision 1)
> +      // like HTMLHelp and others.  Since we don't properly map handling for
> +      // all of them we just avoid importing them.

This comment should be tweaked a bit here: "importing them" isn't really right.

::: browser/components/migration/IEProfileMigrator.js:511
(Diff revision 1)
> +    while (historyEnumerator.hasMoreElements()) {

Idea: We could also (or instead if we guarantee History is imported first) go through existing Fx history in case there are saved logins for URLs in Fx history that expired from IE history.

::: browser/components/migration/IEProfileMigrator.js:520
(Diff revision 1)
> +      let url = entry.get("uri").spec;
> +      // remove the part after ? from the URL
> +      if (url.indexOf("?") != -1) {
> +        url = url.split("?")[0];
> +      }

There is probably a property better than `spec` that avoids having to do this custom URL handling. How about `prePath` or `prePath + path` (if you need the path)?

::: browser/components/migration/IEProfileMigrator.js:639
(Diff revision 1)
> +                         url: url,`

Stray "`"?

::: browser/components/migration/IEProfileMigrator.js:661
(Diff revision 1)
> +  _findValue(key, hashStr) {

Could you make it more clear what this method does? There is no indication that it's related to registry keys just by looking at it. Can you update the name and arguments to make them more clear?

::: toolkit/components/passwordmgr/OSCrypto_win.js:212
(Diff revision 1)
> +   * @param {?string} entropy - the entropy value of the decryption (could be null). Its value must
> +   * be the same as the one used when the data was encrypted.
> +   * @param {number} flags - the flags to setup CryptUnprotectData.
> +   * @returns {string} the decryption of the string.
>     */
> -  decryptData(array) {
> +  decryptData(data, entropy, flags) {

Is `flags` required? If so, put it before the optional `entryopy` argument.

Also indicate optional arguments with a default value in the definition and use the other syntax for JSDoc: @param {string} [entropy=null]

::: toolkit/components/passwordmgr/OSCrypto_win.js
(Diff revision 1)
> -
> -    this._functions.get("LocalFree")(outData.pbData);

Where did this move?

::: browser/components/migration/IEProfileMigrator.js:531
(Diff revision 1)
> +      try {
> +        data = crypto.decryptData(value, url, 1);
> +      } catch (e) {
> +        continue;

I think we should log with Cu.reportError here too

::: browser/components/migration/IEProfileMigrator.js:601
(Diff revision 1)
> +    let arr = [];
> +    for (let i = 0; i < data.length; i++) {
> +      arr.push(data.charCodeAt(i));
> +    }

I think this is the same as stringToArray isn't it?

::: toolkit/components/passwordmgr/OSCrypto_win.js:262
(Diff revision 1)
> -    for (let i = 0; i < string.length; i++) {
> -      decryptedData[i] = string.charCodeAt(i);
> +    for (let i = 0; i < data.length; i++) {
> +      decryptedData[i] = data.charCodeAt(i);
>      }

Use arrayToString?

::: browser/components/migration/IEProfileMigrator.js:573
(Diff revision 1)
> +  _extractDetails(data, url) {

Could you document what this method is doing? It's returning an array? Does `data` sometimes represent more than one login?

::: browser/components/migration/IEProfileMigrator.js:612
(Diff revision 1)
> +    let pInfo = 36;

What is `pInfo` and where is the value from? Please use more descriptive variable names and/or comments (especially for magic numbers).

::: browser/components/migration/IEProfileMigrator.js:508
(Diff revision 1)
> +    key.open(nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kLoginsKey,
> +             nsIWindowsRegKey.ACCESS_READ);

Note that this throws on Win8+ so when you add HTTP auth we will want to make sure that if this throws that it doesn't interfere with importing other passwords. I think we should rename this prototype to IE7FormPasswords and use a separate one for the Credential Manager ones. That helps keep them separate and isolate exceptions.

::: browser/components/migration/IEProfileMigrator.js:551
(Diff revision 1)
> +        login.init(loginInfo.url, loginInfo.url, null,

If IE doesn't store a form submit URL, we should create these with a form submit URL of "" which is a wildcard otherwise we will import passwords that don't work if the form submit URL is on another origin.

::: browser/components/migration/IEProfileMigrator.js:660
(Diff revision 1)
> +  },
> +  _findValue(key, hashStr) {

Nit: add a new line

::: browser/components/migration/IEProfileMigrator.js:497
(Diff revision 1)
> +  get exists() true,

This should be implemented still if it's not too much work to implement or process. Can you simply check if there are any registry keys?

::: browser/components/migration/IEProfileMigrator.js:601
(Diff revision 1)
> +    let arr = [];
> +    for (let i = 0; i < data.length; i++) {
> +      arr.push(data.charCodeAt(i));
> +    }

stringToArray?

::: browser/components/migration/IEProfileMigrator.js:642
(Diff revision 1)
> +      let current = arr.slice(headerSize + 12 + offset);

magic number

::: browser/components/migration/IEProfileMigrator.js:657
(Diff revision 1)
> +      pInfo += 16;

magic number

::: browser/components/migration/IEProfileMigrator.js:648
(Diff revision 1)
> +        let tmpBuf = ctypes.char.array(1024)();

magic number

::: toolkit/components/passwordmgr/OSCrypto_win.js:48
(Diff revision 1)
> +    this._functions.set("CryptCreateHash",
> +                        this._libs.get("advapi32").declare("CryptCreateHash",

Perhaps we could have xpcshell unit tests to sanity check that this API works.

::: browser/components/migration/IEProfileMigrator.js:496
(Diff revision 1)
> +
> +  get exists() true,
> +

We should at least have an xpcshell unit test that ensure that this returns true (the password type exists) on appropriate Windows versions. That would alert us to the change in API that happened in Win8+

::: browser/components/migration/IEProfileMigrator.js:525
(Diff revision 1)
> +      let hashStr = crypto.cryptGetIELoginHash(url);

I think we should unit test this function by checking for a static result (assuming this doesn't vary by user account).

::: browser/components/migration/IEProfileMigrator.js:532
(Diff revision 1)
> +        data = crypto.decryptData(value, url, 1);

Please use a const for the magic number as the flag argument.

::: browser/components/migration/IEProfileMigrator.js:531
(Diff revision 1)
> +      try {
> +        data = crypto.decryptData(value, url, 1);

We should probably have a unit test for this too

::: browser/components/migration/ChromeProfileMigrator.js:388
(Diff revision 1)
> +                                                       null, 0),

Document the magic number or probably best to make it default to 0 and omit it.

::: toolkit/components/passwordmgr/OSCrypto_win.js:33
(Diff revision 1)
> +    this._functions.set("CryptAcquireContext",
> +                        this._libs.get("advapi32").declare("CryptAcquireContextW",

You should probably include the "W" in the name here. Are you properly handling wide characters?

::: toolkit/components/passwordmgr/OSCrypto_win.js:114
(Diff revision 1)
> -   * Decrypt an array of numbers using the windows CryptUnprotectData API.
> -   * @param {number[]} array - the encrypted array that needs to be decrypted.
> +   * Convert an array to a string.
> +   * @param {number[]} arr - the array that needs to be converted.

Describe the contents of the array e.g. only single byte characters.

::: toolkit/components/passwordmgr/OSCrypto_win.js:166
(Diff revision 1)
> +    if (!this._functions.get("CryptAcquireContext")(hProv.address(), null, null, 1, 0)) {

use constants for magic numbers

::: toolkit/components/passwordmgr/OSCrypto_win.js:172
(Diff revision 1)
> +      close.call(this, true);

Bind the `close` name function itself to `this` so you can just do `close(true);`
`
let close = function(contextOnly) {
…
}.bind(this);
`
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review?(MattN+bmo)
Attachment #8643453 - Flags: review?(dolske)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
https://reviewboard.mozilla.org/r/15075/#review13693

> There is probably a property better than `spec` that avoids having to do this custom URL handling. How about `prePath` or `prePath + path` (if you need the path)?

we need to remive eveythin  after ?
But path includes that part
I tried many things but i am unable to find something better.

> I think we should log with Cu.reportError here too

what we are doing here is trying to decrypt the current regestry value using the current url as key, we test with all the keys we have, so even in the normal case we are there is only one link that sceed and all the other one are going to fail, but it's not a problem.
We can report an error however when the number of logins we got is less then the number of values in the keys which means some urls are missings from the history

> Perhaps we could have xpcshell unit tests to sanity check that this API works.

This is going to be tested throught the getIELoginHash test.
This api alone doesnt do anthing, it needs to be used with other apis, and this  is what happpens in the cryptGetIELoginHash
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review14381

r=me ignoring the ctypes/crpyto API usage which dolske is reviewing.

::: browser/components/migration/IEProfileMigrator.js:493
(Diff revision 6)
> +      return (count > 0);

Nit: We don't normally use braces here.

::: browser/components/migration/IEProfileMigrator.js:516
(Diff revision 6)
> +    aCallback(true);
> +  },
> +  /**
> +   * Migrate the logins that were saved for the uris arguments.

Leave a new line between methods please

::: browser/components/migration/IEProfileMigrator.js:547
(Diff revision 6)
> +      // remove the part after ? from the URL
> +      if (url.indexOf("?") != -1) {
> +        url = url.split("?")[0];
> +      }

What about fragments (#)? Are they a problem too?  I always forget that a nsIURI path includes the query string. To avoid the custom parsing you can construct a `new URL(…).pathname`

::: browser/components/migration/IEProfileMigrator.js:557
(Diff revision 6)
> +      let hashStr = this._crypto.getIELoginHash(url);
> +      let value = this._findValueInRegistryKeyByName(key, hashStr);
> +      // if no value was found, the uri is skipped
> +      if (value == null) {
> +        continue;
> +      }
> +      let data;
> +      try {
> +        // the url is used as salt to decrypt the registry value
> +        data = this._crypto.decryptData(value, url, kCryptprotectUIForbidden);
> +      } catch (e) {
> +        continue;
> +      }
> +      // extract the login details from the decrypted data
> +      let newLogins = this._extractDetails(data, uri.prePath);
> +      // if at least a credential was found in the current data, successfullyDecryptedValues should
> +      // be incremented by one
> +      successfullyDecryptedValues += (newLogins.length == 0) ? 0 : 1;
> +      logins = logins.concat(newLogins);

It seems like this system dealing with system calls should probably be wrapped in a try…catch block so that an error with certain logins doesn't cause the rest of the import to fail.

::: browser/components/migration/IEProfileMigrator.js:581
(Diff revision 6)
> +      Cu.reportError("Firefox failed to decrypt and import some logins. " +

You generally shouldn't refer to the brand name "Firefox" from within code since there are versions that don't use the trademarked name.

::: browser/components/migration/IEProfileMigrator.js:587
(Diff revision 6)
> +    this._addLogins(logins);
> +    key.close();
> +    this._crypto.finalize();
> +    this.ctypesHelpers.finalize();

I probably would have added the logins individually during import so that if something throws we at least imported some logins. Is there a reason you're waiting to import?

::: browser/components/migration/IEProfileMigrator.js:605
(Diff revision 6)
> +                   loginInfo.username, loginInfo.password, "", "");
> +        login.QueryInterface(Ci.nsILoginMetaInfo);
> +        login.timeCreated = loginInfo.creation;
> +        login.timeLastUsed = loginInfo.creation;
> +        login.timePasswordChanged = loginInfo.creation;
> +

You're missing timesUsed. Do we default to 1?

::: browser/components/migration/IEProfileMigrator.js:627
(Diff revision 6)
> +   * Extract the details of one or more logins from the raw decrypted data.
> +   * @param {string} data - the decrypted data containing raw information.
> +   * @param {string} url - the URL of page where the login has occur.
> +   * @returns {Object[]} array of objects where each of them contains the username, password, URL,
> +   * and creation time representing all the logins found in the data arguments.
> +   */
> +  _extractDetails(data, url) {

Could you include the name of the Windows data type in the method name and description so people can look it up on MSDN?

::: browser/components/migration/tests/unit/test_IE_passwords.js:1
(Diff revision 6)
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Nit: remove the new lines in between here.

::: browser/components/migration/tests/unit/test_IE_passwords.js:5
(Diff revision 6)
> +XPCOMUtils.defineLazyModuleGetter(this, "WindowsRegistry",
> +                                  "resource://gre/modules/WindowsRegistry.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OSCrypto",
> +                                  "resource://gre/modules/OSCrypto.jsm");

Nit: remove the new lines in between here.

::: browser/components/migration/tests/unit/test_IE_passwords.js:13
(Diff revision 6)
> +const LOGINS_KEY = "Software\\Microsoft\\Internet Explorer\\IntelliForms\\Storage2";

Nit:
`const LOGINS_KEY = LOGINS_KEY_PARENT + "\\Storage2";`

::: browser/components/migration/tests/unit/test_IE_passwords.js:164
(Diff revision 6)
> +let key;

Please use more descriptive variable names. e.g. Storage2Key

::: browser/components/migration/tests/unit/test_IE_passwords.js:215
(Diff revision 6)
> +        case nsIWindowsRegKey.TYPE_STRING:
> +          key.writeStringValue(valueName, key.readStringValue(name));
> +          key.removeValue(name);
> +          break;
> +        case nsIWindowsRegKey.TYPE_BINARY:
> +          key.writeBinaryValue(valueName, key.readBinaryValue(name));
> +          key.removeValue(name);
> +          break;
> +        case nsIWindowsRegKey.TYPE_INT:
> +          key.writeIntValue(valueName, key.readIntValue(name));
> +          key.removeValue(name);
> +          break;
> +        case nsIWindowsRegKey.TYPE_INT64:
> +          key.writeInt64Value(valueName, key.readInt64Value(name));
> +          key.removeValue(name);

Does this test actually need to handle all of those types? Couldn't we only touch the relevant types?

::: browser/components/migration/tests/unit/test_IE_passwords.js:239
(Diff revision 6)
> +  Assert.equal(passwordManagerLogin.username, IELogin.username,
> +               "The two logins ID " + id + " have the same username");
> +  Assert.equal(passwordManagerLogin.password, IELogin.password,
> +               "The two logins ID " + id + " have the same password");
> +  Assert.equal(passwordManagerLogin.hostname, IELogin.hostname,
> +               "The two logins ID " + id + " have the same hostname");
> +  Assert.equal(passwordManagerLogin.formSubmitURL, IELogin.formSubmitURL,
> +               "The two logins ID " + id + " have the same formSubmitURL");
> +  Assert.equal(passwordManagerLogin.httpRealm, IELogin.httpRealm,
> +               "The two logins ID " + id + " have the same httpRealm");
> +  Assert.equal(passwordManagerLogin.usernameField, IELogin.usernameField,
> +               "The two logins ID " + id + " have the same usernameElement");
> +  Assert.equal(passwordManagerLogin.passwordField, IELogin.passwordField,
> +               "The two logins ID " + id + " have the same passwordElement");
> +  Assert.equal(passwordManagerLogin.timeCreated, IELogin.timeCreated,
> +               "The two logins ID " + id + " have the same timeCreated");
> +  Assert.equal(passwordManagerLogin.timeLastUsed, IELogin.timeLastUsed,
> +               "The two logins ID " + id + " have the same timeLastUsed");
> +  Assert.equal(passwordManagerLogin.timePasswordChanged, IELogin.timePasswordChanged,
> +               "The two logins ID " + id + " have the same timePasswordChanged");
> +  Assert.equal(passwordManagerLogin.timesUsed, IELogin.timesUsed,
> +               "The two logins ID " + id + " have the same timesUsed");

You could have done this in a loop iterating over property names to avoid the duplication. I guess you could also use nsILoginInfo.equals though it wouldn't tell you what was wrong (you could dump the stringified representation of both logins in that case though).

::: browser/components/migration/tests/unit/test_IE_passwords.js:263
(Diff revision 6)
> +function getFirstMigrator(type) {

getFirstResourceOfType?

::: browser/components/migration/tests/unit/test_IE_passwords.js:277
(Diff revision 6)
> +  var ioService = Cc["@mozilla.org/network/io-service;1"]
> +                  .getService(Ci.nsIIOService);
> +  return ioService.newURI(aURL, null, null);

`return Services.io.newURI(aURL, null, null);`

::: browser/components/migration/tests/unit/test_IE_passwords.js:283
(Diff revision 6)
> +  if (AppConstants.isPlatformAndVersionAtLeast("win", "8")) {
> +    return;

I believe this may fail since you need to have at least one check in a test. Can you check that .exists returns false before doing the return in the setup function. You can leave the others as-is.

::: browser/components/migration/tests/unit/xpcshell.ini:19
(Diff revision 6)
> +[test_IE_passwords.js]
> +skip-if = os != "win"

I think either this test file should focus just on IE7 password migration and be renamed to reflect that (e.g. test_IE_passwords_IE7.js) or the test functions in the file should include IE7 in the name.

::: toolkit/components/passwordmgr/OSCrypto_win.js:226
(Diff revision 6)
> +    if (!entropy) {
> +      entropyParam = null;
> +    } else {
> +      let entropyArray = this.stringToArray(entropy);

Reverse the order of the bodies of these blocks to get rid of the negative.
Attachment #8643453 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review+ → review?(MattN+bmo)
https://reviewboard.mozilla.org/r/15075/#review14381

> What about fragments (#)? Are they a problem too?  I always forget that a nsIURI path includes the query string. To avoid the custom parsing you can construct a `new URL(…).pathname`

good catch for the fragement: i didnt test is before but it s working.
The URL(…).pathname return "/" for "http://google.com"

> You're missing timesUsed. Do we default to 1?

yes it s. but I added a new comment

> Could you include the name of the Windows data type in the method name and description so people can look it up on MSDN?

it's not documented

> Does this test actually need to handle all of those types? Couldn't we only touch the relevant types?

Yes because we could overwite any values even if it has a diffrent type, so we need to backup and restore them
Attachment #8643453 - Flags: review?(dolske)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review14469

::: browser/components/migration/IEProfileMigrator.js:12
(Diff revision 7)
> +const kCryptprotectUIForbidden = 1;

Nit: kCryptProtectUIForbidden?

::: browser/components/migration/IEProfileMigrator.js:500
(Diff revision 7)
> +    let historyEnumerator = Cc["@mozilla.org/profile/migrator/iehistoryenumerator;1"].

How many history items (order of magnitude) do we expect a heavy IE user to have?

Looks like it defaults to 20 days (at least in IE11 on Win10), so that shouldn't be too bad? Not sure how that translates into overall performance, especially with crypto involved.

Does the enumerator give any particular order? If not, we should sort by time to ensure we the migrate a more recent password, if it was changed during the history timespan.

::: browser/components/migration/IEProfileMigrator.js:509
(Diff revision 7)
> +      if (["http", "https", "ftp", "file"].indexOf(uri.scheme) == -1) {

Seems like we should remove file:// from this list. At best that would be uncommon usage.

Is anything normalizing the case of IE history entries? EG, can an entry end up with "HTTP" or "Http" here?

::: browser/components/migration/IEProfileMigrator.js:530
(Diff revision 7)
> +             nsIWindowsRegKey.ACCESS_ALL);

Use .ACCESS_READ instead of ACCESS_ALL? We don't write to this key.

::: browser/components/migration/IEProfileMigrator.js:549
(Diff revision 7)
> +        if (url.indexOf("?") != -1) {

The indexOf() check is superfluous, the split should work the same even when there's no "?" present.

::: browser/components/migration/IEProfileMigrator.js:552
(Diff revision 7)
> +        // if the current url is already processed, it should be skipped

We could also extend this check to also include just the origin (protocol://host), since Firefox doesn't include the path in stored logins.

Oh, but... That won't work becase IE might have a login for one username saved at host.com/path1, and a different login at host.com/path2. Ideally we'd migrate both.

The Set filtering looks good though, because this should let us skip lots of duplicate history from search engines.

::: browser/components/migration/IEProfileMigrator.js:558
(Diff revision 7)
> +        let hashStr = this._crypto.getIELoginHash(url);

In addition to capitalization, does IE normalize default port on history entries? EG, can it store both "http://site.com/foo" and "https://site.com:80/foo"?

::: browser/components/migration/IEProfileMigrator.js:559
(Diff revision 7)
> +        let value = this._findValueInRegistryKeyByName(key, hashStr);

Nit: this helper isn't actually useful, it's just as short to write:

  if (!key.hasValue(hashStr)) {
      continue;
  }
  let value = key.readBinaryValue(hashStr);

::: browser/components/migration/IEProfileMigrator.js:572
(Diff revision 7)
> +        let newLogins = this._extractDetails(data, uri.prePath);

Nit: I'd suggest just passing in |uri|, and having extractDetails handle putting just .prePath into what it returns.

Nit: s/newLogins/ieLogins/

::: browser/components/migration/IEProfileMigrator.js:575
(Diff revision 7)
> +        successfullyDecryptedValues += (newLogins.length == 0) ? 0 : 1;

This would be more readable as:

if (newLogins.length)
  successfullyDecryptedValues++;

::: browser/components/migration/IEProfileMigrator.js:578
(Diff revision 7)
> +        Cu.reportError("The following error " + e + " has happened while importing the logins " +

Nit: I'd put the exception text at the end, because it could be long (and jsut plain interrupts the message.)

Cu.reportError("Error while importing logins for " + uri.spec + ": " + e);

::: browser/components/migration/IEProfileMigrator.js:603
(Diff revision 7)
> +    for (let loginInfo of logins) {

This naming is a little confusing, it's good practice to have the singular name be the thing you're iterating over.

I'd suggest |for (let ieLogin of ieLogins)|

::: browser/components/migration/IEProfileMigrator.js:608
(Diff revision 7)
> +                   loginInfo.username, loginInfo.password, "", "");

This isn't valid, and I'm actualy surprised if we don't reject it. Only one of the formSubmitURL or httpRealm parameters can be non-null.

Yeah, we do reject it: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#283

Is your code working? I'd expect every addLogin() call to fail.

::: browser/components/migration/IEProfileMigrator.js:616
(Diff revision 7)
> +        let logins = Services.logins.findLogins({}, login.hostname,

Also not great practice to have an inner variable mask a variable of the same name in the outer scope.

Not a big deal here since the usage is short, but I'd recommend using "existingLogins" here.

::: browser/components/migration/IEProfileMigrator.js:617
(Diff revision 7)
> +                                                login.formSubmitURL,

.formSubmitURL and .httpRealm are undefined here.

The IE logins imported here are only form logins, right? Not HTTP auth? I think you want to use an explicit empty string instead of .formSubmitURL, and a null for .httpRealm.

::: browser/components/migration/IEProfileMigrator.js:670
(Diff revision 7)
> +    let details = ["username", "password"];

|details| is unused, remove.

::: browser/components/migration/IEProfileMigrator.js:674
(Diff revision 7)
> +    // the bytes between the null and 3rd from the beginning are not needed

You can drop the "from the beginning" for brevity. And it's byte/offset 0, not "null".

So just "Bytes 0-3 are not needed."

Better still, can you note what's actually there, so it's clearer why it's not needed?

::: browser/components/migration/IEProfileMigrator.js:676
(Diff revision 7)
> +    let headerSize = toInt(memcpy(arr, 4, 4));

I'd recommend defining a couple of ctypes StructTypes here, instead of byte-banging each value.

Something like:

let loginItem = new ctypes.StructType("loginItem",
  [{"user_offset": ctypes.int32},
   {"loDateTime": ctypes.int32},
   {"hiDateTime": ctypes.int32}
   ...
   {"pass_offset": ctypes.int32}]);
   
let loginData = new ctypes.StructType("loginData",
  [{"foo": ctypes.int32},
   {"headerSize": ctypes.int32},
   {"bar": ctypes.int32},
   {"baz": ctypes.int32},
   {"dataMax": ctypes.int32}]);
   
Then you should be able to ctypes.cast data[x] to a loginData pointer, and access the fields directly. (I've hand waved over a few parts, but that's the idea.)

Not sure if it's possible to have loginData's defination include loginItem, I vaguely recall that it isn't for a couple of reasons. But that's fine, just do a cast to loginItem in each loop.

This also help with self-documenting the format of what's being used.

::: browser/components/migration/IEProfileMigrator.js:678
(Diff revision 7)
> +    // the bytes between the 12th and 19th from the beginning are not needed

Again, please note what 12-19 are. (Even if that means noting we don't know what they are!)

::: browser/components/migration/IEProfileMigrator.js:681
(Diff revision 7)
> +    let dataMax = toInt(memcpy(arr, 20, 4));

This would be clearer as just:

let numLogins = toInt(...) / 2;

::: browser/components/migration/IEProfileMigrator.js:683
(Diff revision 7)
> +    // iterating through all the logins: the number of logins is the dataMax / 2 as each username

Can IE store logins with only a username or password? How are those represented?

::: browser/components/migration/IEProfileMigrator.js:691
(Diff revision 7)
> +      // the bytes between the 12th and 15th from the beginning of information data are not needed

What are they?

::: browser/components/migration/IEProfileMigrator.js:701
(Diff revision 7)
> +      let currentUsername = arr.slice(headerSize + 12 + username_offset);

Presumably these values are null-terminated? Would be good to note.

::: browser/components/migration/IEProfileMigrator.js:706
(Diff revision 7)
> +      results[results.length - 1].password = this._crypto.arrayToString(toUni(currentPassword));

Don't probe back into what you just pushed -- push a single value once, containing all the data for a result.
I haven't finished going through the OSCrypto_win.js code yet -- will do that next.
Great, thanks a lot Dolske
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review?(dolske)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
https://reviewboard.mozilla.org/r/15075/#review14469

> How many history items (order of magnitude) do we expect a heavy IE user to have?
> 
> Looks like it defaults to 20 days (at least in IE11 on Win10), so that shouldn't be too bad? Not sure how that translates into overall performance, especially with crypto involved.
> 
> Does the enumerator give any particular order? If not, we should sort by time to ensure we the migrate a more recent password, if it was changed during the history timespan.

I'm not sure exactly but the order is probably 1000.
Yes, i think so, it s not going to be too bad, the users are not going to do the import frequently, so even if it's a bit slow, it doesn't matter.
The enumerator gives the the link in no particular order, not temporal not even alphabetical.
Doing the sorting and migrating the only the recent passwords is great idea, we can keep the last migration time of ie passwords and only migrates the links that are more recent then that. (we  still have at the first migration to import everything.) But we need to create an enumerator that give us the only the recent passwords
My guess we could dont worry a lot yet about the performance and if people start complaining we can try to fix that

> Seems like we should remove file:// from this list. At best that would be uncommon usage.
> 
> Is anything normalizing the case of IE history entries? EG, can an entry end up with "HTTP" or "Http" here?

Good catch, I didnt think about it, but luckily no: the urls in ie history are all in lowercase.

> We could also extend this check to also include just the origin (protocol://host), since Firefox doesn't include the path in stored logins.
> 
> Oh, but... That won't work becase IE might have a login for one username saved at host.com/path1, and a different login at host.com/path2. Ideally we'd migrate both.
> 
> The Set filtering looks good though, because this should let us skip lots of duplicate history from search engines.

There is another reason for that: ie use everything before ? to create the hash and as salt for encryption, so if we dont include everything we are going to miss the hash and not be able to decrypt data.
Cool , I was just follwing your advice :)

> In addition to capitalization, does IE normalize default port on history entries? EG, can it store both "http://site.com/foo" and "https://site.com:80/foo"?

Good catch too, I didnt think about it, but luckily:  No , both of them are stored as the same

> This isn't valid, and I'm actualy surprised if we don't reject it. Only one of the formSubmitURL or httpRealm parameters can be non-null.
> 
> Yeah, we do reject it: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#283
> 
> Is your code working? I'd expect every addLogin() call to fail.

no it's working, I had problem at the beginning when I used null  for both formSubmitURL and httpRealm. So I used the same url for hostname and formSubmitURL.
But Matt told me:
If IE doesn't store a form submit URL, we should create these with a form submit URL of "" which is a wildcard otherwise we will import passwords that don't work if the form submit URL is on another origin.
I think the api has probably chnaged a bit.

> You can drop the "from the beginning" for brevity. And it's byte/offset 0, not "null".
> 
> So just "Bytes 0-3 are not needed."
> 
> Better still, can you note what's actually there, so it's clearer why it's not needed?

unfortunately, it's not documented.

> Again, please note what 12-19 are. (Even if that means noting we don't know what they are!)

unfortunately, it's not documented. there is no official documentation for security reasons probably they dont want to reveal how they store their passwords:
The only available doc is some hacker websites doing reverse engineering

> Can IE store logins with only a username or password? How are those represented?

no it cant do that

> What are they?

unfortunately, it's not documented. there is no official documentation for security reasons probably they dont want to reveal how they store their passwords:
The only available doc is some hacker websites doing reverse engineering
https://reviewboard.mozilla.org/r/15075/#review14381

> good catch for the fragement: i didnt test is before but it s working.
> The URL(…).pathname return "/" for "http://google.com"

> The URL(…).pathname return "/" for "http://google.com"

Is that a problem? You could special case "/" if you need.

> it's not documented

Sorry, I was confused about the data type for HTTP Auth.
https://reviewboard.mozilla.org/r/15075/#review14381

> > The URL(…).pathname return "/" for "http://google.com"
> 
> Is that a problem? You could special case "/" if you need.

for the encryption to work we need to have for http://google.com/home logins http://google.com/home as a key and for http://google.com/home?bala#v http://google.com/home as key too. So this is is why we split after ?
Hi dolske,
I updated my patch and I'm waiting for getting the other file review.
Thanks
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review14723

::: toolkit/components/passwordmgr/OSCrypto_win.js:70
(Diff revision 9)
> +                                                           ctypes.char16_t.ptr,

Why are you using char16 here? That doesn't match the Windows API (IN BYTE _pbData) and doesn't really make sense for an API that hashes random data.

For that matter, it would help with readability to define a "wintypes" (?) object with standard windows.h types in it, so that you can define the APIs here with, for example, wintypes.DWORD instead of ctypes.uint32_t.

::: toolkit/components/passwordmgr/OSCrypto_win.js:140
(Diff revision 9)
> +  },

For converting strings to/from arrays, can you use the built in ArrayType conversions?

See encrypt() and decrypt() in http://mxr.mozilla.org/mozilla-central/source/services/crypto/modules/WeaveCrypto.js

I _think_ your stringToArray can simply become new ctypes.ArrayType(ctypes.jschar)("foobar"), and arrayToString can be array.readString()?

::: toolkit/components/passwordmgr/OSCrypto_win.js:157
(Diff revision 9)
> +      }

I'd suggest not throwing if these cleanup functions fail.

::: toolkit/components/passwordmgr/OSCrypto_win.js:181
(Diff revision 9)
> +    if (!this._functions.get("CryptHashData")(hHash, str, 2 * (data.length + 1), FLAGS_NOT_SET)) {

Ah, this is presumably why you used char16_t as the function signature. You should correct the signature, and use ctypes.cast here.

And you should be able to just use str.size here instead of data.length. +/- 1.

::: toolkit/components/passwordmgr/OSCrypto_win.js:187
(Diff revision 9)
> +    let buffer = ctypes.unsigned_char.array(20)("");

s/20/dwHashLen/ here, just to make it a little more bulletproof / obvious that the value can't change.

(Alternatively, set dwHashLen to buffer.size.)

::: toolkit/components/passwordmgr/OSCrypto_win.js:208
(Diff revision 9)
> +    return hashStr;

So... I realized all this is doing, in the end, is a basic SHA1 hash of the URL? There's no need to use the OS native hashing functions for this.

Please just use nsICryptoHash instead. You can do this in pure JS, no ctypes needed.

::: toolkit/components/passwordmgr/OSCrypto_win.js:227
(Diff revision 9)
> +      let entropyArray = this.stringToArray(entropy);

See previous comments about not needing stringToArray / ArrayToString.

::: toolkit/components/passwordmgr/OSCrypto_win.js:230
(Diff revision 9)
> +      let optionalEntropy = new this._structs.DATA_BLOB(entropyData.length * 2,

Use .size instead of length * 2.
Attachment #8643453 - Flags: review?(dolske)
(In reply to Riadh Chtara[:rchtara] from comment #29)

> > Does the enumerator give any particular order? If not, we should sort by time to ensure we the migrate a more recent password, if it was changed during the history timespan.
> 
> [...]
> Doing the sorting and migrating the only the recent passwords is great idea,
> we can keep the last migration time of ie passwords and only migrates the
> links that are more recent then that. (we  still have at the first migration
> to import everything.) But we need to create an enumerator that give us the
> only the recent passwords

No, my comment was to sort by timespan so that the password we end up importing for the site is always going to be the newest.

For example: Last month I used password "old" to log in on http://site.com/page1. Then I change my password. Yesterday I logged in using password "new" on http://site.com/page2. Separate history visits, separate passwords, but we can ultimately only migrate a single password for http://site.com/ into Firefox. If the history is sorted, you can ensure the newer visit will preferentially be used.

Better yet, instead of sorting the entire IE history, just sort the resulting data that gets passed into _addLogin(). That would be a lot more efficient for the same effect. ;-)


> > This isn't valid, and I'm actualy surprised if we don't reject it. Only one of the formSubmitURL or httpRealm parameters can be non-null.
> > 
> > Yeah, we do reject it: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#283
> > 
> > Is your code working? I'd expect every addLogin() call to fail.
> 
> no it's working

Hmmm... Oh, I see. I was mixing up the last two arguments (aUsernameField / aPasswordField) with the actual formSubmitURL / httpRealm arguments. Yes, your code looks fine here, sorry.


> > So just "Bytes 0-3 are not needed."
> > 
> > Better still, can you note what's actually there, so it's clearer why it's not needed?
> 
> unfortunately, it's not documented.

Yes, but I'd still like to see comments explicitly saying so. Otherwise someone maintaining this code in the future is left wondering why it's just silently ignored.
https://reviewboard.mozilla.org/r/15075/#review14751

::: toolkit/components/passwordmgr/OSCrypto_win.js:208
(Diff revision 9)
> +    return hashStr;

I tried to use the follwing code (UTF16-UTF8) but failed): didnt get the same result
```javascript
var ch = Components.classes["@mozilla.org/security/hash;1"]
                   .createInstance(Components.interfaces.nsICryptoHash);
    ch.init(ch.SHA1);
    var converter =
    Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].
      createInstance(Components.interfaces.nsIScriptableUnicodeConverter);

    // we use UTF-8 here, you can choose other encodings.
    converter.charset = "UTF-16";
    // result is an out parameter,
    // result.value will contain the array length
    var result = {};
    // data is an array of bytes
    var data1 = converter.convertToByteArray(data, result);
    //data1 = new ctypes.ArrayType(ctypes.jschar)(data);
    ch.update(data1, data1.length);
    var hash = ch.finish(false);

    // return the two-digit hexadecimal code for a byte
    function toHexString(charCode) {
      return ("0" + charCode.toString(16)).slice(-2);
    }

    // convert the binary hash data to a hex string.
    var s = [toHexString(hash.charCodeAt(i)) for (i in hash)].join("");
    return s.toUpperCase();
```
https://reviewboard.mozilla.org/r/15075/#review14723

> For converting strings to/from arrays, can you use the built in ArrayType conversions?
> 
> See encrypt() and decrypt() in http://mxr.mozilla.org/mozilla-central/source/services/crypto/modules/WeaveCrypto.js
> 
> I _think_ your stringToArray can simply become new ctypes.ArrayType(ctypes.jschar)("foobar"), and arrayToString can be array.readString()?

The problem is that I'm using them  (arrayToString) in test_Chrome_passwords.js where i need a real array, so for constituency, lets not change them

> Ah, this is presumably why you used char16_t as the function signature. You should correct the signature, and use ctypes.cast here.
> 
> And you should be able to just use str.size here instead of data.length. +/- 1.

size is undefined

> See previous comments about not needing stringToArray / ArrayToString.

for the same reasons, let s not change them

> Use .size instead of length * 2.

size is undefined
Attachment #8643453 - Flags: review?(dolske)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
(In reply to Riadh Chtara[:rchtara] from comment #35)

> I tried to use the follwing code (UTF16-UTF8) but failed): didnt get the
> same result

Your ctypes code is hashing the string including the null terminator, your JS code is not.

WFM as such with the data in test_OSCrypto_win.js. EG |var data = "https://github.com/login\0";|

Well, except that your JS code seems to be dropping the last byte in the output. "0112F7DCE67B8579EA01367678AA44AB9868B5A143" vs "0112F7DCE67B8579EA01367678AA44AB9868B5A1". I didn't debug that since it's presumably trivial. :)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
(In reply to Riadh Chtara[:rchtara] from comment #41)
Awesome, it s working now. The best part is that we had issues running the code in different computers/oses because of these api have a bit different behavior depending on the computers and os.
We kind of managed to solve that issue (I'm not 100% sure exactly about that).
But now with your suggestion, we don't even have to think about that. 
Thanks a lot for that Dolske
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
https://reviewboard.mozilla.org/r/15075/#review14381

> for the encryption to work we need to have for http://google.com/home logins http://google.com/home as a key and for http://google.com/home?bala#v http://google.com/home as key too. So this is is why we split after ?

You should concatenate .origin with .pathname
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
https://reviewboard.mozilla.org/r/15075/#review14381

> You should concatenate .origin with .pathname

concerning the ref: ie doesn't store the ref for history url, so it can be ignored,  and this is why my solution is working.
(I have quickly tested and found out that logins for the same url and different refs are stored in the same reg entry). so even if i was lucky because in history there is not ref, your solution is better
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
MattN, Dolske: all the unit tests are passing now in all the platforms.
please tell me if I need to run another kind of tests.
MattN: could you please have another look to the my last changes
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review15641

::: browser/components/migration/IEProfileMigrator.js:13
(Diff revision 19)
> +const kFlagsNotSet = 0;

kCryptProtectUIForbidden and kFlagsNotSet would be better as properties on OSCrypto.

But better yet, why expose these in the API at all? A boolean "allowUI" arg would be cleaner to use.

But better STILL, why have this arg at all? Seems like it's not actually needed or used... All the call sites seem to pass in kCryptProtectUIForbidden, except for ChromeProfileMigrator. And that seems like an oversight? We're not fixing the Chrome migrator here, are we?

::: browser/components/migration/IEProfileMigrator.js:37
(Diff revision 19)
> -let CtypesHelpers = {
> +function CtypesHelpers() {

Hmm, looks like bug 1192036 has now landed and moved some of this stuff over to MSMigrationUtils.jsm. Doesn't look too hard to merge your changes though.

::: browser/components/migration/IEProfileMigrator.js:478
(Diff revision 19)
> +function IE7FormPasswords () {

It's probably worth a comment here (or at the use site) to note that this isn't specific to _just_ IE7. It might be obvious to us right now, but maybe not to someone looking at this later.

::: browser/components/migration/IEProfileMigrator.js:537
(Diff revision 19)
> +     * contains an encryption of all the details of the multiple logins used to login in this URL

This comment is difficult to read, I'd clean it up as follows:

The logins are stored in the registry, where the key is a hashed URL and its value contains the encrypted details for all logins for that URL.

First iterate through IE history, hashing each URL and looking for a match. If found, decrypt the value, using the URL as a salt. Finally add any found logins to the Firefox password manager.

::: browser/components/migration/IEProfileMigrator.js:608
(Diff revision 19)
> +        if (login.username == existingLogin.username && login.password != existing.password) {

You could just use existingLogin.matches(login, true). But fine either way.

::: browser/components/migration/IEProfileMigrator.js:619
(Diff revision 19)
> +            isNew = false;

I worry about failure modes here, might be better to simply use modifyLogin() to update the existing login's password.

For example: if the first existing login if older, but the second existing login is newer, you'll set isNew back to false and never addLogin() anything. But will have deleted the user's otherwise OK login!

::: browser/components/migration/IEProfileMigrator.js:626
(Diff revision 19)
> +    }

Nit: add a blank line here, after the function() { }.

::: browser/components/migration/IEProfileMigrator.js:674
(Diff revision 19)
> +      {"foo": ctypes.uint32_t},

Heh, I didn't mean for you to take my previous comment here so literally... Please replace foo/bar/baz with "unknown[123]" or "unused[123]". Sorry. :)

::: browser/components/migration/IEProfileMigrator.js:692
(Diff revision 19)
> +      {"hiDateTime": ctypes.uint32_t},

Just to be sure... You verified that these are sane timestamps from actual IE data? At least one of the random webpages that tries to explain this data format claims it's just a unique id.

::: browser/components/migration/IEProfileMigrator.js:706
(Diff revision 19)
> +    let currentInfoPointer = 36;

This implies there are another 12 unknown bytes at the end of the loginData struct... That is, the decrypted data smells like a LoginData header followed by 0-N LoginItem(s).

Maybe add the extra 12 bytes to the defination (3 unknown uint32s is fine), and just use |currentInfoPointer = loginData.size;|?

Also, please rename this to currentInfoIndex -- it's not a pointer, which can be especially confusing in the context of ctypes.

::: browser/components/migration/IEProfileMigrator.js:714
(Diff revision 19)
> +          arr.length - currentInfoPointer)(arr.slice(currentInfoPointer));

You should be stricter with the length here:

let rawLoginItem = ctypes.unsigned_char.array(
  loginInfo.size)(arr.slice(currentInfoPointer));
  
Alternatively, you could skip the slicing & casting each time through the loop by basically getting a loginInfo.ptr for arr[36], and simply incrementing it each loop. That's the more C-like way to iterate over an array of structs, but isn't critical here.

::: browser/components/migration/IEProfileMigrator.js:723
(Diff revision 19)
> +      // the username is encoded in Unicode, null terminated and stored at

"The username is UTF-16 and null-terminated."

(The storage location is obvious from the code)

::: browser/components/migration/IEProfileMigrator.js:726
(Diff revision 19)
> +      // the password is encoded in Unicode, null terminated and stored at

Ditto.

::: browser/components/migration/IEProfileMigrator.js:731
(Diff revision 19)
> +      currentResult.password = this._crypto.arrayToString(toUni(currentPassword));

I do still this code would end up simpler if you avoided all the intermediate arrays/strings, and just operated on raw byte array [ctypes.unsigned_char.array()] coming out of decrypt().

Then all the struct stuff would be simple offsets / casts, and the username/password part here would also just be an offset / cast to char16 + .readString().

The code you've got isn't wrong per se, but it's inefficient by generating a lot of intermediate garbage (in the JS sense ;).

I suppose I'll take it, but it would be a good followup bug to cleanup...

::: browser/components/migration/IEProfileMigrator.js:734
(Diff revision 19)
> +      currentInfoPointer += 32;

Ditto as with currentInfoPointer comment about, this implies there are 12 (again) unknown bytes, and by adding them to the struct this can be += loginItem.size;

::: browser/components/migration/IEProfileMigrator.js:867
(Diff revision 19)
> +  this.wrappedJSObject = this; // for testing...

???
Attachment #8643453 - Flags: review?(dolske)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review?(dolske)
https://reviewboard.mozilla.org/r/15075/#review15863

::: browser/components/migration/IEProfileMigrator.js:608
(Diff revision 19)
> +        if (login.username == existingLogin.username && login.password != existing.password) {

I'm wondering if matches existingLogin.matches(login, true) returns true  even when timestamps are not the same

::: browser/components/migration/IEProfileMigrator.js:619
(Diff revision 19)
> +            isNew = false;

I'm not sure but I think it s not possible for to have two existing logins with the same username and different passwords, so only it's not only possible  to have  isNew false and a removed
login at the same time
But I updated my code to make it more clear.
https://reviewboard.mozilla.org/r/15075/#review15641

> Heh, I didn't mean for you to take my previous comment here so literally... Please replace foo/bar/baz with "unknown[123]" or "unused[123]". Sorry. :)

:)
You know, we programmers like to call staff foo /bar  .. so I thought it's not that bad to do that :-)
You know something even funnier, I used them here https://reviewboard.mozilla.org/r/17061/diff/5#2 (line 150)

> Just to be sure... You verified that these are sane timestamps from actual IE data? At least one of the random webpages that tries to explain this data format claims it's just a unique id.

yes, i saved some logins from ie and then imported them. And the dates were what I expected.There is no official docs so the web pages is wrong (timestamps in many cases could be a unique ids as no one is going to store two passwords in the same time, so it's not that wrong.)

> ???

I just wanted to use the _migrateURIs(uris) function in the test
https://reviewboard.mozilla.org/r/15075/#review15641

> You could just use existingLogin.matches(login, true). But fine either way.

I'm wondering if matches existingLogin.matches(login, true) returns true  even when timestamps are not the same

> I worry about failure modes here, might be better to simply use modifyLogin() to update the existing login's password.
> 
> For example: if the first existing login if older, but the second existing login is newer, you'll set isNew back to false and never addLogin() anything. But will have deleted the user's otherwise OK login!

I'm not sure but I think it s not possible for to have two existing logins with the same username and different passwords, so only it's not only possible  to have  isNew false and a removed
login at the same time
But I updated my code to make it more clear.
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review15915

Looks good except for one last issue. And thanks for cleaning up the ctypes usage!

::: toolkit/components/passwordmgr/OSCrypto_win.js:175
(Diff revisions 19 - 20)
> +      flags = CRYPT_PROTECT_UI_FORBIDDEN;

This doesn't make sense to me. If "allowUI" is true you're setting the UI_FORBIDDEN flag? I think your logic here is backwards.

Also, it's generally better form to write "if (test) {} else {}" than "if (!test) {} else {}". Skips having to think about the double-negative, which can lead to such mistakes.

And again, is there a reason to support this at all? All your callers of this are asking to disallow UI, so it would be simpler just to remove support for this entirely.

::: toolkit/components/passwordmgr/OSCrypto_win.js:228
(Diff revisions 19 - 20)
> +      flags = CRYPT_PROTECT_UI_FORBIDDEN;

Ditto.
Attachment #8643453 - Flags: review?(dolske) → review+
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review+ → review?(dolske)
https://reviewboard.mozilla.org/r/15075/#review15915

> This doesn't make sense to me. If "allowUI" is true you're setting the UI_FORBIDDEN flag? I think your logic here is backwards.
> 
> Also, it's generally better form to write "if (test) {} else {}" than "if (!test) {} else {}". Skips having to think about the double-negative, which can lead to such mistakes.
> 
> And again, is there a reason to support this at all? All your callers of this are asking to disallow UI, so it would be simpler just to remove support for this entirely.

it s used here
http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/ChromeProfileMigrator.js#393
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review16067

::: toolkit/components/passwordmgr/OSCrypto_win.js:149
(Diff revision 21)
> +   * @param {forbiddenUI?} forbiddenUI - this is used for remote situations where presenting a UI is

What is this data type? Did you mean to make this a boolean?

::: toolkit/components/passwordmgr/OSCrypto_win.js:149
(Diff revision 21)
> +   * @param {forbiddenUI?} forbiddenUI - this is used for remote situations where presenting a UI is
> +   * not an option (could be null).

uiForbidden is more clear IMO.

::: toolkit/components/passwordmgr/OSCrypto_win.js:153
(Diff revision 21)
> +  decryptData(data, entropy, forbiddenUI) {

Please provide default values for optional arguments. For the last argument it should probably be true or false.

::: toolkit/components/passwordmgr/OSCrypto_win.js:202
(Diff revision 21)
> +   * @param {forbiddenUI?} forbiddenUI - this is used for remote situations where presenting a UI is
> +   * not an option (could be null).
> +   * @returns {string} the encrypted string.
>     */
> -  encryptData(string) {
> +  encryptData(data, entropy, forbiddenUI) {

Same comments apply.
Attachment #8643453 - Flags: review?(MattN+bmo) → review+
https://reviewboard.mozilla.org/r/15075/#review15863

> I'm not sure but I think it s not possible for to have two existing logins with the same username and different passwords, so only it's not only possible  to have  isNew false and a removed
> login at the same time
> But I updated my code to make it more clear.

I thought it's possible if the formSubmitURL differs.
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Attachment #8643453 - Flags: review+ → review?(MattN+bmo)
https://reviewboard.mozilla.org/r/15075/#review15863

> I thought it's possible if the formSubmitURL differs.

Ah, ok!I didn't know that:  I think it doesn't mater anymore
with https://reviewboard.mozilla.org/r/17063/diff/5#3
Keywords: checkin-needed
(In reply to Riadh Chtara[:rchtara] from comment #60)

> > And again, is there a reason to support this at all? All your callers of this are asking to disallow UI, so it would be simpler just to remove support for this entirely.
> 
> it s used here
> http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> ChromeProfileMigrator.js#393

Why? Does Chrome's storage actually require authentication UI to be shown?
(In reply to Justin Dolske [:Dolske] from comment #65)
> (In reply to Riadh Chtara[:rchtara] from comment #60)
> 
> > > And again, is there a reason to support this at all? All your callers of this are asking to disallow UI, so it would be simpler just to remove support for this entirely.
> > 
> > it s used here
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> > ChromeProfileMigrator.js#393
> 
> Why? Does Chrome's storage actually require authentication UI to be shown?

It doesn't require it but doesn't disallow it either. I was also wondering about this and figured it may have something to do with limited accounts. I agree that we probably can be consistent here since the context for IE vs. Chrome migration is the same to the user.
Keywords: checkin-needed
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #66)
> (In reply to Justin Dolske [:Dolske] from comment #65)
> > (In reply to Riadh Chtara[:rchtara] from comment #60)
> > 
> > > > And again, is there a reason to support this at all? All your callers of this are asking to disallow UI, so it would be simpler just to remove support for this entirely.
> > > 
> > > it s used here
> > > http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> > > ChromeProfileMigrator.js#393
> > 
> > Why? Does Chrome's storage actually require authentication UI to be shown?
> 
> It doesn't require it but doesn't disallow it either. I was also wondering
> about this and figured it may have something to do with limited accounts. I
> agree that we probably can be consistent here since the context for IE vs.
> Chrome migration is the same to the user.

You are right guys, I thought it was required,but I have removed and it's still working
So let's get rid of it
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
Keywords: checkin-needed
checkin-needed is used to indicate that the patch is ready to land. Please avoid setting it until the patch has proper review and has successfully passed tests on a push to Try. Thanks!
Keywords: checkin-needed
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review16189
Attachment #8643453 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

https://reviewboard.mozilla.org/r/15075/#review16191
Attachment #8643453 - Flags: review?(dolske) → review+
Keywords: checkin-needed
(In reply to Justin Dolske [:Dolske] from comment #74)
> Comment on attachment 8643453 [details]
> MozReview Request: Bug 682069 - Password Import from IE not available
> 
> https://reviewboard.mozilla.org/r/15075/#review16191

this patch failed to apply:

applying d81c09a522cf
patching file browser/components/migration/MSMigrationUtils.jsm
Hunk #1 FAILED at 23
Hunk #2 FAILED at 349
Hunk #3 FAILED at 376
Hunk #4 FAILED at 452
4 out of 4 hunks FAILED -- saving rejects to file browser/components/migration/MSMigrationUtils.jsm.rej
patch failed to apply

could you take a look ? Thanks!
Flags: needinfo?(rchtara)
Keywords: checkin-needed
Attachment #8643453 - Flags: review?(dolske)
Attachment #8643453 - Flags: review?(MattN+bmo)
Attachment #8643453 - Flags: review+
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Bug 682069 - Password Import from IE not available
(In reply to Carsten Book [:Tomcat] from comment #75)
> (In reply to Justin Dolske [:Dolske] from comment #74)
> > Comment on attachment 8643453 [details]
> > MozReview Request: Bug 682069 - Password Import from IE not available
> > 
> > https://reviewboard.mozilla.org/r/15075/#review16191
> 
> this patch failed to apply:
> 
> applying d81c09a522cf
> patching file browser/components/migration/MSMigrationUtils.jsm
> Hunk #1 FAILED at 23
> Hunk #2 FAILED at 349
> Hunk #3 FAILED at 376
> Hunk #4 FAILED at 452
> 4 out of 4 hunks FAILED -- saving rejects to file
> browser/components/migration/MSMigrationUtils.jsm.rej
> patch failed to apply
> 
> could you take a look ? Thanks!
I have just done a rebase for it.
thanks
Flags: needinfo?(rchtara)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

In our password stand up, I discussed with MattN about that and he said to flag him and dolske r+ because I just have done a rebase.
Attachment #8643453 - Flags: review?(dolske)
Attachment #8643453 - Flags: review?(MattN+bmo)
Attachment #8643453 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dd89b3556b3d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Approval Request Comment
[Feature/regressing bug #]: Windows 10 (Edge) migrators
[User impact if declined]: No passwords from IE on Windows 7 or earlier
[Describe test coverage new/current, TreeHerder]: Basic automated sanity checks. Manual testing on a few different devices.
[Risks and why]: Low risk of breaking other imports since the migrator isolates each data type with try…catch. Some chance of crashes due to ctype usage but I think that should be low based on the APIs used. 
[String/UUID change made/needed]: None
Attachment #8643453 - Flags: approval-mozilla-beta?
Attachment #8643453 - Flags: approval-mozilla-aurora?
Florin, would you be able to test this functionality on latest Nightly? This would help me decide whether this can be uplifted in Beta41 or not.
Flags: needinfo?(florin.mezei)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Given that windows is our predominant end-user base, I do not feel comfortable approving this change so late in the Beta cycle without additional verification. We have lived through 40 release and we may have to live with this bug in 41 as well. If QE team is able to test this and does not find any major regressions, we can reconsider beta uplift decision. But until then, this is a no-go.
Attachment #8643453 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Ritu Kothari (:ritu) from comment #82)
> Florin, would you be able to test this functionality on latest Nightly? This
> would help me decide whether this can be uplifted in Beta41 or not.

We could test this prior to a Beta uplift, but I see this impacts IE and Chrome, and we might want to do a full pass on Migrators to make sure there are no regressions. Note though that a full pass on Migrators takes some time to properly test (we do it once per Beta cycle and it was already done for 41 Beta 6 - https://goo.gl/b4p4or). This would mean a full pass on Nightly and one on Beta which seriously cuts down from our already very short time available to finish up Beta work (Beta features sign-offs, Beta 9 sign-off, and Beta Bug Verification).

I personally see this patch as being: 
- medium-high risk 
     - "some chance of crashes" specified in comment 81
     - we are very late in the Beta 41 cycle, so any breakage would cause a high amount of extra work on all sides
- low user impact 
     - issue has been around since 2011

My recommendation would be to let this ride the trains (2 cycles), or if we really want this sooner at most uplift it to Aurora 42, so it would at least get a full Beta 42 cycle. Ritu, let me know what you think of the above assessment and recommendation.

Matt, a couple of questions for you:
1. Could this impact migration from Windows 10 Edge as well?
2. What's the impact of this patch on other migrators? I see some work was also done for the Chrome migrator?
Flags: needinfo?(rkothari)
Flags: needinfo?(MattN+bmo)
Thanks Florin for your recommendation and detailed explanation. Everything you said makes sense.
Flags: needinfo?(rkothari)
Comment on attachment 8643453 [details]
MozReview Request: Bug 682069 - Password Import from IE not available

Let's take it in 42 now so that we have time to tackle potential regressions.
Attachment #8643453 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
this cause problems applying to aurora:

merging browser/components/migration/MSMigrationUtils.jsm
3 files to edit
merging browser/components/migration/MSMigrationUtils.jsm failed!
merging toolkit/components/passwordmgr/test/unit/xpcshell.ini   

Riadh, could you take a look, thanks!
Flags: needinfo?(rchtara)
Hi
We're waiting on a decision and uplift in bug 1193387 comment 27
Thanks
Flags: needinfo?(rchtara)
(In reply to Riadh Chtara[:rchtara] from comment #88)
> Hi
> We're waiting on a decision and uplift in bug 1193387 comment 27
> Thanks

thanks also made a comment in there about uplifting since this might then need beta approval
This missed 42 due to confusion from bug 1193387 uplift getting delayed
status-firefox42: affected → wontfix
Are there any particular sites that need an increased attention? Because I’m not so sure which sites are included in this bug, and at what pages is Bug 1191175 referring to.
Flags: needinfo?(dolske)
This isn't a really site-specific bug. "Form logins" are logins entered in web content, "http auth" logins are entered into browser chrome and are generally less common. http://dolske.net/mozilla/tests/httpauth/realmX/ is an example of the latter.
Flags: needinfo?(dolske)
I was able to reproduce this issue on Firefox 43.0a1 (2015-09-01) using Windows 7 64-bit.

Verified fixed on Firefox 43 Beta 9 (20151203163240) under Windows 7 64-bit and Windows Vista 32-bit. The IE passwords are successfully imported and displayed in about:preferences#security.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Flags: needinfo?(MattN+bmo)
Depends on: 1533288
Depends on: 1535757
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: