Closed Bug 1564284 Opened 3 months ago Closed 2 months ago

Passwords saved in Windows profile do not load when copied to Linux

Categories

(NSS :: Libraries, defect, P1, minor)

3.45
Unspecified
Windows

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: faxxx720, Assigned: faxxx720)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.100 Safari/537.36

Steps to reproduce:

  1. Copied %appdata%\Mozilla\Firefox\Profiles\XXXX.profile to ~/.mozilla/firefox
  2. Updated profiles.ini to match new profile
  3. Opened Firefox 67.0.4+build1-0ubuntu0.18.04.1 with copied profile

Actual results:

Found that all saved passwords and usernames were gone, both from the website and Preferences > Saved Logins.

Expected results:

Passwords should have been preserved.

I have debugged this problem and found that in nss/lib/util/utilmod.c, line 415, inside of nssutil_ReadSecmodDB, when reading the pkcs11.txt file, the code assumes only '\n' will be a line separator, but Windows versions of Firefox create this file with '\r\n' line endings.
I fixed the code so that on non-Windows platforms (tested on Windows Subsystem, Ubuntu 18.04), the file is checked for '\r\n' and '\n', and both are appropriately handled & attached a patch.

Attached patch patch.diff (obsolete) — Splinter Review
Comment on attachment 9076693 [details] [diff] [review]
patch.diff

Review of attachment 9076693 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Frank!

Marcus, can you review? Off-hand I'd rather be permissive for this format on all platforms rather than just Windows, but I haven't dug into it.

Also, can you look to see if there's anywhere we could add a gtest for this change?
Attachment #9076693 - Flags: review?(marcus.apb)
Assignee: nobody → faxxx720
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Unspecified → Windows
Priority: -- → P1

Hello J.C.! Thanks for your input! The reason I put in the Windows #ifdef was because the current Windows release of Firefox, 67.0.4 and also the Development edition, 69.0.b2 already handles both '\r\n' and '\n', so I thought to just avoid modifying any behavior on Windows.

Hi Frank,

Thanks for this report and the patch suggestion.
Looks that some other bugs will be direct or indirectly benefited with this patch too.

I am just commenting you patch now. Could you improve it when I try to find a way to automate the tests?

Thanks,

Flags: needinfo?(faxxx720)
Comment on attachment 9076693 [details] [diff] [review]
patch.diff

Review of attachment 9076693 [details] [diff] [review]:
-----------------------------------------------------------------

Frank, I am sure we can add this test for the Windows format without this "IFDEF" in a safe way.
Could you improve this code, please?

Also, would be great is you send this patch to Phabricator. Here are some documentations to help you prepare an environment:
- https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
- https://github.com/marcusburghardt/nss-tips/blob/master/dev-fasttrack (I have to update some points, but is still useful : )

Sadly, this will take me a bit longer to get a Windows dev environment working to test the Windows file reading semantics -- I was using WSL previously (to avoid dealing with updating to vs2019).

Comment on attachment 9076693 [details] [diff] [review]
patch.diff

Review of attachment 9076693 [details] [diff] [review]:
-----------------------------------------------------------------

Ok Frank,

If you don't matter, I can help with this patch.

Thanks,
Attachment #9076693 - Flags: review?(marcus.apb) → review-
Attached patch patch-v2.diffSplinter Review

Marcus,

I've updated the patch to not use an ifdef, and uploaded to phabricator.

However, I wasn't able to properly build tests for a release build for Windows -- while I can build the libraries both Debug and Release, there's a 100% CPU usage on one core from cl.exe when trying to do "LINK_EMBED: .../dist/Release/bin\addbuiltin.exe", which doesn't seem to end. This build is with nss and nspr at the tip of the hg default branch at the time I wrote this commit. This happened even if I did not include my patch.

Thanks
Frank

Attachment #9076693 - Attachment is obsolete: true
Flags: needinfo?(faxxx720)

Hi Frank,

I tested your patch here and looks good.
I am analysing a way to test this change here and than we can validate.

Thanks,

Blocks: 1573942
See Also: → 1453372
See Also: → 1558025
Flags: needinfo?(faxxx720)
Keywords: checkin-needed
Flags: needinfo?(faxxx720)
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.46
Duplicate of this bug: 1453372
Duplicate of this bug: 1558025
You need to log in before you can comment on or make changes to this bug.