Quantcast

fdlopen(3)

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

fdlopen(3)

Konstantin Belousov
Hello,
below is the patch to add fdlopen(3) function to rtld. It allows to load
shared object referenced by the opened file descriptor. Intent is to allow
the caller to do some additional checks on the object before committing
to load it, in race-free manner.

The facility was requested by des@, who also tested the patch.

I will commit this shortly unless somebody objects.

diff --git a/include/dlfcn.h b/include/dlfcn.h
index 794fde1..c508843 100644
--- a/include/dlfcn.h
+++ b/include/dlfcn.h
@@ -118,6 +118,7 @@ void *dlopen(const char *, int);
 void *dlsym(void * __restrict, const char * __restrict);
 
 #if __BSD_VISIBLE
+void *fdlopen(int, int);
 int dladdr(const void * __restrict, Dl_info * __restrict);
 dlfunc_t dlfunc(void * __restrict, const char * __restrict);
 int dlinfo(void * __restrict, int, void * __restrict);
diff --git a/lib/libc/gen/Makefile.inc b/lib/libc/gen/Makefile.inc
index 1e068a5..11d1a58 100644
--- a/lib/libc/gen/Makefile.inc
+++ b/lib/libc/gen/Makefile.inc
@@ -95,8 +95,8 @@ MLINKS+=directory.3 closedir.3 directory.3 dirfd.3 directory.3 opendir.3 \
  directory.3 fdopendir.3 \
  directory.3 readdir.3 directory.3 readdir_r.3 directory.3 rewinddir.3 \
  directory.3 seekdir.3 directory.3 telldir.3
-MLINKS+=dlopen.3 dlclose.3 dlopen.3 dlerror.3 dlopen.3 dlfunc.3 \
- dlopen.3 dlsym.3
+MLINKS+=dlopen.3 fdlopen.3 dlopen.3 dlclose.3 dlopen.3 dlerror.3 \
+ dlopen.3 dlfunc.3 dlopen.3 dlsym.3
 MLINKS+=err.3 err_set_exit.3 err.3 err_set_file.3 err.3 errc.3 err.3 errx.3 \
  err.3 verr.3 err.3 verrc.3 err.3 verrx.3 err.3 vwarn.3 err.3 vwarnc.3 \
  err.3 vwarnx.3 err.3 warnc.3 err.3 warn.3 err.3 warnx.3
diff --git a/lib/libc/gen/Symbol.map b/lib/libc/gen/Symbol.map
index adc5964..e00e746 100644
--- a/lib/libc/gen/Symbol.map
+++ b/lib/libc/gen/Symbol.map
@@ -382,6 +382,7 @@ FBSD_1.2 {
 };
 
 FBSD_1.3 {
+ fdlopen;
  __FreeBSD_libc_enter_restricted_mode;
 };
 
diff --git a/lib/libc/gen/dlfcn.c b/lib/libc/gen/dlfcn.c
index b109cc9..7be9f87 100644
--- a/lib/libc/gen/dlfcn.c
+++ b/lib/libc/gen/dlfcn.c
@@ -147,6 +147,15 @@ dl_iterate_phdr(int (*callback)(struct dl_phdr_info *, size_t, void *),
  return 0;
 }
 
+#pragma weak fdlopen
+void *
+fdlopen(int fd, int mode)
+{
+
+ _rtld_error(sorry);
+ return NULL;
+}
+
 #pragma weak _rtld_atfork_pre
 void
 _rtld_atfork_pre(int *locks)
diff --git a/lib/libc/gen/dlopen.3 b/lib/libc/gen/dlopen.3
index 3da9b6e..871808b 100644
--- a/lib/libc/gen/dlopen.3
+++ b/lib/libc/gen/dlopen.3
@@ -32,11 +32,12 @@
 .\" @(#) dlopen.3 1.6 90/01/31 SMI
 .\" $FreeBSD$
 .\"
-.Dd July 7, 2009
+.Dd December 21, 2011
 .Dt DLOPEN 3
 .Os
 .Sh NAME
 .Nm dlopen ,
+.Nm fdlopen ,
 .Nm dlsym ,
 .Nm dlfunc ,
 .Nm dlerror ,
@@ -49,6 +50,8 @@
 .Ft void *
 .Fn dlopen "const char *path" "int mode"
 .Ft void *
+.Fn fdlopen "int fd" "int mode"
+.Ft void *
 .Fn dlsym "void * restrict handle" "const char * restrict symbol"
 .Ft dlfunc_t
 .Fn dlfunc "void * restrict handle" "const char * restrict symbol"
