Steps to reproduce: 1. Navigate to <about:profiles>. 2. Click [Create a New Profile] to open [Create Profile Wizard]. 3. Click [Next >]. 4. Type a non-ASCII characters that IS contained in the current ANSI code page into the [Enter new profile name:] text box. For example, "1÷1" for English locales. 5. Click [Finish]. 6. Click [Set as default profile] to make the created profile default. 7. Restart Nightly. 8. Navagate to <https://www.cacert.org/>. 9. Click [Advanced]. 10. Click [Add Exception...] to open [Add Security Exception] dialog. 11. Make sure [Permanently store this exception] is checked. 12. Click [Confirm Security Exception]. Actual result: Nothing happens. If you open Browser Console, you will see the following error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICertOverrideService.rememberValidityOverride] Expected result: The [Add Security Exception] dialog closes and CAcert loads normally.

SQLite will expect that database file paths are always encoded in UTF-8[1] regardless of the current system locale while PR_Access uses ANSI encoding. So sdb_init will always fail if the profile path contains a non-ASCII character.[2] [1] https://sqlite.org/c3ref/open.html [2] https://dxr.mozilla.org/mozilla-central/rev/f5a1cb52c12e8fbcf2e3b5a675fe2a84d43507a7/security/nss/lib/softoken/sdb.c#1742,1747

We should flip "security.use_sqldb" on beta unless this bug is fixed. Unfortunately, bug 1417677 removed this pref from Nightly :(

It would be too late to fix NSS on beta.

A local proof-of-concept change to NSS.

Note that this patch alone is insufficient to fix the problem. NSS change is also necessary.

This patch is not landable until NSS change land and is merged into m-c.

Added an automated test.

I see that there could be a problem but I can't reproduce this with Beta or Nightly. Did you do anything else here than what you wrote in comment 0?

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8) > I see that there could be a problem but I can't reproduce this with Beta or > Nightly. Did you do anything else here than what you wrote in comment 0? Possible reason: - I didn't actually try the STR with an English locale. - The profile name must contain a non-ASCII character in first three characters of the name. Can new reproduce the test failure with the attached regression test?

Hm, apparently the short (8.3) path name will never contain a non-ASCII characters on en-US locale: C:\Users\****\AppData\Roaming\Mozilla\Firefox\Profiles>dir /x Volume in drive C has no label. Volume Serial Number is 948D-32ED Directory of C:\Users\****\AppData\Roaming\Mozilla\Firefox\Profiles 2018/01/03 20:57 <DIR> . 2018/01/03 20:57 <DIR> .. 2018/01/03 20:59 <DIR> HK25T3~1.11 hk25t3oj.1÷1 2018/01/03 20:58 <DIR> OTU763~1.DEF otu763d7.default 0 File(s) 0 bytes 4 Dir(s) 44,973,137,920 bytes free So the STR in comment #0 will not work.

On the other hand, the short path name may contain an non-ASCII character on ja-JP locale: C:\Users\****\AppData\Roaming\Mozilla\Firefox\Profiles>dir /x ドライブ C のボリューム ラベルは OS です ボリューム シリアル番号は 0635-5947 です C:\Users\****\AppData\Roaming\Mozilla\Firefox\Profiles のディレクトリ 2017/12/30 14:11 <DIR> . 2017/12/30 14:11 <DIR> .. 2017/12/30 14:13 <DIR> I5II7B~1.1÷ i5ii7bd6.1÷1 0 個のファイル 0 バイト 12 個のディレクトリ 63,663,915,008 バイトの空き領域 So this bug depends on the Windows system locale.

Please add following steps before steps in comment #0: 1. Press Win+R to open the [Run] dialog. 2. Type "intl.cpl" into the dialog and press Enter to open the [Region] property sheet. 3. Click the [Administrative] tab of the [Region] sheet. 4. Click [Change system locale] to open the [Region and Language Settings] dialog. 5. Choose "Japanese (Japan)" from the [Current system locale:] list. 6. Click [OK]-[OK]. 7. Restart Windows twice. (System locale did not change after the first reboot for me.) 8. Make sure that the current system locale is changes to "Japanese (Japan)". Just in case, please make sure that the short file name of the profile path contains a non-ASCII character using "dir /x" after step 5 in comment #0. Could you reproduce the problem with this correction?

FYI only. This could be the reason why sometimes NSS init was failing.

I still don't get the error. But I also don't have non-ASCII characters in the short path. More importantly we need an automated test to check that this is working.

