Message ID | 502A2624.6050701@redhat.com |
---|---|
State | Not Applicable |
Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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