|
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) { |
|
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]" |
|
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()? |
|
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]" |
|
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]" |
|
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]" |
|
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. 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. |
|
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]" |
|
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. |
|
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]" |
| Powered by Nabble | Edit this page |
