[PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities

[PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities

[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

Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities

[Andre Przywara]

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;
> 

Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities

[G. Campana]

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;
>>

Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities

[Will Deacon]

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

Re: [PATCH 1/7] kvmtool: 9p: fix path traversal vulnerabilities

[G. Campana]

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.