* [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities @ 2016-10-18 16:02 G. Campana 2016-10-20 18:13 ` Andre Przywara 2016-11-08 1:48 ` Will Deacon 0 siblings, 2 replies; 5+ messages in thread From: G. Campana @ 2016-10-18 16:02 UTC (permalink / raw) To: Will.Deacon; +Cc: kvm, andre.przywara, G. Campana --- virtio/9p.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/virtio/9p.c b/virtio/9p.c index 49e7c5c..c3edc20 100644 --- a/virtio/9p.c +++ b/virtio/9p.c @@ -222,6 +222,21 @@ static bool is_dir(struct p9_fid *fid) return S_ISDIR(st.st_mode); } +/* path is always absolute */ +static bool path_is_illegal(const char *path) +{ + size_t len; + + if (strstr(path, "/../") != NULL) + return true; + + len = strlen(path); + if (len >= 3 && strcmp(path + len - 3, "/..") == 0) + return true; + + return false; +} + static void virtio_p9_open(struct p9_dev *p9dev, struct p9_pdu *pdu, u32 *outlen) { @@ -278,6 +293,11 @@ static void virtio_p9_create(struct p9_dev *p9dev, flags = virtio_p9_openflags(flags); sprintf(full_path, "%s/%s", dfid->abs_path, name); + if (path_is_illegal(full_path)) { + errno = EACCES; + goto err_out; + } + fd = open(full_path, flags | O_CREAT, mode); if (fd < 0) goto err_out; @@ -319,6 +339,11 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev, dfid = get_fid(p9dev, dfid_val); sprintf(full_path, "%s/%s", dfid->abs_path, name); + if (path_is_illegal(full_path)) { + errno = EACCES; + goto err_out; + } + ret = mkdir(full_path, mode); if (ret < 0) goto err_out; @@ -776,6 +801,11 @@ static void virtio_p9_rename(struct p9_dev *p9dev, new_fid = get_fid(p9dev, new_fid_val); sprintf(full_path, "%s/%s", new_fid->abs_path, new_name); + if (path_is_illegal(full_path)) { + errno = EACCES; + goto err_out; + } + ret = rename(fid->abs_path, full_path); if (ret < 0) goto err_out; @@ -860,6 +890,11 @@ static void virtio_p9_mknod(struct p9_dev *p9dev, dfid = get_fid(p9dev, fid_val); sprintf(full_path, "%s/%s", dfid->abs_path, name); + if (path_is_illegal(full_path)) { + errno = EACCES; + goto err_out; + } + ret = mknod(full_path, mode, makedev(major, minor)); if (ret < 0) goto err_out; @@ -927,6 +962,11 @@ static void virtio_p9_symlink(struct p9_dev *p9dev, dfid = get_fid(p9dev, fid_val); sprintf(new_name, "%s/%s", dfid->abs_path, name); + if (path_is_illegal(new_name)) { + errno = EACCES; + goto err_out; + } + ret = symlink(old_path, new_name); if (ret < 0) goto err_out; @@ -962,6 +1002,11 @@ static void virtio_p9_link(struct p9_dev *p9dev, dfid = get_fid(p9dev, dfid_val); fid = get_fid(p9dev, fid_val); sprintf(full_path, "%s/%s", dfid->abs_path, name); + if (path_is_illegal(full_path)) { + errno = EACCES; + goto err_out; + } + ret = link(fid->abs_path, full_path); if (ret < 0) goto err_out; @@ -1079,6 +1124,11 @@ static void virtio_p9_renameat(struct p9_dev *p9dev, sprintf(old_full_path, "%s/%s", old_dfid->abs_path, old_name); sprintf(new_full_path, "%s/%s", new_dfid->abs_path, new_name); + if (path_is_illegal(old_full_path) || path_is_illegal(new_full_path)) { + errno = EACCES; + goto err_out; + } + ret = rename(old_full_path, new_full_path); if (ret < 0) goto err_out; @@ -1112,6 +1162,11 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev, fid = get_fid(p9dev, fid_val); sprintf(full_path, "%s/%s", fid->abs_path, name); + if (path_is_illegal(full_path)) { + errno = EACCES; + goto err_out; + } + ret = remove(full_path); if (ret < 0) goto err_out; -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities 2016-10-18 16:02 [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities G. Campana @ 2016-10-20 18:13 ` Andre Przywara 2016-10-25 11:34 ` G. Campana 2016-11-08 1:48 ` Will Deacon 1 sibling, 1 reply; 5+ messages in thread From: Andre Przywara @ 2016-10-20 18:13 UTC (permalink / raw) To: G. Campana, Will.Deacon; +Cc: kvm Hi, thanks for the patches! Please add a commit message (applies to the other patches as well). Also we will need your Signed-off-by: for every patch that you want to see committed. On 18/10/16 17:02, G. Campana wrote: > --- > virtio/9p.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/virtio/9p.c b/virtio/9p.c > index 49e7c5c..c3edc20 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -222,6 +222,21 @@ static bool is_dir(struct p9_fid *fid) > return S_ISDIR(st.st_mode); > } > > +/* path is always absolute */ > +static bool path_is_illegal(const char *path) > +{ > + size_t len; > + > + if (strstr(path, "/../") != NULL) > + return true; > + > + len = strlen(path); > + if (len >= 3 && strcmp(path + len - 3, "/..") == 0) > + return true; > + > + return false; > +} > + I was wondering if this is good enough, as it only looks for this specific sequence (for instance omitting "../")? I couldn't quickly find a counterexample (Unicode? Quoting?), but it feels like there are potentially dangerous cases we miss. Also the naming maybe a bit misleading (as having a dot-dot-slash sequence isn't per se illegal). So I was wondering if we could make use of realpath(3) here? That would also detect cases where we have symlinks pointing outside of the root directory (does that matter here?). But I am not sure we want to pay the overhead of normalizing the path each time, since this one will check the path components against the filesystem. Opinions? Cheers, Andre. > static void virtio_p9_open(struct p9_dev *p9dev, > struct p9_pdu *pdu, u32 *outlen) > { > @@ -278,6 +293,11 @@ static void virtio_p9_create(struct p9_dev *p9dev, > flags = virtio_p9_openflags(flags); > > sprintf(full_path, "%s/%s", dfid->abs_path, name); > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > fd = open(full_path, flags | O_CREAT, mode); > if (fd < 0) > goto err_out; > @@ -319,6 +339,11 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev, > dfid = get_fid(p9dev, dfid_val); > > sprintf(full_path, "%s/%s", dfid->abs_path, name); > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = mkdir(full_path, mode); > if (ret < 0) > goto err_out; > @@ -776,6 +801,11 @@ static void virtio_p9_rename(struct p9_dev *p9dev, > new_fid = get_fid(p9dev, new_fid_val); > > sprintf(full_path, "%s/%s", new_fid->abs_path, new_name); > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = rename(fid->abs_path, full_path); > if (ret < 0) > goto err_out; > @@ -860,6 +890,11 @@ static void virtio_p9_mknod(struct p9_dev *p9dev, > > dfid = get_fid(p9dev, fid_val); > sprintf(full_path, "%s/%s", dfid->abs_path, name); > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = mknod(full_path, mode, makedev(major, minor)); > if (ret < 0) > goto err_out; > @@ -927,6 +962,11 @@ static void virtio_p9_symlink(struct p9_dev *p9dev, > > dfid = get_fid(p9dev, fid_val); > sprintf(new_name, "%s/%s", dfid->abs_path, name); > + if (path_is_illegal(new_name)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = symlink(old_path, new_name); > if (ret < 0) > goto err_out; > @@ -962,6 +1002,11 @@ static void virtio_p9_link(struct p9_dev *p9dev, > dfid = get_fid(p9dev, dfid_val); > fid = get_fid(p9dev, fid_val); > sprintf(full_path, "%s/%s", dfid->abs_path, name); > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = link(fid->abs_path, full_path); > if (ret < 0) > goto err_out; > @@ -1079,6 +1124,11 @@ static void virtio_p9_renameat(struct p9_dev *p9dev, > > sprintf(old_full_path, "%s/%s", old_dfid->abs_path, old_name); > sprintf(new_full_path, "%s/%s", new_dfid->abs_path, new_name); > + if (path_is_illegal(old_full_path) || path_is_illegal(new_full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = rename(old_full_path, new_full_path); > if (ret < 0) > goto err_out; > @@ -1112,6 +1162,11 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev, > fid = get_fid(p9dev, fid_val); > > sprintf(full_path, "%s/%s", fid->abs_path, name); > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + goto err_out; > + } > + > ret = remove(full_path); > if (ret < 0) > goto err_out; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities 2016-10-20 18:13 ` Andre Przywara @ 2016-10-25 11:34 ` G. Campana 0 siblings, 0 replies; 5+ messages in thread From: G. Campana @ 2016-10-25 11:34 UTC (permalink / raw) To: Andre Przywara, G. Campana, Will.Deacon; +Cc: kvm Hi, On 10/20/2016 08:13 PM, Andre Przywara wrote: > Hi, > > thanks for the patches! > > Please add a commit message (applies to the other patches as well). Also > we will need your Signed-off-by: for every patch that you want to see > committed. Thanks for the review. I'll send a new version of the patch series (signed-off and with explicit commit messages) once we agree on this one :) > > On 18/10/16 17:02, G. Campana wrote: >> --- >> virtio/9p.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/virtio/9p.c b/virtio/9p.c >> index 49e7c5c..c3edc20 100644 >> --- a/virtio/9p.c >> +++ b/virtio/9p.c >> @@ -222,6 +222,21 @@ static bool is_dir(struct p9_fid *fid) >> return S_ISDIR(st.st_mode); >> } >> >> +/* path is always absolute */ >> +static bool path_is_illegal(const char *path) >> +{ >> + size_t len; >> + >> + if (strstr(path, "/../") != NULL) >> + return true; >> + >> + len = strlen(path); >> + if (len >= 3 && strcmp(path + len - 3, "/..") == 0) >> + return true; >> + >> + return false; >> +} >> + > > I was wondering if this is good enough, as it only looks for this > specific sequence (for instance omitting "../")? I couldn't quickly find > a counterexample (Unicode? Quoting?), but it feels like there are > potentially dangerous cases we miss. The only path component that can lead to path traversal is "..". The strings are directly passed to syscalls and the Linux kernel handles ASCII strings, so there are no decoding issues. As far as I know, this check is good enough. path_is_illegal() checks that the path doesn't end with "/.." and that there are no occurrences of "/../". Since path is absolute (and thus starts with "/"), searching for the "../" pattern isn't required. > Also the naming maybe a bit misleading (as having a dot-dot-slash > sequence isn't per se illegal). > Do you prefer path_is_secure? > So I was wondering if we could make use of realpath(3) here? That would > also detect cases where we have symlinks pointing outside of the root > directory (does that matter here?). But I am not sure we want to pay the > overhead of normalizing the path each time, since this one will check > the path components against the filesystem. > > Opinions? realpath does path resolution and can return an error (eg: ENOENT), while we only want to ensure that there is no possibility of path traversal. Besides, realpath can't be used with creat/link/symlink because in that case the file doesn't exist yet. Regarding symlinks, we can't rely on realpath to ensure that they don't point outside of the root directory: an attacker can modify a component of the path after its resolution by realpath. Thus, the path resolution must be done later by the final filesystem functions (readdir, open, etc.) Cheers > > Cheers, > Andre. > >> static void virtio_p9_open(struct p9_dev *p9dev, >> struct p9_pdu *pdu, u32 *outlen) >> { >> @@ -278,6 +293,11 @@ static void virtio_p9_create(struct p9_dev *p9dev, >> flags = virtio_p9_openflags(flags); >> >> sprintf(full_path, "%s/%s", dfid->abs_path, name); >> + if (path_is_illegal(full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> fd = open(full_path, flags | O_CREAT, mode); >> if (fd < 0) >> goto err_out; >> @@ -319,6 +339,11 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev, >> dfid = get_fid(p9dev, dfid_val); >> >> sprintf(full_path, "%s/%s", dfid->abs_path, name); >> + if (path_is_illegal(full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = mkdir(full_path, mode); >> if (ret < 0) >> goto err_out; >> @@ -776,6 +801,11 @@ static void virtio_p9_rename(struct p9_dev *p9dev, >> new_fid = get_fid(p9dev, new_fid_val); >> >> sprintf(full_path, "%s/%s", new_fid->abs_path, new_name); >> + if (path_is_illegal(full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = rename(fid->abs_path, full_path); >> if (ret < 0) >> goto err_out; >> @@ -860,6 +890,11 @@ static void virtio_p9_mknod(struct p9_dev *p9dev, >> >> dfid = get_fid(p9dev, fid_val); >> sprintf(full_path, "%s/%s", dfid->abs_path, name); >> + if (path_is_illegal(full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = mknod(full_path, mode, makedev(major, minor)); >> if (ret < 0) >> goto err_out; >> @@ -927,6 +962,11 @@ static void virtio_p9_symlink(struct p9_dev *p9dev, >> >> dfid = get_fid(p9dev, fid_val); >> sprintf(new_name, "%s/%s", dfid->abs_path, name); >> + if (path_is_illegal(new_name)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = symlink(old_path, new_name); >> if (ret < 0) >> goto err_out; >> @@ -962,6 +1002,11 @@ static void virtio_p9_link(struct p9_dev *p9dev, >> dfid = get_fid(p9dev, dfid_val); >> fid = get_fid(p9dev, fid_val); >> sprintf(full_path, "%s/%s", dfid->abs_path, name); >> + if (path_is_illegal(full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = link(fid->abs_path, full_path); >> if (ret < 0) >> goto err_out; >> @@ -1079,6 +1124,11 @@ static void virtio_p9_renameat(struct p9_dev *p9dev, >> >> sprintf(old_full_path, "%s/%s", old_dfid->abs_path, old_name); >> sprintf(new_full_path, "%s/%s", new_dfid->abs_path, new_name); >> + if (path_is_illegal(old_full_path) || path_is_illegal(new_full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = rename(old_full_path, new_full_path); >> if (ret < 0) >> goto err_out; >> @@ -1112,6 +1162,11 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev, >> fid = get_fid(p9dev, fid_val); >> >> sprintf(full_path, "%s/%s", fid->abs_path, name); >> + if (path_is_illegal(full_path)) { >> + errno = EACCES; >> + goto err_out; >> + } >> + >> ret = remove(full_path); >> if (ret < 0) >> goto err_out; >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities 2016-10-18 16:02 [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities G. Campana 2016-10-20 18:13 ` Andre Przywara @ 2016-11-08 1:48 ` Will Deacon 2016-11-10 15:13 ` G. Campana 1 sibling, 1 reply; 5+ messages in thread From: Will Deacon @ 2016-11-08 1:48 UTC (permalink / raw) To: G. Campana; +Cc: kvm, andre.przywara On Tue, Oct 18, 2016 at 06:02:38PM +0200, G. Campana wrote: > --- > virtio/9p.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/virtio/9p.c b/virtio/9p.c > index 49e7c5c..c3edc20 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -222,6 +222,21 @@ static bool is_dir(struct p9_fid *fid) > return S_ISDIR(st.st_mode); > } > > +/* path is always absolute */ > +static bool path_is_illegal(const char *path) > +{ > + size_t len; > + > + if (strstr(path, "/../") != NULL) > + return true; > + > + len = strlen(path); > + if (len >= 3 && strcmp(path + len - 3, "/..") == 0) > + return true; Why not just look for ".." and ignore the slashes altogether? Then you wouldn't need to treat the end of the string specially, either. Will ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities 2016-11-08 1:48 ` Will Deacon @ 2016-11-10 15:13 ` G. Campana 0 siblings, 0 replies; 5+ messages in thread From: G. Campana @ 2016-11-10 15:13 UTC (permalink / raw) To: Will Deacon; +Cc: kvm, andre.przywara On 08/11/2016 02:48, Will Deacon wrote: > On Tue, Oct 18, 2016 at 06:02:38PM +0200, G. Campana wrote: >> --- >> virtio/9p.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/virtio/9p.c b/virtio/9p.c >> index 49e7c5c..c3edc20 100644 >> --- a/virtio/9p.c >> +++ b/virtio/9p.c >> @@ -222,6 +222,21 @@ static bool is_dir(struct p9_fid *fid) >> return S_ISDIR(st.st_mode); >> } >> >> +/* path is always absolute */ >> +static bool path_is_illegal(const char *path) >> +{ >> + size_t len; >> + >> + if (strstr(path, "/../") != NULL) >> + return true; >> + >> + len = strlen(path); >> + if (len >= 3 && strcmp(path + len - 3, "/..") == 0) >> + return true; > > Why not just look for ".." and ignore the slashes altogether? Then you > wouldn't need to treat the end of the string specially, either. > Because filenames and dirnames can contain the string "..". For instance, "foo..bar" is a valid filename. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-10 15:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-18 16:02 [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities G. Campana 2016-10-20 18:13 ` Andre Przywara 2016-10-25 11:34 ` G. Campana 2016-11-08 1:48 ` Will Deacon 2016-11-10 15:13 ` G. Campana
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).