Closed Bug 712579 Opened 9 years ago Closed 9 years ago

Load nssckbi from the location of nss3

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: [inbound][qa-])

Attachments

(2 files, 1 obsolete file)

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
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.
Attachment #584758 - Flags: review?(kaie)
Attachment #584530 - Attachment is obsolete: true
Attachment #584530 - Flags: review?(kaie)
Attachment #584530 - Attachment description: Load nssckbi from the location of nss3 → [v1] Load nssckbi from the location of nss3
Attachment #584530 - Attachment is obsolete: false
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)
(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 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.
Blocks: 486716
https://hg.mozilla.org/mozilla-central/rev/345863ffe8c8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
[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+
Whiteboard: [inbound] → [inbound][qa-]
You need to log in before you can comment on or make changes to this bug.