Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
I think I'd prefer to avoid path canonicalization. If it proves not flexible enough, we can add it in later. Thanks, Ben. On Wed, Apr 13, 2016 at 12:08:58PM -0400, Aaron Conole wrote: > Hi Ben, > > I have rebased (due to conflicts) the series and am set to resubmit; > however I'd like to get closure on this issue before I do. > > I have no strong feelings either way - I can manually scan the string > for .. or use the realpath code I've put here. Since this only happens > at initialization time, I figured it didn't matter how heavy it was, > but that may not be the case (especially due to where initialization > occurs). > > Please advise what you would rather see - I can make the changes and test > with them. > > Thanks :-) > -Aaron > > Aaron Conole writes: > > > Hi Ben, > > > > Ben Pfaff writes: > > > >> (Yow, that's a lot of CCs.) > > > > Lots of cooks in the kitchen on this one. > > > >> On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: > >>> This commit adds a new function (ovs_realpath) to perform the role of > >>> realpath on various operating systems. The purpose is to ensure that a > >>> given path to file exists, and to return a completely resolved path (sans > >>> '.' and '..'). > >>> > >>> Signed-off-by: Aaron Conole > >> > >> Path canonicalization is a pretty big hammer. In other cases where OVS > >> relies on an absolute path, it uses textual comparison on a prefix of > >> the name (representing a directory) and rejects use of ".." following > >> the prefix. This is pretty easy to get right, and it is not as > >> heavyweight, and it does not have to actually do file system operations > >> (stat, readlink, ...), and its verdict can't change as a result of > >> changes to the file system (e.g. new or modified or deleted symlinks, > >> NFS servers that are down), and so on. > >> > >> Do you think we really need path canonicalization? > > > > I was nervous about a user putting escapes in the code. Unlike with, > > say, vhost user filenames (where we just blanket deny '/' because the > > semantic is of a file) this is not a file specification, but a directory > > specification. That implies that we would have to keep state and test > > for /../, and ../ (at the beginning of the string), at the least. > > > > If you think it's safe to merely test for the presence of these and > > bail, I'm okay with it, but I didn't want to leave any possibility that > > a malicious DB user could escape out of the rundir when changing the > > vhost-socket dir. > > > > I do agree it's heavy. > > > >> Thanks, > > > > Thanks for the review! > > > >> Ben. > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
Hi Ben, I have rebased (due to conflicts) the series and am set to resubmit; however I'd like to get closure on this issue before I do. I have no strong feelings either way - I can manually scan the string for .. or use the realpath code I've put here. Since this only happens at initialization time, I figured it didn't matter how heavy it was, but that may not be the case (especially due to where initialization occurs). Please advise what you would rather see - I can make the changes and test with them. Thanks :-) -Aaron Aaron Conole writes: > Hi Ben, > > Ben Pfaff writes: > >> (Yow, that's a lot of CCs.) > > Lots of cooks in the kitchen on this one. > >> On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: >>> This commit adds a new function (ovs_realpath) to perform the role of >>> realpath on various operating systems. The purpose is to ensure that a >>> given path to file exists, and to return a completely resolved path (sans >>> '.' and '..'). >>> >>> Signed-off-by: Aaron Conole >> >> Path canonicalization is a pretty big hammer. In other cases where OVS >> relies on an absolute path, it uses textual comparison on a prefix of >> the name (representing a directory) and rejects use of ".." following >> the prefix. This is pretty easy to get right, and it is not as >> heavyweight, and it does not have to actually do file system operations >> (stat, readlink, ...), and its verdict can't change as a result of >> changes to the file system (e.g. new or modified or deleted symlinks, >> NFS servers that are down), and so on. >> >> Do you think we really need path canonicalization? > > I was nervous about a user putting escapes in the code. Unlike with, > say, vhost user filenames (where we just blanket deny '/' because the > semantic is of a file) this is not a file specification, but a directory > specification. That implies that we would have to keep state and test > for /../, and ../ (at the beginning of the string), at the least. > > If you think it's safe to merely test for the presence of these and > bail, I'm okay with it, but I didn't want to leave any possibility that > a malicious DB user could escape out of the rundir when changing the > vhost-socket dir. > > I do agree it's heavy. > >> Thanks, > > Thanks for the review! > >> Ben. > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
> -Original Message- > From: Aaron Conole [mailto:acon...@redhat.com] > Sent: Tuesday, April 12, 2016 2:18 PM > To: Mooney, Sean K > Cc: Ben Pfaff ; dev@openvswitch.org; Flavio Leitner > ; Traynor, Kevin ; Panu > Matilainen ; Wojciechowicz, RobertX > ; Andy Zhou ; Daniele Di > Proietto ; Zoltan Kiss ; > Christian Ehrhardt > Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer > > "Mooney, Sean K" writes: > > >> -Original Message- > >> From: Aaron Conole [mailto:acon...@redhat.com] > >> Sent: Tuesday, April 12, 2016 1:12 AM > >> To: Ben Pfaff > >> Cc: dev@openvswitch.org; Flavio Leitner ; Traynor, > >> Kevin ; Panu Matilainen > >> ; Wojciechowicz, RobertX > >> ; Mooney, Sean K > >> ; Andy Zhou ; Daniele Di > >> Proietto ; Zoltan Kiss > >> ; Christian Ehrhardt > >> > >> Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer > >> > >> Hi Ben, > >> > >> Ben Pfaff writes: > >> > >> > (Yow, that's a lot of CCs.) > >> > >> Lots of cooks in the kitchen on this one. > >> > >> > On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: > >> >> This commit adds a new function (ovs_realpath) to perform the role > >> >> of realpath on various operating systems. The purpose is to ensure > >> >> that a given path to file exists, and to return a completely > >> >> resolved path (sans '.' and '..'). > >> >> > >> >> Signed-off-by: Aaron Conole > >> > > >> > Path canonicalization is a pretty big hammer. In other cases where > >> > OVS relies on an absolute path, it uses textual comparison on a > >> > prefix of the name (representing a directory) and rejects use of > ".." > >> > following the prefix. This is pretty easy to get right, and it is > >> > not as heavyweight, and it does not have to actually do file system > >> > operations (stat, readlink, ...), and its verdict can't change as a > >> > result of changes to the file system (e.g. new or modified or > >> > deleted symlinks, NFS servers that are down), and so on. > >> > > >> > Do you think we really need path canonicalization? > >> > >> I was nervous about a user putting escapes in the code. Unlike with, > >> say, vhost user filenames (where we just blanket deny '/' because the > >> semantic is of a file) this is not a file specification, but a > >> directory specification. That implies that we would have to keep > >> state and test for /../, and ../ (at the beginning of the string), at > the least. > >> > >> If you think it's safe to merely test for the presence of these and > >> bail, I'm okay with it, but I didn't want to leave any possibility > >> that a malicious DB user could escape out of the rundir when changing > >> the vhost-socket dir. > > [Mooney, Sean K] > > currently it is possible to use any dir for the vhost-socket dir if > > the absolute path is specified on the ovs-vswitchd command line > > correct? > > I know for a time we used /tmp/ for the sockets. I have also seen > > /dev/vhostuser/ used. > > The run dir (/var/run/openvswitch or /usr/var/run/openvswitch) makes > > the most sense To me as a default. > > > > More of a clarifying question but are you proposing restricting the > > localtions where the vhost-user sockets can be stored or constraining > > the path format to allow/deny relative path specifiers such as "." and > > ".." and converting that path in an canonical absolute path? > > > > I don't have a strong preference but just wanted to make sure I > > understand what is being proposed And what flexibility we would be > > gaining/losing. > > Sure; this would restrict all paths specified in the string to being > subpaths of the ovs_rundir(). So, your case of using /tmp or > /dev/vhostuser/ would not work. On the other hand, a path like > vhostuser/ would be treated as /var/run/openvswitch/vhostuser/. The code > is merely attempting to prevent arbitrary directory escapes by using the > realpath system call. > > I added this after > http://openvswitch.org/pipermail/dev/2016-March/068743.html [Mooney, Sean K] Ah ok that restriction makes sense in that context. We swapped to using the ovs run directory (e.g. /var/run/openvswitch/) when the vhost-user patches were merge to ovs so this will not affect the OpenStack integration with Neutron or odl so this seems ok to me. Thanks for clarifying. > > Thanks, > -Aaron > > >> > >> I do agree it's heavy. > >> > >> > Thanks, > >> > >> Thanks for the review! > >> > >> > Ben. > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
"Mooney, Sean K" writes: >> -Original Message- >> From: Aaron Conole [mailto:acon...@redhat.com] >> Sent: Tuesday, April 12, 2016 1:12 AM >> To: Ben Pfaff >> Cc: dev@openvswitch.org; Flavio Leitner ; Traynor, >> Kevin ; Panu Matilainen ; >> Wojciechowicz, RobertX ; Mooney, Sean K >> ; Andy Zhou ; Daniele Di >> Proietto ; Zoltan Kiss ; >> Christian Ehrhardt >> Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer >> >> Hi Ben, >> >> Ben Pfaff writes: >> >> > (Yow, that's a lot of CCs.) >> >> Lots of cooks in the kitchen on this one. >> >> > On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: >> >> This commit adds a new function (ovs_realpath) to perform the role of >> >> realpath on various operating systems. The purpose is to ensure that >> >> a given path to file exists, and to return a completely resolved path >> >> (sans '.' and '..'). >> >> >> >> Signed-off-by: Aaron Conole >> > >> > Path canonicalization is a pretty big hammer. In other cases where >> > OVS relies on an absolute path, it uses textual comparison on a prefix >> > of the name (representing a directory) and rejects use of ".." >> > following the prefix. This is pretty easy to get right, and it is not >> > as heavyweight, and it does not have to actually do file system >> > operations (stat, readlink, ...), and its verdict can't change as a >> > result of changes to the file system (e.g. new or modified or deleted >> > symlinks, NFS servers that are down), and so on. >> > >> > Do you think we really need path canonicalization? >> >> I was nervous about a user putting escapes in the code. Unlike with, >> say, vhost user filenames (where we just blanket deny '/' because the >> semantic is of a file) this is not a file specification, but a directory >> specification. That implies that we would have to keep state and test >> for /../, and ../ (at the beginning of the string), at the least. >> >> If you think it's safe to merely test for the presence of these and >> bail, I'm okay with it, but I didn't want to leave any possibility that >> a malicious DB user could escape out of the rundir when changing the >> vhost-socket dir. > [Mooney, Sean K] > currently it is possible to use any dir for the vhost-socket dir if > the absolute path is > specified on the ovs-vswitchd command line correct? > I know for a time we used /tmp/ for the sockets. I have also seen > /dev/vhostuser/ used. > The run dir (/var/run/openvswitch or /usr/var/run/openvswitch) makes > the most sense > To me as a default. > > More of a clarifying question but are you proposing restricting the localtions > where the vhost-user sockets can be stored or constraining the path > format to allow/deny > relative path specifiers such as "." and ".." and converting that path > in an canonical absolute path? > > I don't have a strong preference but just wanted to make sure I > understand what is being proposed > And what flexibility we would be gaining/losing. Sure; this would restrict all paths specified in the string to being subpaths of the ovs_rundir(). So, your case of using /tmp or /dev/vhostuser/ would not work. On the other hand, a path like vhostuser/ would be treated as /var/run/openvswitch/vhostuser/. The code is merely attempting to prevent arbitrary directory escapes by using the realpath system call. I added this after http://openvswitch.org/pipermail/dev/2016-March/068743.html Thanks, -Aaron >> >> I do agree it's heavy. >> >> > Thanks, >> >> Thanks for the review! >> >> > Ben. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
> -Original Message- > From: Aaron Conole [mailto:acon...@redhat.com] > Sent: Tuesday, April 12, 2016 1:12 AM > To: Ben Pfaff > Cc: dev@openvswitch.org; Flavio Leitner ; Traynor, > Kevin ; Panu Matilainen ; > Wojciechowicz, RobertX ; Mooney, Sean K > ; Andy Zhou ; Daniele Di > Proietto ; Zoltan Kiss ; > Christian Ehrhardt > Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer > > Hi Ben, > > Ben Pfaff writes: > > > (Yow, that's a lot of CCs.) > > Lots of cooks in the kitchen on this one. > > > On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: > >> This commit adds a new function (ovs_realpath) to perform the role of > >> realpath on various operating systems. The purpose is to ensure that > >> a given path to file exists, and to return a completely resolved path > >> (sans '.' and '..'). > >> > >> Signed-off-by: Aaron Conole > > > > Path canonicalization is a pretty big hammer. In other cases where > > OVS relies on an absolute path, it uses textual comparison on a prefix > > of the name (representing a directory) and rejects use of ".." > > following the prefix. This is pretty easy to get right, and it is not > > as heavyweight, and it does not have to actually do file system > > operations (stat, readlink, ...), and its verdict can't change as a > > result of changes to the file system (e.g. new or modified or deleted > > symlinks, NFS servers that are down), and so on. > > > > Do you think we really need path canonicalization? > > I was nervous about a user putting escapes in the code. Unlike with, > say, vhost user filenames (where we just blanket deny '/' because the > semantic is of a file) this is not a file specification, but a directory > specification. That implies that we would have to keep state and test > for /../, and ../ (at the beginning of the string), at the least. > > If you think it's safe to merely test for the presence of these and > bail, I'm okay with it, but I didn't want to leave any possibility that > a malicious DB user could escape out of the rundir when changing the > vhost-socket dir. [Mooney, Sean K] currently it is possible to use any dir for the vhost-socket dir if the absolute path is specified on the ovs-vswitchd command line correct? I know for a time we used /tmp/ for the sockets. I have also seen /dev/vhostuser/ used. The run dir (/var/run/openvswitch or /usr/var/run/openvswitch) makes the most sense To me as a default. More of a clarifying question but are you proposing restricting the localtions where the vhost-user sockets can be stored or constraining the path format to allow/deny relative path specifiers such as "." and ".." and converting that path in an canonical absolute path? I don't have a strong preference but just wanted to make sure I understand what is being proposed And what flexibility we would be gaining/losing. > > I do agree it's heavy. > > > Thanks, > > Thanks for the review! > > > Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
Hi Ben, Ben Pfaff writes: > (Yow, that's a lot of CCs.) Lots of cooks in the kitchen on this one. > On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: >> This commit adds a new function (ovs_realpath) to perform the role of >> realpath on various operating systems. The purpose is to ensure that a >> given path to file exists, and to return a completely resolved path (sans >> '.' and '..'). >> >> Signed-off-by: Aaron Conole > > Path canonicalization is a pretty big hammer. In other cases where OVS > relies on an absolute path, it uses textual comparison on a prefix of > the name (representing a directory) and rejects use of ".." following > the prefix. This is pretty easy to get right, and it is not as > heavyweight, and it does not have to actually do file system operations > (stat, readlink, ...), and its verdict can't change as a result of > changes to the file system (e.g. new or modified or deleted symlinks, > NFS servers that are down), and so on. > > Do you think we really need path canonicalization? I was nervous about a user putting escapes in the code. Unlike with, say, vhost user filenames (where we just blanket deny '/' because the semantic is of a file) this is not a file specification, but a directory specification. That implies that we would have to keep state and test for /../, and ../ (at the beginning of the string), at the least. If you think it's safe to merely test for the presence of these and bail, I'm okay with it, but I didn't want to leave any possibility that a malicious DB user could escape out of the rundir when changing the vhost-socket dir. I do agree it's heavy. > Thanks, Thanks for the review! > Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
(Yow, that's a lot of CCs.) On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: > This commit adds a new function (ovs_realpath) to perform the role of > realpath on various operating systems. The purpose is to ensure that a > given path to file exists, and to return a completely resolved path (sans > '.' and '..'). > > Signed-off-by: Aaron Conole Path canonicalization is a pretty big hammer. In other cases where OVS relies on an absolute path, it uses textual comparison on a prefix of the name (representing a directory) and rejects use of ".." following the prefix. This is pretty easy to get right, and it is not as heavyweight, and it does not have to actually do file system operations (stat, readlink, ...), and its verdict can't change as a result of changes to the file system (e.g. new or modified or deleted symlinks, NFS servers that are down), and so on. Do you think we really need path canonicalization? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer
This commit adds a new function (ovs_realpath) to perform the role of
realpath on various operating systems. The purpose is to ensure that a
given path to file exists, and to return a completely resolved path (sans
'.' and '..').
Signed-off-by: Aaron Conole
---
v11:
* Added
configure.ac | 2 +-
lib/util.c| 52
lib/util.h| 1 +
tests/library.at | 5 +
tests/test-util.c | 23 +++
5 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 05d80d5..a490260 100644
--- a/configure.ac
+++ b/configure.ac
@@ -105,7 +105,7 @@ AC_CHECK_DECLS([sys_siglist], [], [], [[#include
]])
AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct stat.st_mtimensec],
[], [], [[#include ]])
AC_CHECK_MEMBERS([struct ifreq.ifr_flagshigh], [], [], [[#include ]])
-AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r])
+AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r realpath])
AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h
stdatomic.h])
AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include
#include ]])
diff --git a/lib/util.c b/lib/util.c
index 94311ac..6bb6d9e 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -2098,6 +2098,58 @@ is_stdout_a_tty(void)
return (isatty(STDOUT_FILENO) && t && strcmp(t, "dumb") != 0);
}
+/* Derives, from the pathname pointed to by path, an absolute pathname that
+ * names the same file, whose resolution does not involve '.', '..' or
+ * symbolic links. If path is null or does not exist, returns NULL.
+ */
+char *
+ovs_realpath(const char *path)
+{
+if (!path) {
+return NULL;
+}
+char *realpath_res = NULL;
+
+#ifdef HAVE_REALPATH
+realpath_res = realpath(path, NULL);
+if (realpath_res == NULL) {
+goto error;
+}
+struct stat s;
+int err = stat(realpath_res, &s);
+if (err) {
+goto error;
+}
+return realpath_res;
+#elif defined(_WIN32)
+realpath_res = xstrdup(path);
+DWORD ret = GetFullPathName(path, strlen(realpath_res), realpath_res,
+NULL);
+if (ret == 0) {
+goto error;
+} else if (ret > strlen(realpath_res)) {
+DWORD new_ret;
+char *new_path = (char *)xrealloc(realpath_res, ret);
+if (new_path == NULL) {
+goto error;
+}
+realpath_res = new_path;
+new_ret = GetFullPathName(path, strlen(realpath_res), realpath_res,
+ NULL);
+if (new_ret == 0 || new_ret != ret ) {
+goto error;
+}
+}
+return realpath_res;
+#else
+#error Need realpath() on this platform
+#endif
+
+error:
+free(realpath_res);
+return NULL;
+}
+
#ifdef _WIN32
char *
diff --git a/lib/util.h b/lib/util.h
index 41c5ea6..8d7f335 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -220,6 +220,7 @@ char *dir_name(const char *file_name);
char *base_name(const char *file_name);
#endif
char *abs_file_name(const char *dir, const char *file_name);
+char *ovs_realpath(const char *path);
char *follow_symlinks(const char *filename);
diff --git a/tests/library.at b/tests/library.at
index e1bac92..b6561e5 100644
--- a/tests/library.at
+++ b/tests/library.at
@@ -234,3 +234,8 @@ AT_CLEANUP
AT_SETUP([test rcu])
AT_CHECK([ovstest test-rcu-quiesce], [0], [])
AT_CLEANUP
+
+AT_SETUP([test ovs_realpath])
+AT_CHECK([ovstest test-util realpath \
+/tmp/../tmp/../usr/bin/.//../share:/usr/share /tmpNOEXISTS:], [0], [])
+AT_CLEANUP
diff --git a/tests/test-util.c b/tests/test-util.c
index ef45903..2755200 100644
--- a/tests/test-util.c
+++ b/tests/test-util.c
@@ -1128,6 +1128,28 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
ovs_assert(snprintf(NULL, 0, "abcde") == 5);
}
+static void
+test_ovs_realpath(struct ovs_cmdl_context *ctx)
+{
+int i;
+for (i = 1; i < ctx->argc; i++) {
+char *str_toks = NULL;
+char *path = strtok_r(ctx->argv[i], ":", &str_toks);
+ovs_assert(path != NULL);
+
+char *check = strtok_r(NULL, ":", &str_toks);
+char *rpath = ovs_realpath(path);
+
+if (check) {
+ovs_assert(rpath != NULL);
+ovs_assert(!strcmp(check, rpath));
+free(rpath);
+} else {
+ovs_assert(rpath == NULL);
+}
+}
+}
+
#ifndef _WIN32
static void
test_file_name(struct ovs_cmdl_context *ctx)
@@ -1164,6 +1186,7 @@ static const struct ovs_cmdl_command commands[] = {
{"assert", NULL, 0, 0, test_assert},
{"ovs_scan", NULL, 0, 0, test_ovs_scan},
{"snprintf", NULL, 0, 0, test_snprintf},
+{"realpath", NULL, 1, INT_MAX, test_ovs_realpath},
#ifndef _WIN32
{"file_name", NULL, 1, INT_MAX, test_file_name},
#endif
--
2.5.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
