[PATCH] libnfnetlink

[PATCH] libnfnetlink

[Maik Hentsche]

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

Hello Pablo, hello readers of the list,
I found another bug in libnfnetlink. The comment of nfnl_recv states, in
case of success 0 is returned. In fact at success the returnvalue of
recvfrom is returned, which is the number of received bytes
(libnfnetlink_recv_comment.patch). The second issue is a little more
serious. The comment states, in case of an error, errno is set when in
fact it is not. I appended a patch for two occurences, but I since I
don't know, in which case addrlen might be != sizeof(peer) and what
peer.nl_pid means (and therefore why it is a problem, if it's not 0)
two error cases without appropriate errno value still exist.

so long
Maik

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libnfnetlink_recv_errno.patch --]
[-- Type: text/x-patch; name="libnfnetlink_recv_errno.patch", Size: 850 bytes --]

diff -U 3 -b -B -d -i -r -N -x .svn -- libnfnetlink-0.0.16-pablo-r1/src/libnfnetlink.c libnfnetlink-0.0.16-pablo-r1.new/src/libnfnetlink.c
--- libnfnetlink-0.0.16-pablo-r1/src/libnfnetlink.c	2006-09-26 10:00:12.000000000 +0200
+++ libnfnetlink-0.0.16-pablo-r1.new/src/libnfnetlink.c	2006-09-29 09:50:44.000000000 +0200
@@ -462,8 +462,10 @@
 	assert(len > 0);
 	
 	if (len < sizeof(struct nlmsgerr)
-	    || len < sizeof(struct nlmsghdr))
+	    || len < sizeof(struct nlmsghdr)){
+		errno = ENOBUFS;
 		return -1; 
+	}
 
 	addrlen = sizeof(h->peer);
 	status = recvfrom(h->fd, buf, len, 0, (struct sockaddr *)&peer,	
@@ -478,8 +480,10 @@
 		return -1;
 
 	nlh = (struct nlmsghdr *)buf;
-	if (nlh->nlmsg_flags & MSG_TRUNC || status > len)
+	if (nlh->nlmsg_flags & MSG_TRUNC || status > len){
+		errno = ENOBUFS;
 		return -1;
+	}
 
 	return status;
 }

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: libnfnetlink_recv_comment.patch --]
[-- Type: text/x-patch; name="libnfnetlink_recv_comment.patch", Size: 790 bytes --]

diff -U 3 -b -B -d -i -r -N -x .svn -- libnfnetlink-0.0.16-pablo-r1/src/libnfnetlink.c libnfnetlink-0.0.16-pablo-r1.new/src/libnfnetlink.c
--- libnfnetlink-0.0.16-pablo-r1/src/libnfnetlink.c	2006-09-29 09:54:27.000000000 +0200
+++ libnfnetlink-0.0.16-pablo-r1.new/src/libnfnetlink.c	2006-09-29 10:17:11.000000000 +0200
@@ -443,8 +443,8 @@
  * that the data is well-formed. Such checkings are done by the parsing
  * functions.
  *
- * On success, 0 is returned. On error, -1 is returned and errno is set
- * appropiately.
+ * On success, the number of bytes received is returned. On error, -1 is
+ * returned and errno is set appropiately.
  *
  * Note that ENOBUFS is returned in case that nfnetlink is exhausted. In
  * that case is possible that the information requested is incomplete.

Re: [PATCH] libnfnetlink

[Patrick McHardy]

