Closed Bug 496013 Opened 11 years ago Closed 11 years ago

Loading PKCS#11 module fails due to incorrect usage of nsiLocalFile.persistentDescriptor

Categories

(Core :: Security: PSM, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .4-fixed

People

(Reporter: jbecerra, Assigned: whimboo)

References

Details

(Keywords: verified1.9.1, verified1.9.2, Whiteboard: [FFT3.5])

Attachments

(2 files)

While verifying bug 494899 using commetn #18, I noticed that if you want to Load a pkcs#11 device specifying a module filename using the Browse button you get a long string of characters, and this results in an alert message saying it wasn't able to load the module. Related to this is bug 496005. 

I tried loading a file (libnssckbi.dylib) I copied from the package contents onto the desktop.

Steps:
1. Install the latest Mac Fx3.5pre
2. Copy the libnssckbi.dylib file in the package contents (same directory as the firefox executable) and paste onto the desktop.
3. Go to Preferences/Advanced/Encryption and click on Security Devices
4. Click on the Load button, Browse and select the file you pasted onto the desktop.
5. Click OK

Expected: An file path should be inserted in the field after step 4, and a new module should be added to the list of devices.

Actual: A long string of characters is inserted in the field, and after you click OK you get an alert message saying the module was not loaded.

This has been around since at least Fx2.0.0.20.
Nominating for blocking1.9.1 to get it evaluated.
Flags: blocking1.9.1?
Not blocking 3.5 final, would take a fix for 3.5.x, should fix for .next.
Flags: wanted1.9.1.x?
Flags: blocking1.9.2+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Ehsan, I believe we show the nsIFile value here instead of the real path value, or?
Hardware: x86 → All
Version: unspecified → 1.9.1 Branch
That's an easy one. Fix upcoming...
Assignee: kaie → hskupin
Status: NEW → ASSIGNED
We shouldn't use persistentDescriptor but directly the path. That will fix the display and the correct module loading which fails too currently.
Attachment #383803 - Flags: review?
Attachment #383803 - Flags: review? → review?(kaie)
Whiteboard: [FFT3.5]
Summary: browsing for pkcs#11 module filename enters a long string of characters → Loading PKCS#11 module fails due to incorrect usage of nsiLocalFile.persistentDescriptor
(In reply to comment #4)
> Ehsan, I believe we show the nsIFile value here instead of the real path value,
> or?

I don't know much about this code, but I think the approach in your patch is correct.  And this problem is Mac-specific since I believe that persistentDescriptor is merely the path string on Windows and Linux, but on Mac it's some binary data uniquely identifying the file.

If you want to spend some more time on this, I think it would be a good idea to check this list <http://mxr.mozilla.org/mozilla-central/ident?i=persistentDescriptor&tree=mozilla-central&filter=&scriptidly=1> to make sure we're not making the same mistake elsewhere.
I'll not have time to check other components. It would be nice if someone other could do this.

Nelson, do you have a chance to review the patch? Kaie seems to be absent.
Attachment #383803 - Flags: review?(honzab.moz)
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]

Honza can do reviews in Kai's absence, IINM,
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]

nsIFile.path is AString (UTF-16). On one place we do ToNewCString that does LossyConvertEncoding on it. We have to convert because we pass it to NSPR that takes char*, but I don't think this is the correct way.

Please try with libs that has some unicode chars in a path to check the code is correct. On all platforms would be best. To have an automated test would be awesome.
Honza, thanks for your review. Since I'm not familiar with the string classes and it's not really my area of work I have to retire from this bug. I hope someone else can fix it in an appropriate way so that adding modules will work again.
Assignee: hskupin → kaie
Status: ASSIGNED → NEW
Attachment #383803 - Attachment is obsolete: true
Attachment #383803 - Flags: review?(kaie)
Attachment #383803 - Flags: review?(honzab.moz)
I checked the patch, we definitely cannot load modules with diacs in path.

I don't know (till these days) how to pass a path string from nsI(Local)File to NSPR. However, there is a way to change the SECMOD_LoadPKCS11Module function and mainly the SECMODModule structure to keep the string in unicode form and instead of PR_LoadLibary use PR_LoadLibraryWithFlags and fill the unicode variant of the path in PRLibSpec structure passed to that function. It's a lot of work, large overhaul of path handling in the whole nss.

Are there any tests or examples of code in NSS that work with diacritics in the path?
If the DLL name is in the local code page, you should be able to use GetNativePath. The other case is probably not worth fixing.
The change envisioned in comment 13 affects NSPR as well as NSS.
NSS's present "solution" is to use a UTF8 "code page".  That is, expect 
the "char *" file name to be UTF8.
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]

This patch fixes the immediate problem. I was trying to find out a simple way to make this also working for paths with diacritic characters but is seems to be a more complex problem. I will create a new bug for that issue.

For now, let's take this patch and fix this bug.

r=honzab
Attachment #383803 - Flags: review+
Attachment #383803 - Attachment is obsolete: false
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]

http://hg.mozilla.org/mozilla-central/rev/3d41797a4b2c
Attachment #383803 - Attachment description: Patch v1 → Patch v1 [Checkin comment 17]
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Fixed before 1.9.2 branching.
Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090728 Minefield/3.6a1pre ID:20090728032726

(In reply to comment #16)
> (From update of attachment 383803 [details] [diff] [review])
> This patch fixes the immediate problem. I was trying to find out a simple way
> to make this also working for paths with diacritic characters but is seems to
> be a more complex problem. I will create a new bug for that issue.

Have you filed a new bug already?
Assignee: kaie → hskupin
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]

Setting myself as assignee again. If we need a test for 1.9.1 someone else has to come up with it.
Attachment #383803 - Flags: approval1.9.1.3?
(In reply to comment #19)
> Have you filed a new bug already?

Bug 506686.
status1.9.1: --- → ?
Flags: wanted1.9.1.x? → blocking1.9.0.15?
Not blocking 1.9.0.x
Flags: blocking1.9.0.15?
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #383803 - Flags: approval1.9.1.3? → approval1.9.1.4+
Flags: wanted1.9.0.x-
Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)
Keywords: fixed1.9.2
Comment on attachment 383803 [details] [diff] [review]
Patch v1 [Checkin comment 17 & 25]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0c66347e8cc4
Attachment #383803 - Attachment description: Patch v1 [Checkin comment 17] → Patch v1 [Checkin comment 17 & 25]
Keywords: checkin-needed
Version: 1.9.1 Branch → Trunk
Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4pre) Gecko/20090907 Shiretoko/3.5.4pre ID:20090907040203
Keywords: verified1.9.1
Duplicate of this bug: 321258
You need to log in before you can comment on or make changes to this bug.