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
