274984 - libsoftokn3 fails to load libfreebl in setuid programs

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

In setuid programs, LD_LIBRARY_PATH is ignored . 
When libsoftokn3 tries to dlopen libfreebl, it fails .
RPATH is also not a solution here, because it is only considered for implicit
dependencies, not for dlopen .

There are a couple possible possible solutions :

1) when PR_LoadLibrary without a PATH fails, try again with a hard-coded PATH to
the Sun absolute location (/usr/lib/mps ). This would be code conditionally
compiled (BUILD_SUN_PKG).

2) if we can get this to work, try to obtain the load path of libsoftokn3.so at
runtime, and try to load libfreebl from that same location . This would probably
be accepted into the main NSS tree without conditional compilation, since it
wouldn't require hardcoding our Solaris/JES NSS shared location .

Note that building separate versions of libsoftokn3 and installing the right one
isn't an option, unfortunately, due to the way installers work and diskless
workstation cases which might not be exactly the same architecture as the server .

Comment 1

[Julien Pierre]

The function to use to get the current path for libsoftokn3.so is
PR_GetLibraryFilePathname . We would then construct a fully qualified path for
libfreebl from this information .

Comment 2

[Chris Newman]

Personally, I think /usr/lib/mps and /usr/lib/mps/secv1 should be added to the
list of trusted paths in /etc/ld.config on Solaris 10 (making this a Solaris bug
rather than an NSS bug).  Type "man crle" on Solaris 8+ and read about the "-s"
option.  That should also suggest the obvious workaround.

Comment 3

[Julien Pierre]

Thanks, Chris, I'll look at that . However, this problem might not be specific
to Solaris . We also have a libfreebl on HP-UX and AIX . I'm not sure if there
is a crle or equivalent on those platforms .

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

It appears to me that this bug was targetted at 3.9.5 but wasn't fixed 
in 3.9.5.  (or was it fixed and the fix merely was not reported here?)
If it wasn't fixed (as it appears), we should retarget it for another 
release.  Should we try to fix it for 3.10?

Comment 5

[Wan-Teh Chang]

This may have been fixed in NSS 3.9.3 as bug 258779.
(That fix was enhanced in bug 285237 for NSS 3.10 to
also work with AMD64.)  Julien or Saul needs to confirm
whether this is a duplicate or continuation of bug 258779.

Comment 6

[Julien Pierre]

I don't believe this was fixed. I don't think the RPATH applies to dlopen()
calls, only to implicit dependencies . We should make a test case to verify this.

Comment 7

[Saul Edwards]

Attachment: Attempt to load freebl from the same path as libsoftokn, v1

We are still considering other options (i.e. filter libraries) but here is a
working patch that determines the path from the current directory of libsoftokn
and successfully loads libfreebl without using rpath, ORIGIN, LD_LIBRARY_PATH,
etc.  Tested on Solaris only.  We will need testing on all other platforms,
especially if we go ahead with making freebl a DSO on all platforms.

Comment 8

[Robert Relyea]

Sigh after years of worrying about problems 'finding' the library and not having
anything happen, I finally started to feel confident that there wasn't a problem
with the 'load the library on the fly' issue.

question: if we go with static bindings, doesn't this problem go away (there is
still the issue of apps trying to call the binding directly, but that's a
separate problem).

bob

Comment 9

[Julien Pierre]

Bob,

If we use static binding, then we are limited to the lowest common denominator
instruction set. The shared libraries in Solaris contain information about the
instruction set (eg. MMX, SSE2, i386, sparcv8, sparcv8+, sparcv8+vis) . The OS
won't even load the program if any implicit dependency contains code that
doesn't run on the local chip . So, we would be able to use only i386 or
sparcv8, for example.

We also can't deal with the machine differences at install time by copying
files, because people are sharing OS directories with NSS libraries in them, and
doing remote boot, and the remote boot clients may have varying CPUs in them.

Comment 10

[Robert Relyea]

right, I knew there was a reason for dynamic loading...;).

Comment 11

[Julien Pierre]

Comment on attachment 190902 [details] [diff] [review]
Attempt to load freebl from the same path as libsoftokn, v1

Saul,

- There is a potential buffer overflow in your patch. You are using a string of
1024 bytes. You clearly thought about the problem because you have a commented
PR_ASSERT, but there is no test in your code.

You aren't checking if softoknPathSize is greater than that before you do the
first strncpy, or if the next strcpy appends too far.

IMO, the best solution would be to use dynamic allocation - just
malloc(strlen(softoknPath)+strlen(name)+1)
 .

- Also, if softoknPath is NULL, completeName is uninitialized, and thus may
cause your second bl_LoadLibrary call to crash, if the first one failed  .

Comment 12

[Wan-Teh Chang]

Comment on attachment 190902 [details] [diff] [review]
Attempt to load freebl from the same path as libsoftokn, v1

You should use PORT_Alloc to allocate the completeName buffer.

>+  /* Get the pathname for the loaded libsoftokn, i.e. /usr/lib/libsoftokn.so */
>+  fn_addr = PR_FindFunctionSymbolAndLibrary("NSC_GetFunctionList" , &lib_addr);
>+  softoknPath = PR_GetLibraryFilePathname("", fn_addr);

The first argument to PR_GetLibraryFilePathname should be
the file name of the softokn3 shared library (e.g., "libsoftokn3.so"
for most Unix platforms and "softokn3.dll" for Windows).

You can pass the address of a static function in this file
as the second argument to PR_GetLibraryFilePathname, which is
cheaper than the PR_FindFunctionSymbolAndLibrary call.	The
second argument just needs to be an address in this shared
library.

