Load nssckbi from the location of nss3

RESOLVED FIXED in Firefox 11

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed)

Details

(Whiteboard: [inbound][qa-])

Attachments

(2 attachments, 1 obsolete attachment)

Currently, PSM tries to load nssckbi from the application and GRE directories, before trying in the system directories. With the upcoming new linker for android in bug 683127, nssckbi will be in none of these locations. However, it will still be wherever nss3 is loaded from.

The PR_GetLibraryFilePathname function can allow us to get the location of the nss3 library, and try to load nssckbi from there.
OS: Linux → All
Hardware: x86_64 → All
Created attachment 584530 [details] [diff] [review]
[v1] Load nssckbi from the location of nss3

This works on linux, mac and windows:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-84d52bf7b258/
Note these builds have an additional patch that also removes the current search locations, to check that the new one is the one being hit.
On android, this new search location alone doesn't work, but searching in NS_XPCOM_CURRENT_PROCESS_DIR and NS_GRE_DIR already fails today, so in fact it already falls back to load without a path and getting what the custom linker already loaded. It will however work with the new linker (bug 683127).
Attachment #584530 - Flags: review?(kaie)
Is it possible for an addon or plugin to to load its own module also named nss3, such that PR_GetLibraryFilePathname could return the path to that library instead of the path to NSS's libnss3?

Is the new linker only to be used on Android? Or also on Linux?

Does this patch work correctly when the path to libnss3 contains non-ASCII characters?
(In reply to Brian Smith (:bsmith) from comment #2)
> Is it possible for an addon or plugin to to load its own module also named
> nss3, such that PR_GetLibraryFilePathname could return the path to that
> library instead of the path to NSS's libnss3?

On Linux, it is definitely not possible, because PR_GetLibraryFilePathname uses dladdr on a NSS function, which symbol is bound when libxul is loaded. On Mac, that would depend in what order libraries are returned by _dyld_get_image_name, and I guess the last loaded ones would be last, making the first NSS loaded the one being returned. On Windows, GetModuleHandleW is used, which documentation says that the behavior is undefined if there are several modules loaded with the same name. Not sure what that means in practice.

> Is the new linker only to be used on Android? Or also on Linux?

Only Android at first, but it can be used on Linux for easier debugging, and may be used in releases in the future.

> Does this patch work correctly when the path to libnss3 contains non-ASCII
> characters?

I must say I didn't test that, but that can surely be tested.

Note that if finding libnssckbi.so fails this way, we still fall back to loading from the GRE and APP directories, so even if the part using PR_GetLibraryFilePathname fails, it should still work.

Anyways, if there's concern over the behavior on Windows and Mac, we can #ifdef MOZ_LINKER and add the relevant define in Makefile.in. Would you prefer that?
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Brian Smith (:bsmith) from comment #2)

> Anyways, if there's concern over the behavior on Windows and Mac, we can
> #ifdef MOZ_LINKER and add the relevant define in Makefile.in. Would you
> prefer that?

I think that would be better, since the behavior is, at best, unclear on those platforms. It would also be good to #error in this code if MOZ_LINKER is defined and the platform isn't Android or Linux, so that we know to revisit this code in case we ever implement MOZ_LINKER on other platforms.

What are the possible directories that libnss could be in on Android and on Linux when the new linker is used? Are these directories that only the current app (on Android) or current user (on Linux) can write to?
They are in no directory, they are in a zip.
Created attachment 584758 [details] [diff] [review]
Load nssckbi from the location of nss3.
Attachment #584758 - Flags: review?(kaie)
Attachment #584530 - Attachment is obsolete: true
Attachment #584530 - Flags: review?(kaie)

Updated

5 years ago
Attachment #584530 - Attachment description: Load nssckbi from the location of nss3 → [v1] Load nssckbi from the location of nss3

Updated

5 years ago
Attachment #584530 - Attachment is obsolete: false

Comment 7

5 years ago
Comment on attachment 584758 [details] [diff] [review]
Load nssckbi from the location of nss3.

I think we don't need the additional changes in patch v2.
I like v1 better.
Marking v2 as obsolete.

The strategy used in patch v1 has been proposed in bug 486716.
We should use it on all platforms.
Attachment #584758 - Attachment is obsolete: true
Attachment #584758 - Flags: review?(kaie)

Comment 8

5 years ago
(In reply to Brian Smith (:bsmith) from comment #2)
> Is it possible for an addon or plugin to to load its own module also named
> nss3, such that PR_GetLibraryFilePathname could return the path to that
> library instead of the path to NSS's libnss3?
> 
> Does this patch work correctly when the path to libnss3 contains non-ASCII
> characters?


We shouldn't have to worry about these theoretical issues.

According to Bob, this strategy is already used by NSS to find one of the other core crypto modules of NSS (freebl).

Comment 9

5 years ago
Comment on attachment 584530 [details] [diff] [review]
[v1] Load nssckbi from the location of nss3

>+      if (possible_ckbi_locations[il] == nss_lib) {
>+        // Get the location of the nss3 library.
>+        char *library_name = PR_GetLibraryName(nsnull, nss_lib);


I would another non-null check.
However, in bug 486716, Bob proposes to use

  static const char *library_name = SHLIB_PREFIX"nss3."SHLIB_SUFFIX;  

If you like this approach, you could get rid of the function call and avoid having to add another non-null check.


>+        char *nss_path = PR_GetLibraryFilePathname(library_name, (PRFuncPtr) NSS_Initialize);
>+        PR_FreeLibraryName(library_name);


and yould avoid alloc/free, too.


r=kaie with this change.

Updated

5 years ago
Blocks: 486716
https://hg.mozilla.org/integration/mozilla-inbound/rev/345863ffe8c8
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/345863ffe8c8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Created attachment 591498 [details] [diff] [review]
Load nssckbi from the location of nss3 - for aurora

[Approval Request Comment]
This is a dependency of bug 683127 (bug 683127 doesn't work without this). It adds a location where PSM looks for nssckbi, which is where nss3 was loaded from. This fixes bug 486716. It has been on m-c for 2 weeks.
Attachment #591498 - Flags: review+
Attachment #591498 - Flags: approval-mozilla-aurora?
Attachment #591498 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/06ee9e6ba425
status-firefox11: --- → fixed
Whiteboard: [inbound] → [inbound][qa-]
You need to log in before you can comment on or make changes to this bug.