@@ -164,6 +167,32 @@ be interrogated with
 .Fn dlerror .
 .Pp
 The
+.Fn fdlopen
+function is similar to
+.Fn dlopen ,
+but it takes the file descriptor argument
+.Fa fd ,
+which is used for the file operations needed to load an object
+into the address space.
+The file descriptor
+.Fa fd
+is not closed by the function regardless a result of execution.
+The
+.Fa fd
+argument -1 is interpreted as a reference to the main
+executable of the process, similar to
+.Va NULL
+value for
+.Fa name
+for
+.Fn dlopen .
+The
+.Fn fdlopen
+function can be used by the code that needs to perform
+additional checks on the loaded objects, to prevent races with
+symlinking or renames.
+.Pp
+The
 .Fn dlsym
 function
 returns the address binding of the symbol described in the null-terminated
@@ -354,6 +383,7 @@ option to the C language compiler.
 .Sh ERRORS
 The
 .Fn dlopen ,
+.Fn fdlopen ,
 .Fn dlsym ,
 and
 .Fn dlfunc
diff --git a/libexec/rtld-elf/Symbol.map b/libexec/rtld-elf/Symbol.map
index 28b24a1..9ad6251 100644
--- a/libexec/rtld-elf/Symbol.map
+++ b/libexec/rtld-elf/Symbol.map
@@ -18,6 +18,10 @@ FBSD_1.0 {
     __tls_get_addr;
 };
 
