Re: [systemd-devel] [PATCH] libudev-monitor: ensure proper string termination

[Topi Miettinen]

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

[Lennart Poettering]

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

[Topi Miettinen]

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

[Zbigniew Jędrzejewski-Szmek]

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

[Topi Miettinen]

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