Tracking for 58 and 59. Gerry, do you think we should make this a blocker for 58?

Comment on attachment 8939139 [details] [diff] [review] Reression test to make sure non-ASCII file paths work Review of attachment 8939139 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't fail for me without the fix patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2765fdb0fab7761fc9d4482024a8451cb28eed2e&selectedJob=153921674 (the orange is unrelated) I guess we have to programmatically set the codepage on the test machine? Or maybe we have to set up a special image to run this test?

Comment on attachment 8939138 [details] [diff] [review] Use UTF-8 file paths for NSS database Review of attachment 8939138 [details] [diff] [review]: ----------------------------------------------------------------- Seems like the right thing to do but I'm guessing there are other places in NSS where this goes wrong. Do we have any way to test this? ::: security/manager/ssl/nsNSSComponent.cpp @@ +1754,5 @@ > "Some things may not work as expected."); > return NS_OK; > } > > + // SQLite always takes UTF-8 file paths regardless of the current system ... but does NSS? Are there not other areas in NSS where this could go wrong?

Comment on attachment 8939152 [details] [diff] [review] Disable sqldb when the profile path contains a non-ASCII character Review of attachment 8939152 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable, but we should limit the scope as much as possible - hopefully by codepage and certainly by platform. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +1064,5 @@ > flags |= NSS_INIT_NOMODDB; > } > + // At the moment, sqldb does not work with non-ASCII file paths. > + bool useSQLDB = Preferences::GetBool("security.use_sqldb", false) && > + NS_IsAscii(PromiseFlatCString(dir).get()); Can we determine what the codepage is here as well? It would be nice to not disable this for users that aren't even affected by the bug (e.g. en-US windows, other platforms entirely). ::: security/manager/ssl/tests/unit/test_db_format_nonascii.js @@ +33,5 @@ > + ok(!keyDBFile.exists(), "key3.db should not exist beforehand"); > + // This should start PSM. > + Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports); > + ok(certificateDBFile.exists(), "cert8.db should exist in the profile"); > + ok(keyDBFile.exists(), "key3.db should exist in the profile"); Also check that cert9.db/key4.db don't exist.