+FBSD_1.3 {
+    fdlopen;
+};
+
 FBSDprivate_1.0 {
     _rtld_thread_init;
     _rtld_allocate_tls;
diff --git a/libexec/rtld-elf/rtld.c b/libexec/rtld-elf/rtld.c
index 6cddd15..e1b813a 100644
--- a/libexec/rtld-elf/rtld.c
+++ b/libexec/rtld-elf/rtld.c
@@ -83,7 +83,7 @@ static void digest_dynamic2(Obj_Entry *, const Elf_Dyn *, const Elf_Dyn *);
 static void digest_dynamic(Obj_Entry *, int);
 static Obj_Entry *digest_phdr(const Elf_Phdr *, int, caddr_t, const char *);
 static Obj_Entry *dlcheck(void *);
-static Obj_Entry *dlopen_object(const char *name, Obj_Entry *refobj,
+static Obj_Entry *dlopen_object(const char *name, int fd, Obj_Entry *refobj,
     int lo_flags, int mode);
 static Obj_Entry *do_load_object(int, const char *, char *, struct stat *, int);
 static int do_search_info(const Obj_Entry *obj, int, struct dl_serinfo *);
@@ -103,7 +103,7 @@ static void load_filtees(Obj_Entry *, int flags, RtldLockState *);
 static void unload_filtees(Obj_Entry *);
 static int load_needed_objects(Obj_Entry *, int);
 static int load_preload_objects(void);
-static Obj_Entry *load_object(const char *, const Obj_Entry *, int);
+static Obj_Entry *load_object(const char *, int fd, const Obj_Entry *, int);
 static void map_stacks_exec(RtldLockState *);
 static Obj_Entry *obj_from_addr(const void *);
 static void objlist_call_fini(Objlist *, Obj_Entry *, RtldLockState *);
@@ -120,6 +120,7 @@ static int resolve_objects_ifunc(Obj_Entry *first, bool bind_now,
     RtldLockState *lockstate);
 static int rtld_dirname(const char *, char *);
 static int rtld_dirname_abs(const char *, char *);
+static void *rtld_dlopen(const char *name, int fd, int mode);
 static void rtld_exit(void);
 static char *search_library_path(const char *, const char *);
 static const void **get_program_var_addr(const char *, RtldLockState *);
@@ -1543,7 +1544,7 @@ load_filtee1(Obj_Entry *obj, Needed_Entry *needed, int flags)
 {
 
     for (; needed != NULL; needed = needed->next) {
- needed->obj = dlopen_object(obj->strtab + needed->name, obj,
+ needed->obj = dlopen_object(obj->strtab + needed->name, -1, obj,
   flags, ((ld_loadfltr || obj->z_loadfltr) ? RTLD_NOW : RTLD_LAZY) |
   RTLD_LOCAL);
     }
@@ -1567,7 +1568,7 @@ process_needed(Obj_Entry *obj, Needed_Entry *needed, int flags)
     Obj_Entry *obj1;
 
     for (; needed != NULL; needed = needed->next) {
- obj1 = needed->obj = load_object(obj->strtab + needed->name, obj,
+ obj1 = needed->obj = load_object(obj->strtab + needed->name, -1, obj,
   flags & ~RTLD_LO_NOLOAD);
  if (obj1 == NULL && !ld_tracing && (flags & RTLD_LO_FILTEES) == 0)
     return (-1);
@@ -1614,7 +1615,7 @@ load_preload_objects(void)
 
  savech = p[len];
  p[len] = '\0';
- if (load_object(p, NULL, 0) == NULL)
+ if (load_object(p, -1, NULL, 0) == NULL)
     return -1; /* XXX - cleanup */
  p[len] = savech;
  p += len;
@@ -1624,6 +1625,13 @@ load_preload_objects(void)
     return 0;
 }
 
+static const char *
+printable_path(const char *path)
+{
+
+ return (path == NULL ? "<unknown>" : path);
+}
+
 /*
  * Load a shared object into memory, if it is not already loaded.
  *
@@ -1631,36 +1639,51 @@ load_preload_objects(void)
  * on failure.
  */
 static Obj_Entry *
-load_object(const char *name, const Obj_Entry *refobj, int flags)
+load_object(const char *name, int fd1, const Obj_Entry *refobj, int flags)
 {
     Obj_Entry *obj;
-    int fd = -1;
+    int fd;
     struct stat sb;
     char *path;
 
-    for (obj = obj_list->next;  obj != NULL;  obj = obj->next)
- if (object_match_name(obj, name))
-    return obj;
+    if (name != NULL) {
+ for (obj = obj_list->next;  obj != NULL;  obj = obj->next) {
+    if (object_match_name(obj, name))
+ return (obj);
+ }
 
-    path = find_library(name, refobj);
-    if (path == NULL)
- return NULL;
+ path = find_library(name, refobj);
+ if (path == NULL)
+    return (NULL);
+    } else
+ path = NULL;
 
     /*
-     * If we didn't find a match by pathname, open the file and check
-     * again by device and inode.  This avoids false mismatches caused
-     * by multiple links or ".." in pathnames.
+     * If we didn't find a match by pathname, or the name is not
+     * supplied, open the file and check again by device and inode.
+     * This avoids false mismatches caused by multiple links or ".."
+     * in pathnames.
      *
      * To avoid a race, we open the file and use fstat() rather than
      * using stat().
      */
-    if ((fd = open(path, O_RDONLY)) == -1) {
- _rtld_error("Cannot open \"%s\"", path);
- free(path);
- return NULL;
+    fd = -1;
+    if (fd1 == -1) {
+ if ((fd = open(path, O_RDONLY)) == -1) {
+    _rtld_error("Cannot open \"%s\"", path);
+    free(path);
+    return (NULL);
+ }
+    } else {
+ fd = dup(fd1);
+ if (fd == -1) {
+    _rtld_error("Cannot dup fd");
+    free(path);
+    return (NULL);
+ }
     }
     if (fstat(fd, &sb) == -1) {
- _rtld_error("Cannot fstat \"%s\"", path);
+ _rtld_error("Cannot fstat \"%s\"", printable_path(path));
  close(fd);
  free(path);
  return NULL;
@@ -1668,7 +1691,7 @@ load_object(const char *name, const Obj_Entry *refobj, int flags)
     for (obj = obj_list->next;  obj != NULL;  obj = obj->next)
  if (obj->ino == sb.st_ino && obj->dev == sb.st_dev)
     break;
-    if (obj != NULL) {
+    if (obj != NULL && name != NULL) {
  object_add_name(obj, name);
  free(path);
  close(fd);
@@ -1702,20 +1725,25 @@ do_load_object(int fd, const char *name, char *path, struct stat *sbp,
      */
     if (dangerous_ld_env) {
  if (fstatfs(fd, &fs) != 0) {
-    _rtld_error("Cannot fstatfs \"%s\"", path);
- return NULL;
+    _rtld_error("Cannot fstatfs \"%s\"", printable_path(path));
+    return NULL;
  }
  if (fs.f_flags & MNT_NOEXEC) {
     _rtld_error("Cannot execute objects on %s\n", fs.f_mntonname);
     return NULL;
  }
     }
-    dbg("loading \"%s\"", path);
-    obj = map_object(fd, path, sbp);
+    dbg("loading \"%s\"", printable_path(path));
+    obj = map_object(fd, printable_path(path), sbp);
     if (obj == NULL)
         return NULL;
 
-    object_add_name(obj, name);
+    /*
+     * If DT_SONAME is present in the object, digest_dynamic2 already
+     * added it to the object names.
+     */
+    if (name != NULL)
+ object_add_name(obj, name);
     obj->path = path;
     digest_dynamic(obj, 0);
     if (obj->z_noopen && (flags & (RTLD_LO_DLOPEN | RTLD_LO_TRACE)) ==
@@ -2211,6 +2239,20 @@ dllockinit(void *context,
 void *
 dlopen(const char *name, int mode)
 {
+
+ return (rtld_dlopen(name, -1, mode));
+}
+
+void *
+fdlopen(int fd, int mode)
+{
+
+ return (rtld_dlopen(NULL, fd, mode));
+}
+
+static void *
+rtld_dlopen(const char *name, int fd, int mode)
+{
     RtldLockState lockstate;
     int lo_flags;
 
@@ -2231,7 +2273,7 @@ dlopen(const char *name, int mode)
     if (ld_tracing != NULL)
     lo_flags |= RTLD_LO_TRACE;
 
-    return (dlopen_object(name, obj_main, lo_flags,
+    return (dlopen_object(name, fd, obj_main, lo_flags,
       mode & (RTLD_MODEMASK | RTLD_GLOBAL)));
 }
 
@@ -2246,7 +2288,8 @@ dlopen_cleanup(Obj_Entry *obj)
 }
 
 static Obj_Entry *
-dlopen_object(const char *name, Obj_Entry *refobj, int lo_flags, int mode)
+dlopen_object(const char *name, int fd, Obj_Entry *refobj, int lo_flags,
+    int mode)
 {
     Obj_Entry **old_obj_tail;
     Obj_Entry *obj;
@@ -2261,11 +2304,11 @@ dlopen_object(const char *name, Obj_Entry *refobj, int lo_flags, int mode)
 
     old_obj_tail = obj_tail;
     obj = NULL;
-    if (name == NULL) {
+    if (name == NULL && fd == -1) {
  obj = obj_main;
  obj->refcount++;
     } else {
- obj = load_object(name, refobj, lo_flags);
+ obj = load_object(name, fd, refobj, lo_flags);
     }
 
     if (obj) {

attachment0 (203 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Xin LI-5
Hi,

On Thu, Dec 29, 2011 at 12:43 AM, Kostik Belousov <[hidden email]> wrote:
> Hello,
> below is the patch to add fdlopen(3) function to rtld. It allows to load
> shared object referenced by the opened file descriptor. Intent is to allow
> the caller to do some additional checks on the object before committing
> to load it, in race-free manner.
>
> The facility was requested by des@, who also tested the patch.
>
> I will commit this shortly unless somebody objects.

Will this prevent e.g. writes to the .so file after open, but before fdlopen()?

Cheers,
--
Xin LI <[hidden email]> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Konstantin Belousov
On Thu, Dec 29, 2011 at 03:15:09AM -0800, Xin LI wrote:

> Hi,
>
> On Thu, Dec 29, 2011 at 12:43 AM, Kostik Belousov <[hidden email]> wrote:
> > Hello,
> > below is the patch to add fdlopen(3) function to rtld. It allows to load
> > shared object referenced by the opened file descriptor. Intent is to allow
> > the caller to do some additional checks on the object before committing
> > to load it, in race-free manner.
> >
> > The facility was requested by des@, who also tested the patch.
> >
> > I will commit this shortly unless somebody objects.
>
> Will this prevent e.g. writes to the .so file after open, but before fdlopen()?
How can it ? Even in theory ?

attachment0 (203 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Dag-Erling Smørgrav
In reply to this post by Xin LI-5
Xin LI <[hidden email]> writes:
> Will this prevent e.g. writes to the .so file after open, but before
> fdlopen()?

The latest version of OpenPAM checks the ownership and permissions of
modules before it loads them; it will not load modules that are writable
by anyone except root and the process's euid.  This patch prevents an
attacker from switching the .so file between the ownership checks and the
dlopen(3) call.

DES
--
Dag-Erling Smørgrav - [hidden email]
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Xin LI-5
In reply to this post by Konstantin Belousov
On Thu, Dec 29, 2011 at 3:38 AM, Kostik Belousov <[hidden email]> wrote:

> On Thu, Dec 29, 2011 at 03:15:09AM -0800, Xin LI wrote:
>> Hi,
>>
>> On Thu, Dec 29, 2011 at 12:43 AM, Kostik Belousov <[hidden email]> wrote:
>> > Hello,
>> > below is the patch to add fdlopen(3) function to rtld. It allows to load
>> > shared object referenced by the opened file descriptor. Intent is to allow
>> > the caller to do some additional checks on the object before committing
>> > to load it, in race-free manner.
>> >
>> > The facility was requested by des@, who also tested the patch.
>> >
>> > I will commit this shortly unless somebody objects.
>>
>> Will this prevent e.g. writes to the .so file after open, but before fdlopen()?
>
> How can it ? Even in theory ?

e.g. process 1 open(), check for permissions, etc., then process 2
open() for write, write something, then process proceed with
fdlopen()?  (We do not have mandatory "lock" on file handles to
prevent writing I think?).

But no, this is not a "major" concern and yes, I think fdlopen() is
something good to have.

Cheers,
--
Xin LI <[hidden email]> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Dag-Erling Smørgrav
Xin LI <[hidden email]> writes:
> Kostik Belousov <[hidden email]> writes:
> > Xin LI <[hidden email]> writes:
> > > Will this prevent e.g. writes to the .so file after open, but
> > > before fdlopen()?
> > How can it ? Even in theory ?
> e.g. process 1 open(), check for permissions, etc., then process 2
> open() for write, write something, then process proceed with
> fdlopen()?  (We do not have mandatory "lock" on file handles to
> prevent writing I think?).

I think Kostik meant "how could you, even in theory, prevent this from
happening", and the answer is that you can't.

DES
--
Dag-Erling Smørgrav - [hidden email]
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Konstantin Belousov
On Fri, Dec 30, 2011 at 02:12:29PM +0100, Dag-Erling Sm??rgrav wrote:

> Xin LI <[hidden email]> writes:
> > Kostik Belousov <[hidden email]> writes:
> > > Xin LI <[hidden email]> writes:
> > > > Will this prevent e.g. writes to the .so file after open, but
> > > > before fdlopen()?
> > > How can it ? Even in theory ?
> > e.g. process 1 open(), check for permissions, etc., then process 2
> > open() for write, write something, then process proceed with
> > fdlopen()?  (We do not have mandatory "lock" on file handles to
> > prevent writing I think?).
>
> I think Kostik meant "how could you, even in theory, prevent this from
> happening", and the answer is that you can't.
Sorry if it was not clear enough. BSD is not SysV, we do not have
mandatory locking. Some people consider it a feature.

In principle, fdlopen() can take additional argument struct timespec mtim,
that would be compared with st_mtim from fstat(2) right before calling
constructors from the dso. The passed mtim shall be taken from fstat(2)
before the application code starts its checks.
But this is completely ridiculous and pointless.

Presented use case for fdlopen(3) is valid and useful IMO.

attachment0 (203 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Poul-Henning Kamp
In message <[hidden email]>, Kostik Belousov
writes:

>Presented use case for fdlopen(3) is valid and useful IMO.

I agree, I even have a similar use-case in Varnish.

Has anybody racked their brains to make sure this doesn't have security
implications ?

--
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
[hidden email]         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Konstantin Belousov
On Fri, Dec 30, 2011 at 04:00:39PM +0000, Poul-Henning Kamp wrote:
> In message <[hidden email]>, Kostik Belousov
> writes:
>
> >Presented use case for fdlopen(3) is valid and useful IMO.
>
> I agree, I even have a similar use-case in Varnish.
>
> Has anybody racked their brains to make sure this doesn't have security
> implications ?

I am wondering what kind of security consequences you have in mind ?

My initial concern with the patch was the lack of the name supplied for
the loaded dso. But, the rtld already adds DT_SONAME to the names of the
object, and later it checks for duplicates using vnode identity returned
by fstat(2), so I removed the name argument from API.

attachment0 (203 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate
star

Re: fdlopen(3)

Poul-Henning Kamp
In message <[hidden email]>, Kostik Belousov
writes:

>> Has anybody racked their brains to make sure this doesn't have security
>> implications ?
>
>I am wondering what kind of security consequences you have in mind ?

I don't have anything specifically in mind, but adding a new way to
communicate executable code always trigger my "is that really a good
idea" detector...


--
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
[hidden email]         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Loading...