>   handle = bl_LoadLibrary(name);
>+  if (!handle) {
>+      handle = bl_LoadLibrary(completeName);
>+  }

Rather than making two bl_LoadLibrary calls, I suggest that you just
make the bl_LoadLibrary(completeName) call so that this code path is
always tested.

Comment 13

[Robert Relyea]

A safe way to get the correct name for the softoken shared library would be to
use the build system to add a -D flag. Softoken already has such a define:
SOFTOKEN_LIB_NAME
This handles name differences on other platforms.

bob

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Bob, is anything thought to be unsafe about this?

+  /* Get the pathname for the loaded libsoftokn, i.e. /usr/lib/libsoftokn.so */
+  fn_addr = PR_FindFunctionSymbolAndLibrary("NSC_GetFunctionList" , &lib_addr);
+  softoknPath = PR_GetLibraryFilePathname("", fn_addr);

Is this known to not work on some platforms?

Comment 15

[Robert Relyea]

Yes, Mac for one. The problem with implementing
PR_FindFunctionSymbolAndLibrary() on various platforms is different platforms
have different requirements for how to get a handle on the library in question.
Some get it by library name, some by address in the library, some the address
must be a function. The parameters for
PR_FindFunctionSymbolAndLibrary() were chosen to work on all platforms. Most
ignore one or the other parameter.

Comment 16

[Wan-Teh Chang]

Attachment: Patch for NSS_3_10_BRANCH

Saul:

I didn't have time to test this patch, but something
along this line should work.  Please test it and merge
the good ideas from your patch (attachment 190902 [details] [diff] [review]) into
this patch, and add a comment to explain why we need to
do this.

Comment 17

[Saul Edwards]

Comment on attachment 195487 [details] [diff] [review]
Patch for NSS_3_10_BRANCH

Looks good as is, and I tested on Solaris in various setuid cases.  I think the
comment should read something like "In setuid programs, Solaris is unwilling to
load freebl unless we use an absolute path.  We construct one from the path of
the library that contains this function."

Do you want me to check this in for you?

Comment 18

[Wan-Teh Chang]

Comment on attachment 195487 [details] [diff] [review]
Patch for NSS_3_10_BRANCH

Thank you, Saul.

Could you write a new patch with the comment you wrote,
and also use a local variable to store the value of
(slash - dli.dli_fname) or (slash - dli.dli_fname) + 1?
I put that expression in parenthese to help the compiler
find it as a common subexpression, but it would be good
to do that explicitly as you did in your patch.  Thanks.

Comment 19

[Saul Edwards]

Attachment: Patch for NSS_3_10_BRANCH with Wan-Teh's suggestions

Comment 20

[Wan-Teh Chang]

Comment on attachment 195810 [details] [diff] [review]
Patch for NSS_3_10_BRANCH with Wan-Teh's suggestions

r=wtc.

Please ask some other Sun developer to make sure your
comment accurately describes the problem we're fixing.

>+            size_t pathsize = slash - dli.dli_fname + 1;

"ptrdiff_t" is a better type than "size_t".

The variable's name probably should be "dirsize" or
"dirpathsize".

Comment 21

[Julien Pierre]

Comment on attachment 195810 [details] [diff] [review]
Patch for NSS_3_10_BRANCH with Wan-Teh's suggestions

The code looks OK as written.

I have a few of suggestions for improvements. Feel free to implement them or
not :

1) I may be paranoid here, but I think we should do more checks on the return
of dladdr.

I suggest that we should zero the DL_info structure so that dli.dli_fname is
NULL on entry.

Then, the if (dladdr ...) statement can have a second test added to it, &&
(dli.dli_fname)

This avoids possibly dereferencing an invalid pointer in the strrchr call .

This can only happen if dladdr has a bug, but you never know . bl_LoadLibrary
is a function that's rarely called, so the cost of doing it is very cheap.

2) I'm not sure if we should "return NULL;" in the case where the PR_Malloc
failed. Most likely a 2nd dlopen would still fail since we are out of memory.
But logically, I don't like the fact that we are short-circuiting the function
while trying to find the fully-qualified path. I would prefer to let it fall
through to the 2nd dlopen and let it fail there with PR_LOAD_LIBRARY_ERROR .

3) if you are going to ignore my comment 2) above, you might want to move the
PR_SetError call below the PR_Free and just before return NULL . 
This is because any NSPR call might internally reset the error code even if it
will itself not return an error - although it's probably not the case with
PR_Free specifically. If you didn't have the PR_Free(lib) call, you wouldn't
need the PR_SetError at all, since you are using PR_Malloc and it would be
responsible for PR_SetError.

4) nit: the declaration and assignments of pathname can be collapsed to a
single line

Comment 22

[Saul Edwards]

Attachment: Patch as checked in

Checked into 3.10 Branch:

Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.18.8.1; previous revision: 1.18
done

Comment 23

[Wan-Teh Chang]

Attachment: Finishing touches

On second thought, I think 'dirsize' probably should be a
size_t, so I changed it back.  I collapsed the declaration
and assignment of 'pathname' into one statement as Julien
suggested.

I've checked in this patch on the NSS_3_10_BRANCH for 3.10.2.

Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.18.8.2; previous revision: 1.18.8.1
done

Comment 24

[Wan-Teh Chang]

This bug has been fixed on the NSS trunk (NSS 3.11) by
a patch in bug 303508.  The patches in this bug report
fixed it on the NSS_3_10_BRANCH (NSS 3.10.2).