Maik Hentsche wrote:
> Hello Pablo, hello readers of the list,
> I found another bug in libnfnetlink. The comment of nfnl_recv states, in
> case of success 0 is returned. In fact at success the returnvalue of
> recvfrom is returned, which is the number of received bytes
> (libnfnetlink_recv_comment.patch). The second issue is a little more
> serious. The comment states, in case of an error, errno is set when in
> fact it is not. I appended a patch for two occurences, but I since I
> don't know, in which case addrlen might be != sizeof(peer) and what
> peer.nl_pid means (and therefore why it is a problem, if it's not 0)
> two error cases without appropriate errno value still exist.

addrlen != sizeof(peer) should never happen. I can't think of anything
better than EINVAL. nl_pid != 0 means the message originated in
userspace and some other program is trying to feed us messages.
We could handle this by just calling recvmsg again. But this is mainly
because I can't think of a proper errno code for this either :)

Re: [PATCH] libnfnetlink

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Maik Hentsche wrote:
>> Hello Pablo, hello readers of the list,
>> I found another bug in libnfnetlink. The comment of nfnl_recv states, in
>> case of success 0 is returned. In fact at success the returnvalue of
>> recvfrom is returned, which is the number of received bytes
>> (libnfnetlink_recv_comment.patch). The second issue is a little more
>> serious. The comment states, in case of an error, errno is set when in
>> fact it is not. I appended a patch for two occurences, but I since I
>> don't know, in which case addrlen might be != sizeof(peer) and what
>> peer.nl_pid means (and therefore why it is a problem, if it's not 0)
>> two error cases without appropriate errno value still exist.
> 
> addrlen != sizeof(peer) should never happen. I can't think of anything
> better than EINVAL. nl_pid != 0 means the message originated in
> userspace and some other program is trying to feed us messages.
> We could handle this by just calling recvmsg again. But this is mainly
> because I can't think of a proper errno code for this either :)

what do you think about the following solution?

> 	if (len < sizeof(struct nlmsgerr)
> 	    || len < sizeof(struct nlmsghdr))

errno = EBADMSG;

> [...]
> 	if (addrlen != sizeof(peer))

errno = EINVAL;

> 		return -1;
> 
> 	if (peer.nl_pid != 0)

errno = ENOMSG;

> 		return -1;
> 
> 	nlh = (struct nlmsghdr *)buf;
> 	if (nlh->nlmsg_flags & MSG_TRUNC || status > len)

errno = ENOSPC;

> 		return -1;

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH] libnfnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> what do you think about the following solution?
> 
> 
>>	if (len < sizeof(struct nlmsgerr)
>>	    || len < sizeof(struct nlmsghdr))
> 
> 
> errno = EBADMSG;
> 
> 
>>[...]
>>	if (addrlen != sizeof(peer))
> 
> 
> errno = EINVAL;
> 
> 
>>		return -1;
>>
>>	if (peer.nl_pid != 0)
> 
> 
> errno = ENOMSG;

The above all seem fine.

> 
>>		return -1;
>>
>>	nlh = (struct nlmsghdr *)buf;
>>	if (nlh->nlmsg_flags & MSG_TRUNC || status > len)
> 
> 
> errno = ENOSPC;

ENOSPC is fine for MSG_TRUNC, but it is a msghdr flag, not nlmsg.
status > len implies serious recvmsg brokeness and it doesn't
really make sense to check for kernel bugs, so I'd remove it.

[PATCH] libnfnetlink

[Maik Hentsche]

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

Hallo pablo, hello readers of the list,
attached is a patch, that corrects another error in the comments of
libnfnetlink. nfnl_send uses send_to and thus returns the number of
send bytes instead of 0 in case of success. Pardon me for bothering you
with such trivial patches, but I think, as long as the
sgml-documentation isn't finished (I'm working on it), developers using
the lib might need those comments.

so long
Maik


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libnfnetlink_send_comment.patch --]
[-- Type: text/x-patch; name="libnfnetlink_send_comment.patch", Size: 672 bytes --]

diff -U 3 -b -B -d -i -r -N -x .svn -- libnfnetlink-0.0.16-pablo-r1/src/libnfnetlink.c libnfnetlink-0.0.16-pablo-r1.new/src/libnfnetlink.c
--- libnfnetlink-0.0.16-pablo-r1/src/libnfnetlink.c	2006-10-09 16:54:30.000000000 +0200
+++ libnfnetlink-0.0.16-pablo-r1.new/src/libnfnetlink.c	2006-10-09 16:52:43.000000000 +0200
@@ -330,8 +330,8 @@
  * @nfnlh: nfnetlink handler
  * @n: netlink message
  *
- * On success, 0 is returned. On error, -1 is returned and errno is set
- * appropiately.
+ * On success, the number of send bytes is returned. On error, -1 is
+ * returned and errno is set appropiately.
  */
 int nfnl_send(struct nfnl_handle *nfnlh, struct nlmsghdr *n)
 {

Re: [PATCH] libnfnetlink

[Pablo Neira Ayuso]

Maik,

Maik Hentsche wrote:
> Hallo pablo, hello readers of the list,
> attached is a patch, that corrects another error in the comments of
> libnfnetlink. nfnl_send uses send_to and thus returns the number of
> send bytes instead of 0 in case of success. Pardon me for bothering you
> with such trivial patches, but I think, as long as the
> sgml-documentation isn't finished (I'm working on it), developers using
> the lib might need those comments.

Thanks it is worth the fix, I'll add it to my patchlet for libnfnetlink.

--
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris