[PATCH] mnl_attr_get_str: document that the result is not NUL-terminated

[PATCH] mnl_attr_get_str: document that the result is not NUL-terminated

[Florian Weimer]

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

Hi,

while reviewing from libmnl-using code, I discovered that the result of 
mnl_attr_get_str was used as a NUL-terminated string, although in 
reality the string wasn't.  I think this should be mentioned in the 
documentation.

Florian
-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: 0001-mnl_attr_get_str-document-that-the-result-is-not-NUL.patch --]
[-- Type: text/x-patch, Size: 769 bytes --]

>From 1e4b192907eb15f4fbfd581c8c0fa8359aa9439f Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Tue, 14 Aug 2012 12:16:32 +0200
Subject: [PATCH] mnl_attr_get_str: document that the result is not
 NUL-terminated

Signed-off-by: Florian Weimer <fweimer@redhat.com>
---
 src/attr.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/attr.c b/src/attr.c
index 1136c50..c890bf2 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -393,6 +393,7 @@ EXPORT_SYMBOL(mnl_attr_get_u64);
  * \param attr pointer to netlink attribute
  *
  * This function returns the payload of string attribute value.
+ * Note that the string is not necessarily NUL-terminated.
  */
 const char *mnl_attr_get_str(const struct nlattr *attr)
 {
-- 
1.7.7.6

Re: [PATCH] mnl_attr_get_str: document that the result is not NUL-terminated

[Jan Engelhardt]

On Tuesday 2012-08-14 12:19, Florian Weimer wrote:

> Hi,
>
> while reviewing from libmnl-using code, I discovered that the result of
> mnl_attr_get_str was used as a NUL-terminated string, although in reality the
> string wasn't.  I think this should be mentioned in the documentation.

IIRC, mnl_attr_get_str should only be called for NLA_NUL_STRING attrs.

Re: [PATCH] mnl_attr_get_str: document that the result is not NUL-terminated

[Pablo Neira Ayuso]

On Tue, Aug 14, 2012 at 12:19:16PM +0200, Florian Weimer wrote:
> Hi,
>
> while reviewing from libmnl-using code, I discovered that the result
> of mnl_attr_get_str was used as a NUL-terminated string, although in
> reality the string wasn't.  I think this should be mentioned in the
> documentation.

Looking at the kernel:

static inline int nla_put_string(struct sk_buff *skb, int attrtype,
                                 const char *str)
{
        return nla_put(skb, attrtype, strlen(str) + 1, str);
}

It seems it always returns the string plus one byte (that is assumed
to be NULL).

Are you looking at any netlink subsystem in particular? What are you
noticing?

> Florian
> --
> Florian Weimer / Red Hat Product Security Team

> From 1e4b192907eb15f4fbfd581c8c0fa8359aa9439f Mon Sep 17 00:00:00 2001
> From: Florian Weimer <fweimer@redhat.com>
> Date: Tue, 14 Aug 2012 12:16:32 +0200
> Subject: [PATCH] mnl_attr_get_str: document that the result is not
>  NUL-terminated
>
> Signed-off-by: Florian Weimer <fweimer@redhat.com>
> ---
>  src/attr.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/attr.c b/src/attr.c
> index 1136c50..c890bf2 100644
> --- a/src/attr.c
> +++ b/src/attr.c
> @@ -393,6 +393,7 @@ EXPORT_SYMBOL(mnl_attr_get_u64);
>   * \param attr pointer to netlink attribute
>   *
>   * This function returns the payload of string attribute value.
> + * Note that the string is not necessarily NUL-terminated.
>   */
>  const char *mnl_attr_get_str(const struct nlattr *attr)
>  {
> --
> 1.7.7.6
>

Re: [PATCH] mnl_attr_get_str: document that the result is not NUL-terminated

[Florian Weimer]

On 08/14/2012 01:10 PM, Pablo Neira Ayuso wrote:
> On Tue, Aug 14, 2012 at 12:19:16PM +0200, Florian Weimer wrote:
>> Hi,
>>
>> while reviewing from libmnl-using code, I discovered that the result
>> of mnl_attr_get_str was used as a NUL-terminated string, although in
>> reality the string wasn't.  I think this should be mentioned in the
>> documentation.
>
> Looking at the kernel:
>
> static inline int nla_put_string(struct sk_buff *skb, int attrtype,
>                                   const char *str)
> {
>          return nla_put(skb, attrtype, strlen(str) + 1, str);
> }
>
> It seems it always returns the string plus one byte (that is assumed
> to be NULL).
>
> Are you looking at any netlink subsystem in particular? What are you
> noticing?

Here is what happens: The kernel sends an attribute of type 
NLA_NUL_STRING.  The application checks it with mnl_attr_validate(attr, 
MNL_TYPE_STRING).  The application then proceeds to call 
mnl_attr_get_str(attr) and uses the result as if it were NUL-terminated. 
  So if the Netlink packet actually comes from the kernel, the 
application code is correct in the sense that it works with current 
libmnl. If the packet does not come from the kernel, the application is 
incorrect.

Based on your comments, I think the application should use 
mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) instead.  In this light, 
the documentation for mnl_attr_get_str should probably suggest to 
validate with MNL_TYPE_NUL_STRING before calling mnl_attr_get_str, and 
my original patch is wrong.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] mnl_attr_get_str: document that the result is not NUL-terminated

[Pablo Neira Ayuso]

On Tue, Aug 14, 2012 at 01:28:40PM +0200, Florian Weimer wrote:
> On 08/14/2012 01:10 PM, Pablo Neira Ayuso wrote:
> >On Tue, Aug 14, 2012 at 12:19:16PM +0200, Florian Weimer wrote:
> >>Hi,
> >>
> >>while reviewing from libmnl-using code, I discovered that the result
> >>of mnl_attr_get_str was used as a NUL-terminated string, although in
> >>reality the string wasn't.  I think this should be mentioned in the
> >>documentation.
> >
> >Looking at the kernel:
> >
> >static inline int nla_put_string(struct sk_buff *skb, int attrtype,
> >                                  const char *str)
> >{
> >         return nla_put(skb, attrtype, strlen(str) + 1, str);
> >}
> >
> >It seems it always returns the string plus one byte (that is assumed
> >to be NULL).
> >
> >Are you looking at any netlink subsystem in particular? What are you
> >noticing?
> 
> Here is what happens: The kernel sends an attribute of type
> NLA_NUL_STRING.  The application checks it with
> mnl_attr_validate(attr, MNL_TYPE_STRING).  The application then
> proceeds to call mnl_attr_get_str(attr) and uses the result as if it
> were NUL-terminated.  So if the Netlink packet actually comes from
> the kernel, the application code is correct in the sense that it
> works with current libmnl. If the packet does not come from the
> kernel, the application is incorrect.
> 
> Based on your comments, I think the application should use
> mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) instead.  In this
> light, the documentation for mnl_attr_get_str should probably
> suggest to validate with MNL_TYPE_NUL_STRING before calling
> mnl_attr_get_str, and my original patch is wrong.

That makes sense. MNL_TYPE_NUL_STRING should be used if possible.

Still, IIRC, old kernels used to send strings without guaranteeing
NULL-termination of strings.

So validating this with MNL_TYPE_NUL_STRING may result in problems if
your application needs to work with an old kernel.