Re: more fexecve questions
Please add a comment explaining that this is for pathnames we can't
resolve in chroots.
> @@ -224,9 +225,11 @@ elf_populate_auxv(struct lwp *l, struct
> a->a_v = l->l_proc->p_stackbase;
> a++;
>
> - execname = a;
> - a->a_type = AT_SUN_EXECNAME;
> - a++;
> + if (path[0] == '/' && path[1] != '\0') {
> + execname = a;
> + a->a_type = AT_SUN_EXECNAME;
> + a++;
> + }
>
> exec_free_emul_arg(pack);
> } else {
segvguard and veriexec ony use the name for printing error messages.
I don't think it's necessary to give up on them because of it.
segvguard even handles a NULL name already.
> + if (epp->ep_resolvedname) {
> #if NVERIEXEC > 0
> - error = veriexec_verify(l, vp, epp->ep_resolvedname,
> - epp->ep_flags & EXEC_INDIR ? VERIEXEC_INDIRECT : VERIEXEC_DIRECT,
> - NULL);
> - if (error)
> - goto bad2;
> + error = veriexec_verify(l, vp, epp->ep_resolvedname,
> + epp->ep_flags & EXEC_INDIR ? VERIEXEC_INDIRECT
> + : VERIEXEC_DIRECT, NULL);
> + if (error)
> + goto bad2;
> #endif /* NVERIEXEC > 0 */
>
> #ifdef PAX_SEGVGUARD
> - error = pax_segvguard(l, vp, epp->ep_resolvedname, false);
> - if (error)
> - goto bad2;
> + error = pax_segvguard(l, vp, epp->ep_resolvedname, false);
> + if (error)
> + goto bad2;
> #endif /* PAX_SEGVGUARD */
> + }
>
We really should stop stubbing out stuff like this. I wouldn't be
surprised if working around things would be easier if we didn't have the
function they're (rightly) feature-testing for.
(I had similar issues with fallocate...)
> @@ -559,7 +598,8 @@ sys_fexecve(struct lwp *l, const struct
> syscallarg(char * const *) envp;
> } */
>
> - return ENOSYS;
> + return execve1(l, NULL, SCARG(uap, fd), SCARG(uap, argp),
> + SCARG(uap, envp), execve_fetch_element);
> }
>
> /*
Re: more fexecve questions
In article <20190910145247.c52cc17f...@rebar.astron.com>,
Christos Zoulas wrote:
1. So the consensus is to leave the file descriptor alone.
2. I've added the reverse cache lookup, and now script execution works.
3. Nobody suggested anything to improve security.
Here's the latest patch; if I don't hear objections soon I will commit it...
Index: compat/netbsd32/netbsd32_execve.c
===
RCS file: /cvsroot/src/sys/compat/netbsd32/netbsd32_execve.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 netbsd32_execve.c
--- compat/netbsd32/netbsd32_execve.c 3 Sep 2018 16:29:29 - 1.39
+++ compat/netbsd32/netbsd32_execve.c 15 Sep 2019 00:14:10 -
@@ -71,9 +71,8 @@ netbsd32_execve(struct lwp *l, const str
syscallarg(netbsd32_charpp) argp;
syscallarg(netbsd32_charpp) envp;
} */
- const char *path = SCARG_P32(uap, path);
- return execve1(l, path, SCARG_P32(uap, argp),
+ return execve1(l, SCARG_P32(uap, path), -1, SCARG_P32(uap, argp),
SCARG_P32(uap, envp), netbsd32_execve_fetch_element);
}
@@ -86,13 +85,9 @@ netbsd32_fexecve(struct lwp *l, const st
syscallarg(netbsd32_charpp) argp;
syscallarg(netbsd32_charpp) envp;
} */
- struct sys_fexecve_args ua;
- NETBSD32TO64_UAP(fd);
- NETBSD32TOP_UAP(argp, char * const);
- NETBSD32TOP_UAP(envp, char * const);
-
- return sys_fexecve(l, , retval);
+ return execve1(l, NULL, SCARG(uap, fd), SCARG_P32(uap, argp),
+ SCARG_P32(uap, envp), netbsd32_execve_fetch_element);
}
static int
Index: kern/exec_elf.c
===
RCS file: /cvsroot/src/sys/kern/exec_elf.c,v
retrieving revision 1.98
diff -u -p -u -r1.98 exec_elf.c
--- kern/exec_elf.c 7 Jun 2019 23:35:52 - 1.98
+++ kern/exec_elf.c 15 Sep 2019 00:14:10 -
@@ -157,6 +157,7 @@ elf_populate_auxv(struct lwp *l, struct
size_t len, vlen;
AuxInfo ai[ELF_AUX_ENTRIES], *a, *execname;
struct elf_args *ap;
+ char *path = l->l_proc->p_path;
int error;
a = ai;
@@ -224,9 +225,11 @@ elf_populate_auxv(struct lwp *l, struct
a->a_v = l->l_proc->p_stackbase;
a++;
- execname = a;
- a->a_type = AT_SUN_EXECNAME;
- a++;
+ if (path[0] == '/' && path[1] != '\0') {
+ execname = a;
+ a->a_type = AT_SUN_EXECNAME;
+ a++;
+ }
exec_free_emul_arg(pack);
} else {
@@ -242,7 +245,6 @@ elf_populate_auxv(struct lwp *l, struct
KASSERT(vlen <= sizeof(ai));
if (execname) {
- char *path = l->l_proc->p_path;
execname->a_v = (uintptr_t)(*stackp + vlen);
len = strlen(path) + 1;
if ((error = copyout(path, (*stackp + vlen), len)) != 0)
Index: kern/exec_script.c
===
RCS file: /cvsroot/src/sys/kern/exec_script.c,v
retrieving revision 1.79
diff -u -p -u -r1.79 exec_script.c
--- kern/exec_script.c 27 Jan 2019 02:08:43 - 1.79
+++ kern/exec_script.c 15 Sep 2019 00:14:10 -
@@ -290,7 +290,7 @@ check_shell:
/* try loading the interpreter */
if ((error = exec_makepathbuf(l, shellname, UIO_SYSSPACE,
_pathbuf, NULL)) == 0) {
- error = check_exec(l, epp, shell_pathbuf);
+ error = check_exec(l, epp, shell_pathbuf, NULL);
pathbuf_destroy(shell_pathbuf);
}
Index: kern/kern_exec.c
===
RCS file: /cvsroot/src/sys/kern/kern_exec.c,v
retrieving revision 1.479
diff -u -p -u -r1.479 kern_exec.c
--- kern/kern_exec.c7 Sep 2019 15:34:44 - 1.479
+++ kern/kern_exec.c15 Sep 2019 00:14:10 -
@@ -257,7 +257,7 @@ struct execve_data {
struct ps_strings ed_arginfo;
char*ed_argp;
const char *ed_pathstring;
- char*ed_resolvedpathbuf;
+ char*ed_resolvedname;
size_t ed_ps_strings_sz;
int ed_szsigcode;
size_t ed_argslen;
@@ -309,9 +309,32 @@ exec_path_free(struct execve_data *data)
{
pathbuf_stringcopy_put(data->ed_pathbuf, data->ed_pathstring);
pathbuf_destroy(data->ed_pathbuf);
- PNBUF_PUT(data->ed_resolvedpathbuf);
+ if (data->ed_resolvedname)
+ PNBUF_PUT(data->ed_resolvedname);
}
+static void
+exec_resolvename(struct lwp *l, struct exec_package *epp, struct vnode *vp,
+char **rpath)
+{
+ int error;
+ char *p;
+
+ KASSERT(rpath != NULL);
+
+ *rpath =
Re: more fexecve questions
On Wed, Sep 11, 2019 at 06:34:11AM +0300, Jason Thorpe wrote: > > The implementation I posted requires O_EXEC because this is the only way > > to set FEXEC which is part of the check_exec() tests. Of course we can > > elide this test and not require it in the fd case. It just seems nicely > > symmetric to me the way it is now. > > It may be "nicely symmetrical", but it's not a correct > implementation. O_RDONLY (without O_EXEC) is explicitly allowed. ...by a POSIX_MISTAKE :-) -- David A. Holland dholl...@netbsd.org
Re: more fexecve questions
> On Sep 11, 2019, at 4:37 AM, Christos Zoulas wrote: > > The implementation I posted requires O_EXEC because this is the only way > to set FEXEC which is part of the check_exec() tests. Of course we can > elide this test and not require it in the fd case. It just seems nicely > symmetric to me the way it is now. It may be "nicely symmetrical", but it's not a correct implementation. O_RDONLY (without O_EXEC) is explicitly allowed. -- thorpej
Re: more fexecve questions
In article <20190910195235.gp...@irregular-apocalypse.k.bsd.de>, Christoph Badura wrote: >On Tue, Sep 10, 2019 at 09:45:50PM +0200, Christoph Badura wrote: >> This is to catch the following case(s): The file might have the >> following permissions "--x--x--x" (or equivalent where the open()ing >> process doesn't have read or write permission). Since you can open such > ^can't open such >> a file for reading or writing (except as root) you need an additional >> flag to open() to obtain a file descriptor that can be passed to >> fexecve(). That's what O_EXEC is for. Whether the receiving process >> can successfully fexecve(2) such a file descriptor is another matter. >> That's what the "execute permission is checked by fexecve()" specifies. The implementation I posted requires O_EXEC because this is the only way to set FEXEC which is part of the check_exec() tests. Of course we can elide this test and not require it in the fd case. It just seems nicely symmetric to me the way it is now. christos
Re: more fexecve questions
>> [...O_EXEC...] It sounds to me as though O_EXEC would be more honestly spelled something more like O_NOACCESS. It would also be useful to obtain a descriptor on a search-only directory for fchdir() purposes, which was the motivation that led me to add O_NOACCESS to my own systems. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: more fexecve questions
On Tue, Sep 10, 2019 at 09:45:50PM +0200, Christoph Badura wrote: > This is to catch the following case(s): The file might have the > following permissions "--x--x--x" (or equivalent where the open()ing > process doesn't have read or write permission). Since you can open such ^can't open such > a file for reading or writing (except as root) you need an additional > flag to open() to obtain a file descriptor that can be passed to > fexecve(). That's what O_EXEC is for. Whether the receiving process > can successfully fexecve(2) such a file descriptor is another matter. > That's what the "execute permission is checked by fexecve()" specifies. > > --chris
Re: more fexecve questions
On Tue, Sep 10, 2019 at 07:31:47PM +0200, Kamil Rytarowski wrote: > On 10.09.2019 18:21, David Holland wrote: > > > O_EXEC should be tunable in runtime, with: > > > fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_EXEC); > > Why? You can't do that with O_WRITE. > I don't know. I was looking for a corner case when we would skip this > cache of O_EXEC on fexecve(). > POSIX states: > https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > > 'Since execute permission is checked by fexecve(), the file description > fd need not have been opened with the O_EXEC flag. ' > 'However, if the file to be executed denies read and write permission > for the process preparing to do the exec, the only way to provide the fd > to fexecve() will be to use the O_EXEC flag when opening fd. In this > case, the application will not be able to perform a checksum test since > it will not be able to read the contents of the file.' This is to catch the following case(s): The file might have the following permissions "--x--x--x" (or equivalent where the open()ing process doesn't have read or write permission). Since you can open such a file for reading or writing (except as root) you need an additional flag to open() to obtain a file descriptor that can be passed to fexecve(). That's what O_EXEC is for. Whether the receiving process can successfully fexecve(2) such a file descriptor is another matter. That's what the "execute permission is checked by fexecve()" specifies. --chris
Re: more fexecve questions
> On Sep 10, 2019, at 8:31 PM, Kamil Rytarowski wrote: > > I don't know. I was looking for a corner case when we would skip this > cache of O_EXEC on fexecve(). > > POSIX states: > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > > 'Since execute permission is checked by fexecve(), the file description > fd need not have been opened with the O_EXEC flag. ' Right, I understand that O_EXEC is there for binaries that are "-r-x--x--x" and you're just some random "other" user that wants to exec the file. Obviously you have to be able to open it to get a descriptor, so O_EXEC is there for this case ... such a descriptor grants neither read nor write, but it allows you to get a descriptor, and a descriptor + "x"-permission-for-you on the file is all you need to fexecve() (just as if you were using regular execve()). > but it is unclear to me later: > > 'However, if the file to be executed denies read and write permission > for the process preparing to do the exec, the only way to provide the fd > to fexecve() will be to use the O_EXEC flag when opening fd. In this > case, the application will not be able to perform a checksum test since > it will not be able to read the contents of the file.' "the application" ... of course nothing precludes the kernel (or its delegate) from performing a code signing check in this case. -- thorpej
Re: more fexecve questions
> On Sep 10, 2019, at 9:26 PM, Christos Zoulas wrote: > > I think is time to ditch NCHNAMLEN like FreeBSD did, and rely on > the namei cache for reverse mappings. This way we can also implement > F_GETPATH which Kamil seems to keep bringing up :-). FWIW, XNU's VFS uses this approach as well. There are cases where it will call into the file system to get the name associated with a vnode, but it uses name cache data pretty frequently (and F_GETPATH is pretty heavily used on platforms that use XNU). -- thorpej
Re: more fexecve questions
In article <20190910162429.a525e60...@jupiter.mumble.net>, Taylor R Campbell wrote: >> >> I guess we could, since the problem is that if the filename is greater than >> NCHNAMLEN, they will not be cached. But I am not familiar enough with the >> cache to make the changes required. > >I don't mean using the vnode cache. I mean finding somewhere else to >hang a cached copy of the executable and p_comm on the struct fdfile >or something. Maybe a global table mapping fdfile pointer to cached >stuff if we don't want to make the struct larger for non-O_EXEC fds. While this is possible, it creates even more complexity, it would require hanging something from struct vnode or some global map which will need hooks everywhere (and a place for bugs and abuse). I think is time to ditch NCHNAMLEN like FreeBSD did, and rely on the namei cache for reverse mappings. This way we can also implement F_GETPATH which Kamil seems to keep bringing up :-). It does not look too complicated. christos
Re: more fexecve questions
On 10.09.2019 18:21, David Holland wrote: > On Tue, Sep 10, 2019 at 06:11:55PM +0200, Kamil Rytarowski wrote: > > O_EXEC should be tunable in runtime, with: > > > > fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_EXEC); > > Why? You can't do that with O_WRITE. > I don't know. I was looking for a corner case when we would skip this cache of O_EXEC on fexecve(). POSIX states: https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html 'Since execute permission is checked by fexecve(), the file description fd need not have been opened with the O_EXEC flag. ' but it is unclear to me later: 'However, if the file to be executed denies read and write permission for the process preparing to do the exec, the only way to provide the fd to fexecve() will be to use the O_EXEC flag when opening fd. In this case, the application will not be able to perform a checksum test since it will not be able to read the contents of the file.' signature.asc Description: OpenPGP digital signature
Re: more fexecve questions
> Date: Tue, 10 Sep 2019 16:03:23 - (UTC) > From: chris...@astron.com (Christos Zoulas) > > In article <20190910150418.becab60...@jupiter.mumble.net>, > Taylor R Campbell wrote: > >Can we just cache these when the file descriptor is opened with > >O_EXEC? > > > >The cache could become stale if the executable or any parent directory > >is moved or deleted. But they could become stale after exec too for > >the same reason. > > I guess we could, since the problem is that if the filename is greater than > NCHNAMLEN, they will not be cached. But I am not familiar enough with the > cache to make the changes required. I don't mean using the vnode cache. I mean finding somewhere else to hang a cached copy of the executable and p_comm on the struct fdfile or something. Maybe a global table mapping fdfile pointer to cached stuff if we don't want to make the struct larger for non-O_EXEC fds.
Re: more fexecve questions
On Tue, Sep 10, 2019 at 06:11:55PM +0200, Kamil Rytarowski wrote: > O_EXEC should be tunable in runtime, with: > > fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_EXEC); Why? You can't do that with O_WRITE. -- David A. Holland dholl...@netbsd.org
Re: more fexecve questions
On 10.09.2019 17:04, Taylor R Campbell wrote: >> Date: Tue, 10 Sep 2019 10:52:47 -0400 >> From: chris...@zoulas.com (Christos Zoulas) >> >> 1. Looks like FreeBSD (and my initial posting) leaves the file descriptor >>of the executable open in the process's image. The Linux man page says >>to set close-on-exec if you don't want it to be passed to the child >>process. Which behavior do you prefer? To have fexecve close the fd >>automatically or to leave it up to the caller? It seems less magical >>to leave it to the caller, but it also requires action from the caller. > > I don't see why fexec should behave differently from exec in the > decision of which fds stay open. We already have a mechanism to > request any other fd be closed on exec. > Am I reading it correctly that FreeBSD and Linux behave the same way? By default the fd is open, unless close-on-exec set? I would keep the same behavior and leave it to caller. >> 2. I am setting the path of the executable to "/" and p_comm to "*fexecve*". >>I could also do a reverse lookup and set them to the path of the binary, >>I found and default to "/" and "*fexecve*" if that's not found. I know >>people don't like those reverse lookups because of the vnode cache >>issues... > > Can we just cache these when the file descriptor is opened with > O_EXEC? > > The cache could become stale if the executable or any parent directory > is moved or deleted. But they could become stale after exec too for > the same reason. > O_EXEC should be tunable in runtime, with: fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_EXEC); It would be nice to get support for: F_GETPATH, for every file descriptor with associated file path. signature.asc Description: OpenPGP digital signature
Re: more fexecve questions
In article <20190910150418.becab60...@jupiter.mumble.net>, Taylor R Campbell wrote: >> Date: Tue, 10 Sep 2019 10:52:47 -0400 >> From: chris...@zoulas.com (Christos Zoulas) >> >> 1. Looks like FreeBSD (and my initial posting) leaves the file descriptor >>of the executable open in the process's image. The Linux man page says >>to set close-on-exec if you don't want it to be passed to the child >>process. Which behavior do you prefer? To have fexecve close the fd >>automatically or to leave it up to the caller? It seems less magical >>to leave it to the caller, but it also requires action from the caller. > >I don't see why fexec should behave differently from exec in the >decision of which fds stay open. We already have a mechanism to >request any other fd be closed on exec. Fine, >> 2. I am setting the path of the executable to "/" and p_comm to "*fexecve*". >>I could also do a reverse lookup and set them to the path of the binary, >>I found and default to "/" and "*fexecve*" if that's not found. I know >>people don't like those reverse lookups because of the vnode cache >>issues... > >Can we just cache these when the file descriptor is opened with >O_EXEC? > >The cache could become stale if the executable or any parent directory >is moved or deleted. But they could become stale after exec too for >the same reason. I guess we could, since the problem is that if the filename is greater than NCHNAMLEN, they will not be cached. But I am not familiar enough with the cache to make the changes required. Finally I should fix the code so that it works for scripts. It currently does not. christos
Re: more fexecve questions
> Which behavior do you prefer? To have fexecve close the fd > automatically or to leave it up to the caller? It seems less magical > to leave it to the caller, but it also requires action from the > caller. For designed-for-NetBSD code, callers already need changing to use fexecve at all, so I don't see that as a big deal. If I've read what you wrote right, that's what other implementations do, so designed-for-elsewhere code probably expects that. And, for what little it may be worth, my preference would be that fexecve act like execve in this respect. So, all of those point in the same direction: close it iff CLOEXEC is set on it. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
more fexecve questions
1. Looks like FreeBSD (and my initial posting) leaves the file descriptor of the executable open in the process's image. The Linux man page says to set close-on-exec if you don't want it to be passed to the child process. Which behavior do you prefer? To have fexecve close the fd automatically or to leave it up to the caller? It seems less magical to leave it to the caller, but it also requires action from the caller. 2. I am setting the path of the executable to "/" and p_comm to "*fexecve*". I could also do a reverse lookup and set them to the path of the binary, I found and default to "/" and "*fexecve*" if that's not found. I know people don't like those reverse lookups because of the vnode cache issues... 3. Are there any security requirements you want me to implement before I commit this? There were some concerns about chroot issues and fd passing, but my undestanding is that Taylor mentioned that those are already present. Best, christos
