[Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Shannon Zhao]

It's detected by coverity. As max of sockaddr_un.sun_path is
sizeof(helper.sun_path), should check the length of source
and use strncpy instead of strcpy.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
    v1->v2: Still use strcpy [Paolo]
---
 fsdev/virtfs-proxy-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index bf2e5f3..13fe032 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t gid)
         return -1;
     }
 
+    g_assert(strlen(path) < sizeof(proxy.sun_path));
     sock = socket(AF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
         do_perror("socket");
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Aneesh Kumar K.V]

Shannon Zhao <zhaoshenglong@huawei.com> writes:

> It's detected by coverity. As max of sockaddr_un.sun_path is
> sizeof(helper.sun_path), should check the length of source
> and use strncpy instead of strcpy.

updated such that,

The socket name specified should fit in the sockadd_un.sun_path. If not
abort.

with that applied.

>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>     v1->v2: Still use strcpy [Paolo]
> ---
>  fsdev/virtfs-proxy-helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index bf2e5f3..13fe032 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t gid)
>          return -1;
>      }
>  
> +    g_assert(strlen(path) < sizeof(proxy.sun_path));
>      sock = socket(AF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
>          do_perror("socket");
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Sat, Mar 14, 2015 at 10:00:16AM +0800, Shannon Zhao wrote:
> It's detected by coverity. As max of sockaddr_un.sun_path is
> sizeof(helper.sun_path), should check the length of source
> and use strncpy instead of strcpy.
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>     v1->v2: Still use strcpy [Paolo]
> ---
>  fsdev/virtfs-proxy-helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index bf2e5f3..13fe032 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t gid)
>          return -1;
>      }
>  
> +    g_assert(strlen(path) < sizeof(proxy.sun_path));
>      sock = socket(AF_UNIX, SOCK_STREAM, 0);

path is user input.  While the assertion check silences Coverity, it is
not suitable for input validation.  Users expect a graceful exit with an
error message, not an assertion failure if the given path is too long.

I will send a patch.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Aneesh Kumar K.V]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Sat, Mar 14, 2015 at 10:00:16AM +0800, Shannon Zhao wrote:
>> It's detected by coverity. As max of sockaddr_un.sun_path is
>> sizeof(helper.sun_path), should check the length of source
>> and use strncpy instead of strcpy.
>> 
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>     v1->v2: Still use strcpy [Paolo]
>> ---
>>  fsdev/virtfs-proxy-helper.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>> index bf2e5f3..13fe032 100644
>> --- a/fsdev/virtfs-proxy-helper.c
>> +++ b/fsdev/virtfs-proxy-helper.c
>> @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t gid)
>>          return -1;
>>      }
>>  
>> +    g_assert(strlen(path) < sizeof(proxy.sun_path));
>>      sock = socket(AF_UNIX, SOCK_STREAM, 0);
>
> path is user input.  While the assertion check silences Coverity, it is
> not suitable for input validation.  Users expect a graceful exit with an
> error message, not an assertion failure if the given path is too long.
>
> I will send a patch.

That is the proxy helper. The assert will cause an exit() which is good,
isn't it ? I did update the qemu side of the patch to do a graceful exit

-aneesh

Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On Mon, Mar 30, 2015 at 07:33:33PM +0530, Aneesh Kumar K.V wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Sat, Mar 14, 2015 at 10:00:16AM +0800, Shannon Zhao wrote:
> >> It's detected by coverity. As max of sockaddr_un.sun_path is
> >> sizeof(helper.sun_path), should check the length of source
> >> and use strncpy instead of strcpy.
> >> 
> >> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >>     v1->v2: Still use strcpy [Paolo]
> >> ---
> >>  fsdev/virtfs-proxy-helper.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> >> index bf2e5f3..13fe032 100644
> >> --- a/fsdev/virtfs-proxy-helper.c
> >> +++ b/fsdev/virtfs-proxy-helper.c
> >> @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t gid)
> >>          return -1;
> >>      }
> >>  
> >> +    g_assert(strlen(path) < sizeof(proxy.sun_path));
> >>      sock = socket(AF_UNIX, SOCK_STREAM, 0);
> >
> > path is user input.  While the assertion check silences Coverity, it is
> > not suitable for input validation.  Users expect a graceful exit with an
> > error message, not an assertion failure if the given path is too long.
> >
> > I will send a patch.
> 
> That is the proxy helper. The assert will cause an exit() which is good,
> isn't it ? I did update the qemu side of the patch to do a graceful exit

assert(3) calls abort(3), which sends SIGABRT.  The signal causes a core
dump by default.  That's not a proper way to exit on invalid input.

Plus, if the code is ever compiled with the NDEBUG macro set, the assert
would be removed and the check bypassed.

This is why assert() isn't suitable for input validation.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]