Closed
Bug 496013
Opened 15 years ago
Closed 15 years ago
Loading PKCS#11 module fails due to incorrect usage of nsiLocalFile.persistentDescriptor
Categories
(Core :: Security: PSM, defect)
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)
71.30 KB,
image/png
|
Details | |
1003 bytes,
patch
|
mayhemer
:
review+
dveditz
:
approval1.9.1.4+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
Reporter | ||
Comment 2•15 years ago
|
||
Nominating for blocking1.9.1 to get it evaluated.
Flags: blocking1.9.1?
Comment 3•15 years ago
|
||
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-
Assignee | ||
Comment 4•15 years ago
|
||
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
Assignee | ||
Comment 5•15 years ago
|
||
The problem is here: http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/device_manager.js#476 We shouldn't use fp.file.persistentDescriptor in a textbox users will be able to see and modify. See: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsILocalFile.idl#147
Assignee | ||
Comment 6•15 years ago
|
||
That's an easy one. Fix upcoming...
Assignee: kaie → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•15 years ago
|
||
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?
Assignee | ||
Updated•15 years ago
|
Attachment #383803 -
Flags: review? → review?(kaie)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [FFT3.5]
Assignee | ||
Updated•15 years ago
|
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
Comment 8•15 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #383803 -
Flags: review?(honzab.moz)
Comment 10•15 years ago
|
||
Comment on attachment 383803 [details] [diff] [review] Patch v1 [Checkin comment 17 & 25] Honza can do reviews in Kai's absence, IINM,
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #383803 -
Attachment is obsolete: true
Attachment #383803 -
Flags: review?(kaie)
Attachment #383803 -
Flags: review?(honzab.moz)
Comment 13•15 years ago
|
||
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?
Comment 14•15 years ago
|
||
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.
Comment 15•15 years ago
|
||
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 16•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #383803 -
Attachment is obsolete: false
Comment 17•15 years ago
|
||
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]
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 18•15 years ago
|
||
Fixed before 1.9.2 branching.
Assignee | ||
Comment 19•15 years ago
|
||
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
Assignee | ||
Comment 20•15 years ago
|
||
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?
Comment 21•15 years ago
|
||
(In reply to comment #19) > Have you filed a new bug already? Bug 506686.
Assignee | ||
Updated•15 years ago
|
status1.9.1:
--- → ?
Flags: wanted1.9.1.x? → blocking1.9.0.15?
Comment 23•15 years ago
|
||
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+
Updated•15 years ago
|
Flags: wanted1.9.0.x-
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 24•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Keywords: fixed1.9.2 → verified1.9.2
Updated•15 years ago
|
status1.9.2:
--- → beta1-fixed
Comment 25•15 years ago
|
||
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]
Updated•15 years ago
|
Assignee | ||
Comment 26•15 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•