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;
>>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
