Re: [systemd-devel] [PATCH] libudev-monitor: ensure proper string termination
On 01/27/15 00:19, Lennart Poettering wrote: > On Sun, 25.01.15 07:10, Topi Miettinen (toiwo...@gmail.com) wrote: > >> On 01/25/15 03:34, Zbigniew Jędrzejewski-Szmek wrote: >>> On Sat, Jan 24, 2015 at 10:39:56AM +0200, Topi Miettinen wrote: Leave space for the terminating zero when reading and make sure that the last byte is zero. This also makes the check for long packets equivalent to code before 9c89c1ca: reject also packets with size 8192. --- src/libudev/libudev-monitor.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c index 4cfb2f6..b7fc031 100644 --- a/src/libudev/libudev-monitor.c +++ b/src/libudev/libudev-monitor.c @@ -590,7 +590,7 @@ retry: if (udev_monitor == NULL) return NULL; iov.iov_base = &buf; -iov.iov_len = sizeof(buf); +iov.iov_len = sizeof(buf) - 1; /* Leave space for terminating zero */ memzero(&smsg, sizeof(struct msghdr)); smsg.msg_iov = &iov; smsg.msg_iovlen = 1; @@ -642,6 +642,8 @@ retry: if (udev_device == NULL) return NULL; +buf.raw[sizeof(buf.raw) - 1] = '\0'; + if (memcmp(buf.raw, "libudev", 8) == 0) { /* udev message needs proper version magic */ if (buf.nlh.magic != htonl(UDEV_MONITOR_MAGIC)) { >>> A buffer only needs to be terminated by a zero in certain cases: usually if >>> it >>> is passed to a function which expectes that. iovecs can contain binary data, >>> and have an explicit size field, so they do not need to be zero-terminated. >>> Is there a reason why the buffer has to be zero-terminated in this case? >> >> String functions strcmp, strlen and strstr, used a few lines later, >> expect null byte terminated strings. Alternatively they could be changed >> to strncmp and friends where the scope can be limited to only the buffer. > > But the data comes from the kernel (and that's verified, securely), > hence I am wondering what kind of vulnerability you have precisely in > mind. If we don't trust the kernel, then we'll have quite a problem... Right, I didn't look at the context. In that case, this would be just defensive programming. -Topi > > Lennart > ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev-monitor: ensure proper string termination
On Sun, 25.01.15 07:10, Topi Miettinen (toiwo...@gmail.com) wrote: > On 01/25/15 03:34, Zbigniew Jędrzejewski-Szmek wrote: > > On Sat, Jan 24, 2015 at 10:39:56AM +0200, Topi Miettinen wrote: > >> Leave space for the terminating zero when reading and make sure > >> that the last byte is zero. This also makes the check for long packets > >> equivalent to code before 9c89c1ca: reject also packets with size 8192. > >> --- > >> src/libudev/libudev-monitor.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c > >> index 4cfb2f6..b7fc031 100644 > >> --- a/src/libudev/libudev-monitor.c > >> +++ b/src/libudev/libudev-monitor.c > >> @@ -590,7 +590,7 @@ retry: > >> if (udev_monitor == NULL) > >> return NULL; > >> iov.iov_base = &buf; > >> -iov.iov_len = sizeof(buf); > >> +iov.iov_len = sizeof(buf) - 1; /* Leave space for terminating > >> zero */ > >> memzero(&smsg, sizeof(struct msghdr)); > >> smsg.msg_iov = &iov; > >> smsg.msg_iovlen = 1; > >> @@ -642,6 +642,8 @@ retry: > >> if (udev_device == NULL) > >> return NULL; > >> > >> +buf.raw[sizeof(buf.raw) - 1] = '\0'; > >> + > >> if (memcmp(buf.raw, "libudev", 8) == 0) { > >> /* udev message needs proper version magic */ > >> if (buf.nlh.magic != htonl(UDEV_MONITOR_MAGIC)) { > > A buffer only needs to be terminated by a zero in certain cases: usually if > > it > > is passed to a function which expectes that. iovecs can contain binary data, > > and have an explicit size field, so they do not need to be zero-terminated. > > Is there a reason why the buffer has to be zero-terminated in this case? > > String functions strcmp, strlen and strstr, used a few lines later, > expect null byte terminated strings. Alternatively they could be changed > to strncmp and friends where the scope can be limited to only the buffer. But the data comes from the kernel (and that's verified, securely), hence I am wondering what kind of vulnerability you have precisely in mind. If we don't trust the kernel, then we'll have quite a problem... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev-monitor: ensure proper string termination
On 01/25/15 03:34, Zbigniew Jędrzejewski-Szmek wrote: > On Sat, Jan 24, 2015 at 10:39:56AM +0200, Topi Miettinen wrote: >> Leave space for the terminating zero when reading and make sure >> that the last byte is zero. This also makes the check for long packets >> equivalent to code before 9c89c1ca: reject also packets with size 8192. >> --- >> src/libudev/libudev-monitor.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c >> index 4cfb2f6..b7fc031 100644 >> --- a/src/libudev/libudev-monitor.c >> +++ b/src/libudev/libudev-monitor.c >> @@ -590,7 +590,7 @@ retry: >> if (udev_monitor == NULL) >> return NULL; >> iov.iov_base = &buf; >> -iov.iov_len = sizeof(buf); >> +iov.iov_len = sizeof(buf) - 1; /* Leave space for terminating zero >> */ >> memzero(&smsg, sizeof(struct msghdr)); >> smsg.msg_iov = &iov; >> smsg.msg_iovlen = 1; >> @@ -642,6 +642,8 @@ retry: >> if (udev_device == NULL) >> return NULL; >> >> +buf.raw[sizeof(buf.raw) - 1] = '\0'; >> + >> if (memcmp(buf.raw, "libudev", 8) == 0) { >> /* udev message needs proper version magic */ >> if (buf.nlh.magic != htonl(UDEV_MONITOR_MAGIC)) { > A buffer only needs to be terminated by a zero in certain cases: usually if it > is passed to a function which expectes that. iovecs can contain binary data, > and have an explicit size field, so they do not need to be zero-terminated. > Is there a reason why the buffer has to be zero-terminated in this case? String functions strcmp, strlen and strstr, used a few lines later, expect null byte terminated strings. Alternatively they could be changed to strncmp and friends where the scope can be limited to only the buffer. -Topi > > Zbyszek > ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev-monitor: ensure proper string termination
On Sat, Jan 24, 2015 at 10:39:56AM +0200, Topi Miettinen wrote: > Leave space for the terminating zero when reading and make sure > that the last byte is zero. This also makes the check for long packets > equivalent to code before 9c89c1ca: reject also packets with size 8192. > --- > src/libudev/libudev-monitor.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c > index 4cfb2f6..b7fc031 100644 > --- a/src/libudev/libudev-monitor.c > +++ b/src/libudev/libudev-monitor.c > @@ -590,7 +590,7 @@ retry: > if (udev_monitor == NULL) > return NULL; > iov.iov_base = &buf; > -iov.iov_len = sizeof(buf); > +iov.iov_len = sizeof(buf) - 1; /* Leave space for terminating zero */ > memzero(&smsg, sizeof(struct msghdr)); > smsg.msg_iov = &iov; > smsg.msg_iovlen = 1; > @@ -642,6 +642,8 @@ retry: > if (udev_device == NULL) > return NULL; > > +buf.raw[sizeof(buf.raw) - 1] = '\0'; > + > if (memcmp(buf.raw, "libudev", 8) == 0) { > /* udev message needs proper version magic */ > if (buf.nlh.magic != htonl(UDEV_MONITOR_MAGIC)) { A buffer only needs to be terminated by a zero in certain cases: usually if it is passed to a function which expectes that. iovecs can contain binary data, and have an explicit size field, so they do not need to be zero-terminated. Is there a reason why the buffer has to be zero-terminated in this case? Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] libudev-monitor: ensure proper string termination
Leave space for the terminating zero when reading and make sure that the last byte is zero. This also makes the check for long packets equivalent to code before 9c89c1ca: reject also packets with size 8192. --- src/libudev/libudev-monitor.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c index 4cfb2f6..b7fc031 100644 --- a/src/libudev/libudev-monitor.c +++ b/src/libudev/libudev-monitor.c @@ -590,7 +590,7 @@ retry: if (udev_monitor == NULL) return NULL; iov.iov_base = &buf; -iov.iov_len = sizeof(buf); +iov.iov_len = sizeof(buf) - 1; /* Leave space for terminating zero */ memzero(&smsg, sizeof(struct msghdr)); smsg.msg_iov = &iov; smsg.msg_iovlen = 1; @@ -642,6 +642,8 @@ retry: if (udev_device == NULL) return NULL; +buf.raw[sizeof(buf.raw) - 1] = '\0'; + if (memcmp(buf.raw, "libudev", 8) == 0) { /* udev message needs proper version magic */ if (buf.nlh.magic != htonl(UDEV_MONITOR_MAGIC)) { -- 2.1.4 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel