* [Qemu-trivial] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow
@ 2015-03-14 2:00 ` Shannon Zhao
0 siblings, 0 replies; 10+ messages in thread
From: Shannon Zhao @ 2015-03-14 2:00 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, peter.huangpeng,
aneesh.kumar, pbonzini, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow @ 2015-03-14 2:00 ` Shannon Zhao 0 siblings, 0 replies; 10+ messages in thread From: Shannon Zhao @ 2015-03-14 2:00 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, peter.huangpeng, aneesh.kumar, pbonzini, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-trivial] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow 2015-03-14 2:00 ` [Qemu-devel] " Shannon Zhao @ 2015-03-16 6:31 ` Aneesh Kumar K.V -1 siblings, 0 replies; 10+ messages in thread From: Aneesh Kumar K.V @ 2015-03-16 6:31 UTC (permalink / raw) To: Shannon Zhao, qemu-devel Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, peter.huangpeng, shannon.zhao, pbonzini 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow @ 2015-03-16 6:31 ` Aneesh Kumar K.V 0 siblings, 0 replies; 10+ messages in thread From: Aneesh Kumar K.V @ 2015-03-16 6:31 UTC (permalink / raw) To: Shannon Zhao, qemu-devel Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, peter.huangpeng, shannon.zhao, pbonzini 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow 2015-03-14 2:00 ` [Qemu-devel] " Shannon Zhao @ 2015-03-30 13:29 ` Stefan Hajnoczi -1 siblings, 0 replies; 10+ messages in thread From: Stefan Hajnoczi @ 2015-03-30 13:29 UTC (permalink / raw) To: Shannon Zhao Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, qemu-devel, peter.huangpeng, aneesh.kumar, pbonzini, shannon.zhao [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow @ 2015-03-30 13:29 ` Stefan Hajnoczi 0 siblings, 0 replies; 10+ messages in thread From: Stefan Hajnoczi @ 2015-03-30 13:29 UTC (permalink / raw) To: Shannon Zhao Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, qemu-devel, peter.huangpeng, aneesh.kumar, pbonzini, shannon.zhao [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow 2015-03-30 13:29 ` Stefan Hajnoczi @ 2015-03-30 14:03 ` Aneesh Kumar K.V -1 siblings, 0 replies; 10+ messages in thread From: Aneesh Kumar K.V @ 2015-03-30 14:03 UTC (permalink / raw) To: Stefan Hajnoczi, Shannon Zhao Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, qemu-devel, peter.huangpeng, shannon.zhao, pbonzini 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow @ 2015-03-30 14:03 ` Aneesh Kumar K.V 0 siblings, 0 replies; 10+ messages in thread From: Aneesh Kumar K.V @ 2015-03-30 14:03 UTC (permalink / raw) To: Stefan Hajnoczi, Shannon Zhao Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, qemu-devel, peter.huangpeng, shannon.zhao, pbonzini 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow 2015-03-30 14:03 ` Aneesh Kumar K.V @ 2015-03-31 14:07 ` Stefan Hajnoczi -1 siblings, 0 replies; 10+ messages in thread From: Stefan Hajnoczi @ 2015-03-31 14:07 UTC (permalink / raw) To: Aneesh Kumar K.V Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, qemu-devel, peter.huangpeng, shannon.zhao, Shannon Zhao, pbonzini [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow @ 2015-03-31 14:07 ` Stefan Hajnoczi 0 siblings, 0 replies; 10+ messages in thread From: Stefan Hajnoczi @ 2015-03-31 14:07 UTC (permalink / raw) To: Aneesh Kumar K.V Cc: peter.maydell, hangaohuai, qemu-trivial, mjt, qemu-devel, peter.huangpeng, shannon.zhao, Shannon Zhao, pbonzini [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-03-31 14:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-14 2:00 [Qemu-trivial] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow Shannon Zhao 2015-03-14 2:00 ` [Qemu-devel] " Shannon Zhao 2015-03-16 6:31 ` [Qemu-trivial] " Aneesh Kumar K.V 2015-03-16 6:31 ` [Qemu-devel] " Aneesh Kumar K.V 2015-03-30 13:29 ` [Qemu-trivial] " Stefan Hajnoczi 2015-03-30 13:29 ` Stefan Hajnoczi 2015-03-30 14:03 ` [Qemu-trivial] " Aneesh Kumar K.V 2015-03-30 14:03 ` Aneesh Kumar K.V 2015-03-31 14:07 ` [Qemu-trivial] " Stefan Hajnoczi 2015-03-31 14:07 ` Stefan Hajnoczi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.