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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Frank Migliorino]

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.

Comment 1

[Frank Migliorino]

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.

Comment 2

[Frank Migliorino]

Attachment: patch.diff

Comment 3

[J.C. Jones [:jcj] (he/they)]

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?

Comment 4

[Frank Migliorino]

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.

Comment 5

[Marcus Burghardt]

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.

• https://bugzilla.mozilla.org/show_bug.cgi?id=1453372
• https://bugzilla.mozilla.org/show_bug.cgi?id=1558025
• https://bugzilla.mozilla.org/show_bug.cgi?id=1541284
• https://bugzilla.mozilla.org/show_bug.cgi?id=1505038

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

Thanks,

Comment 6

[Marcus Burghardt]

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 : )

Comment 7

[Frank Migliorino]

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 8

[Marcus Burghardt]

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,

Comment 9

[Frank Migliorino]

Attachment: patch-v2.diff

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

Comment 10

[Frank Migliorino]

Attachment: Bug 1564284: Added check for CR + LF, r=marcusburghardt

Comment 11

[Marcus Burghardt]

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,

Comment 12

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/d1d2e1e320cd7ec4e71075aaea7ec4f185b27375