(In reply to David Keeler [:keeler] (use needinfo) from comment #16) > This doesn't fail for me without the fix patches: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2765fdb0fab7761fc9d4482024a8451cb28eed2e&selectedJob=1 > 53921674 (the orange is unrelated) You are right, this test will not fail on en-US locale as long as we use short paths. The fix patch stops using short paths. > I guess we have to programmatically set the codepage on the test machine? Or > maybe we have to set up a special image to run this test? Changing system locale requires restarting Windows. So we will need a special image... But at least the current test will catch regressions if the fix patch applied.

(In reply to David Keeler [:keeler] (use needinfo) from comment #18) > This seems reasonable, but we should limit the scope as much as possible - > hopefully by codepage and certainly by platform. > > ::: security/certverifier/NSSCertDBTrustDomain.cpp > @@ +1064,5 @@ > > flags |= NSS_INIT_NOMODDB; > > } > > + // At the moment, sqldb does not work with non-ASCII file paths. > > + bool useSQLDB = Preferences::GetBool("security.use_sqldb", false) && > > + NS_IsAscii(PromiseFlatCString(dir).get()); > > Can we determine what the codepage is here as well? It would be nice to not > disable this for users that aren't even affected by the bug (e.g. en-US > windows, other platforms entirely). I added a non-Windows check, but the current system locale is unreliable to avoid the bug. The short file name is determined when the file is created. If the file is created under the ja_JP locale, the short path contains a non-ASCII character regardless of the current system locale (I verified it locally). > ::: security/manager/ssl/tests/unit/test_db_format_nonascii.js > @@ +33,5 @@ > > + ok(!keyDBFile.exists(), "key3.db should not exist beforehand"); > > + // This should start PSM. > > + Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports); > > + ok(certificateDBFile.exists(), "cert8.db should exist in the profile"); > > + ok(keyDBFile.exists(), "key3.db should exist in the profile"); > > Also check that cert9.db/key4.db don't exist. Done. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=306b9d84669f332375b672f20076b211f025b6e2

Using GetCanonicalPath to avoid regressions on the en_US locale. (In reply to David Keeler [:keeler] (use needinfo) from comment #17) > Seems like the right thing to do but I'm guessing there are other places in > NSS where this goes wrong. Do we have any way to test this? We will need a special image as I said above. > ::: security/manager/ssl/nsNSSComponent.cpp > @@ +1754,5 @@ > > "Some things may not work as expected."); > > return NS_OK; > > } > > > > + // SQLite always takes UTF-8 file paths regardless of the current system > > ... but does NSS? Are there not other areas in NSS where this could go wrong? If the profile path contains a character that is NOT in the current system code page, NSS couldn't create cert8.db/key3.db even before this bug. But it is a known problem (bug 336829). This bug is about the regression that even non-ASCII characters that are in the current system code page break SSL exceptions. cert*.db/key*.db are only files that NSS creates under the profile folder. If the Firefox install path contains a character that is not in the current system code page (it's possible when Firefox is installed per-user), NSS will not work correctly because it cannot find some libralies correctly. Again, this is an existing issue (although I could not find a bug number). We have to make NSPR Unicode-aware and completely stop using MSVCRT/UCRT narrow functions and Win32 "A" functions to handle all cases properly. But is is far beyond the scope of this bug. Please do not extend the bug scope endlessly. This regression is really critical for Japanese users. On Japanese Windows, short paths can contain non-ASCII characters, and user names are very likely to contain non-ASCII characters due to Microsoft accounts.

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #14) > I still don't get the error. But I also don't have non-ASCII characters in > the short path. Did you recreated the profile after changing the system locale? If so, I'm running out of ideas why you can't create short path with non-ASCII characters. > More importantly we need an automated test to check that this is working. It will need a special image. We couldn't go forward without preparing the image?

Per comment #22, this regression is critical for Japanese users. Mark blocking for 58.

Oh, a special image was not neccessary after all. (In reply to David Keeler [:keeler] (use needinfo) from comment #16) > This doesn't fail for me without the fix patches: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2765fdb0fab7761fc9d4482024a8451cb28eed2e&selectedJob=1 > 53921674 (the orange is unrelated) > I guess we have to programmatically set the codepage on the test machine? Or > maybe we have to set up a special image to run this test? Even though this test does not work on your local machine, automation can catch regressions because automation disables 8dot3name creations. So short paths will be equal to long paths that contain non-ASCII characters. But we can't use non-ASCII characters that are not in the current system codepage because nsIEnvironment.set does not support such characters. So I changed characters tha are used in the profile path. Try runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2197407cf6a49df2c675f6bcf8d3a08dd7f9d59&filter-searchStr=Windows%2010%20x64%20opt%20Xpcshell%20tests%20executed%20by%20TaskCluster%20test-windows10-64 (test only, failed) https://treeherder.mozilla.org/#/jobs?repo=try&revision=e01a1db5b51c686cec2d22b688fbb1273bd121d5&filter-searchStr=Windows%2010%20x64%20opt%20Xpcshell%20tests%20executed%20by%20TaskCluster%20test-windows10-64 (test + PSM patch, failed) https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f19086c1f0c1fe63a920075427788cceb95b006&filter-searchStr=Windows%2010%20x64%20opt%20Xpcshell%20tests%20executed%20by%20TaskCluster%20test-windows10-64 (test + PSM patch + NSS patch, succeeded)

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #14) > More importantly we need an automated test to check that this is working. Please see the latest attachment.

(In reply to Masatoshi Kimura [:emk] from comment #22) > If the profile path contains a character that is NOT in the current system > code page, NSS couldn't create cert8.db/key3.db even before this bug. But it > is a known problem (bug 336829). This bug is about the regression that even > non-ASCII characters that are in the current system code page break SSL > exceptions. ... > Please do not extend the bug scope endlessly. This regression is really > critical for Japanese users. On Japanese Windows, short paths can contain > non-ASCII characters, and user names are very likely to contain non-ASCII > characters due to Microsoft accounts. If all that's necessary here is to make overrides work again, I think the most limited-scope fix would be to change https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/security/manager/ssl/nsCertOverrideService.cpp#381 to not check the return value. This should be fine to do, since storing the certificate isn't even necessary - we match on hashes, not the full bytes of the certificates. I'm trying this out now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eba403ad03fb889ce63ba83fafa48ef7589e1c6 (should fail, except maybe on windows, ironically, since I'm using permissions, since I think it illustrates the issue more clearly than the ascii/codepage issue) https://treeherder.mozilla.org/#/jobs?repo=try&revision=157e80e55b038f4e44bd284cc0e151c1dddd2f50 (should succeed)

Looks like that hack works. I'd be fine with that as a hot fix but we should get this fixed properly eventually.

(In reply to David Keeler [:keeler] (use needinfo) from comment #27) > If all that's necessary here is to make overrides work again, I think the > most limited-scope fix would be to change > https://dxr.mozilla.org/mozilla-central/rev/ > 351c75ab74c9a83db5c0662ba271b49479adb1f1/security/manager/ssl/ > nsCertOverrideService.cpp#381 to not check the return value. This should be > fine to do, since storing the certificate isn't even necessary - we match on > hashes, not the full bytes of the certificates. The site load with this fix, but the Certificate Name is not displayed in the Certificate Manager. Moreover, Certificate Manager cannot import Client Certificates. Still severe regressions (although they would be less critical).

For the record, I found a relevant Windows setting. http://archives.miloush.net/michkap/archive/2006/07/25/676295.html > It came up because we had an interest in changing the defaults for the > NtfsAllowExtendedCharacterIn8dot3Name setting, documented as: > >> Specifies whether the characters from the extended character set, >> including diacritic characters, can be used in short file names using the >> 8.3 naming convention on NTFS volumes. >> >> Value Meaning >> 0 On NTFS volumes, file names using the 8.3 naming convention are >> limited to the standard ASCII character set (minus any reserved >> values). >> 1 On NTFS volumes, file names using the 8.3 naming convention may >> use extended characters. >> >> This entry does not exist in the registry by default. You can add it by >> using the registry editor Regedit.exe. > > Of course what is not mentioned in that informational topic is that years > ago it was decided that this value should be set anytime the default system > locale was Chinese, Japanese, or Korean (and unset anytime it wasn't).

Comment on attachment 8939720 [details] [diff] [review] Use UTF-8 file paths for NSS database Review of attachment 8939720 [details] [diff] [review]: ----------------------------------------------------------------- Let's move this to a separate bug that we can land once the NSS changes are made.

Comment on attachment 8939777 [details] [diff] [review] Reression test to make sure non-ASCII file paths work Review of attachment 8939777 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this is relevant at this point.

Comment on attachment 8940352 [details] bug 1427273 - don't require importing the server certificate for overrides to succeed https://reviewboard.mozilla.org/r/210622/#review216400 Looks good; I don't see anything that warrants changing.

Thanks! I'm actually going to move this piece to bug 1428498 so we don't confuse things here.

Comment on attachment 8939718 [details] [diff] [review] Disable sqldb when the profile path contains a non-ASCII character (beta only) Approval Request Comment [Feature/Bug causing the regression]: 783994 [User impact if declined]: Some users in CJK region cannot add SSL exception and cannot update certificate database using Certificate Manager. [Is this code covered by automated tests?]: Yes, included in the patch. [Has the fix been verified in Nightly?]: No, this change is specific to Beta. The fallback code path that this patch depends on was removed from Nightly. [Needs manual test from QE? If yes, steps to reproduce]: See comment #12. But it would be difficult to reproduce the problem on English locales. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: This patch only enables the fallback path for limited range of users that was used until Firefox 57. [String changes made/needed]: none

Comment on attachment 8939720 [details] [diff] [review] Use UTF-8 file paths for NSS database Review of attachment 8939720 [details] [diff] [review]: ----------------------------------------------------------------- > Let's move this to a separate bug that we can land once the NSS changes are made. OK, filed bug 1428538.

Comment on attachment 8939137 [details] [diff] [review] Fix NSS sdb to handle UTF-8 paths correctly on Windows No longer relevant here.

Comment on attachment 8939777 [details] [diff] [review] Reression test to make sure non-ASCII file paths work I'll re-attach this in bug 1428538 when NSS change is merged to m-c.

Hi :emk, Does this patch need to be in m-c or it's just for beta?

This patch is just for beta.

Comment on attachment 8939718 [details] [diff] [review] Disable sqldb when the profile path contains a non-ASCII character (beta only) Fix an issue that users can't add SSL exception and cannot update certificate database. Beta58+.

I believe this is fixed as all remaining work is in other bugs.

I managed to reproduce this bug using an older version of Nightly from 2017-12-28 on Windows 10 x64. I did all the steps from comment 12 and after that I opened Firefox with a new profile and performed all the steps from comment 0 using only japanese characters from the profile name. The browser didn't do anything and in Browser Console I got this error: TypeError: aField is null FormLikeFactory.jsm:109:1 NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICertOverrideService.rememberValidityOverride] I retested everything using the latest Nightly 59.0a1 and beta 58.0b15 on Windows 10 x64 and the bug is not reproducing anymore.