[PATCH] Set mark to 0 from libnetfilter_conntrack

[PATCH] Set mark to 0 from libnetfilter_conntrack

[Eric Leblond]

[-- Attachment #1.1: Type: text/plain, Size: 519 bytes --]

Hi,

Damien Boucard from INL has discovered a bug in libnetfilter_conntrack :
Mark can not be set to 0.

After looking at the code I've found that we only change the mark if it
is not set to 0 :
	if (ct->mark != 0)
		nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_MARK, &mark,
		       sizeof(u_int32_t));

What's the cleanest way to solve this. I don't see any mean to correct
this except adding an IPS_CHANGE_MARK flag.

Proposed patch is attached to the mail.

BR,
-- 
Eric Leblond <eric@inl.fr>
INL

[-- Attachment #1.2: enable_setting_mark_to_zero.patch --]
[-- Type: text/x-patch, Size: 1015 bytes --]

Index: include/libnetfilter_conntrack/libnetfilter_conntrack.h
===================================================================
--- include/libnetfilter_conntrack/libnetfilter_conntrack.h	(revision 6689)
+++ include/libnetfilter_conntrack/libnetfilter_conntrack.h	(working copy)
@@ -196,6 +196,10 @@
 	IPS_FIXED_TIMEOUT_BIT = 10,
 	IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT),
 
+    /* Connectio must change MARK */
+	IPS_CHANGE_MARK_BIT = 11,
+	IPS_CHANGE_MARK = (1 << IPS_FIXED_CHANGE_MARK),
+
 };
 
 enum {
Index: src/libnetfilter_conntrack.c
===================================================================
--- src/libnetfilter_conntrack.c	(revision 6689)
+++ src/libnetfilter_conntrack.c	(working copy)
@@ -976,7 +976,7 @@
 	nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_TIMEOUT, &timeout, 
 		       sizeof(u_int32_t));
 	
-	if (ct->mark != 0)
+	if (ct->status & IPS_CHANGE_MARK)
 		nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_MARK, &mark,
 			       sizeof(u_int32_t));
 

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH] Set mark to 0 from libnetfilter_conntrack

[Patrick McHardy]

Eric Leblond wrote:
> Hi,
> 
> Damien Boucard from INL has discovered a bug in libnetfilter_conntrack :
> Mark can not be set to 0.
> 
> After looking at the code I've found that we only change the mark if it
> is not set to 0 :
> 	if (ct->mark != 0)
> 		nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_MARK, &mark,
> 		       sizeof(u_int32_t));
> 
> What's the cleanest way to solve this. I don't see any mean to correct
> this except adding an IPS_CHANGE_MARK flag.
> 
> Proposed patch is attached to the mail.
> 
> BR,
> 
> 
> ------------------------------------------------------------------------
> 
> Index: include/libnetfilter_conntrack/libnetfilter_conntrack.h
> ===================================================================
> --- include/libnetfilter_conntrack/libnetfilter_conntrack.h	(revision 6689)
> +++ include/libnetfilter_conntrack/libnetfilter_conntrack.h	(working copy)
> @@ -196,6 +196,10 @@
>  	IPS_FIXED_TIMEOUT_BIT = 10,
>  	IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT),
>  
> +    /* Connectio must change MARK */
> +	IPS_CHANGE_MARK_BIT = 11,
> +	IPS_CHANGE_MARK = (1 << IPS_FIXED_CHANGE_MARK),
> +
>  };
>  
>  enum {
> Index: src/libnetfilter_conntrack.c
> ===================================================================
> --- src/libnetfilter_conntrack.c	(revision 6689)
> +++ src/libnetfilter_conntrack.c	(working copy)
> @@ -976,7 +976,7 @@
>  	nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_TIMEOUT, &timeout, 
>  		       sizeof(u_int32_t));
>  	
> -	if (ct->mark != 0)
> +	if (ct->status & IPS_CHANGE_MARK)
>  		nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_MARK, &mark,
>  			       sizeof(u_int32_t));
>  

I don't see anything setting this bit and it shouldn't be a conntrack
bit if it doesn't exist in the kernel (and we certainly don't want this
in the kernel). The idea is still the right one, I think the library
should take care of adding a CTA_MARK attribute without any user bitmask
fiddling by including it if the value differs from the mark contained in
the received conntrack. I think Pablo's new API will allow this.

Re: [PATCH] Set mark to 0 from libnetfilter_conntrack

[Eric Leblond]

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

Hello,

Le jeudi 26 octobre 2006 à 01:37 +0200, Patrick McHardy a écrit :
> The idea is still the right one, I think the library
> should take care of adding a CTA_MARK attribute without any user bitmask
> fiddling by including it if the value differs from the mark contained in
> the received conntrack. I think Pablo's new API will allow this.

What's the status of this new API ? Current status of
libnetfilter_conntrack code is really disappointing.

We at INL are currently working on using libnetfilter_conntrack to
provide tools for firewall administrators. The first one
pynetfilter_conntrack is already available and a web management
interface will soon be released. By doing this we are trying to improve
the usability of this new system.

We are doing this because we think the new features provided by
libnetfilter_conntrack really improve a lot the way we use and "live"
with a Netfilter firewall.

Sadly, the uncertainty on code evolution and the lack of frequent
releases are wounding the expansion of these ideas.

From my point of view, a decision on libnetfilter_conntrack
developpement should be made quickly :
      * Should pablo's version become the next step in this branch ? or
      * Is it necessary to create a new branch to keep a stable version
        and have compatibility with the few existing applications ?

Best regards,

PS:
pynetfilter_conntrack : http://software.inl.fr/trac/trac.cgi/wiki/pynetfilter_conntrack
-- 
Eric Leblond <eric@inl.fr>

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Set mark to 0 from libnetfilter_conntrack

[Pablo Neira Ayuso]

Eric Leblond wrote:
> Hello,
> 
> Le jeudi 26 octobre 2006 à 01:37 +0200, Patrick McHardy a écrit :
>> The idea is still the right one, I think the library
>> should take care of adding a CTA_MARK attribute without any user bitmask
>> fiddling by including it if the value differs from the mark contained in
>> the received conntrack. I think Pablo's new API will allow this.
> 
> What's the status of this new API ? Current status of
> libnetfilter_conntrack code is really disappointing.

Indeed, you can blame me for that. I'm working on the new API that I'll 
try to release asap. I'm going to put this in my high priority queue.

> We at INL are currently working on using libnetfilter_conntrack to
> provide tools for firewall administrators. The first one
> pynetfilter_conntrack is already available and a web management
> interface will soon be released. By doing this we are trying to improve
> the usability of this new system.

I'd like to see your python binding in the libnetfilter_conntrack tree 
but I have to fix some stuff before, please consider that this would 
require some extra work from your part.

> We are doing this because we think the new features provided by
> libnetfilter_conntrack really improve a lot the way we use and "live"
> with a Netfilter firewall.
> 
> Sadly, the uncertainty on code evolution and the lack of frequent
> releases are wounding the expansion of these ideas.

We are working to improve the release process, expect changes soon.

> From my point of view, a decision on libnetfilter_conntrack
> developpement should be made quickly :
>       * Should pablo's version become the next step in this branch ? or
>       * Is it necessary to create a new branch to keep a stable version
>         and have compatibility with the few existing applications ?

I already commented what my intentions are: add more new API, do not 
remove the old one but mark it as deprecated, strongly recommend people 
to move to the new API. The deprecated API will be removed after quite 
some time.

-- 
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] Set mark to 0 from libnetfilter_conntrack

[Eric Leblond]

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

Hi,

Le vendredi 27 octobre 2006 à 15:53 +0200, Pablo Neira Ayuso a écrit :
> Eric Leblond wrote:
> > What's the status of this new API ? Current status of
> > libnetfilter_conntrack code is really disappointing.
> 
> Indeed, you can blame me for that. I'm working on the new API that I'll 
> try to release asap. I'm going to put this in my high priority queue.

That's good news !

> 
> > We at INL are currently working on using libnetfilter_conntrack to
> > provide tools for firewall administrators. The first one
> > pynetfilter_conntrack is already available and a web management
> > interface will soon be released. By doing this we are trying to improve
> > the usability of this new system.
> 
> I'd like to see your python binding in the libnetfilter_conntrack tree 
> but I have to fix some stuff before, please consider that this would 
> require some extra work from your part.

No problem. We really think it's the correct place for this code and it
will worth the work to put it there.

> 
> > We are doing this because we think the new features provided by
> > libnetfilter_conntrack really improve a lot the way we use and "live"
> > with a Netfilter firewall.
>
> I already commented what my intentions are: add more new API, do not 
> remove the old one but mark it as deprecated, strongly recommend people 
> to move to the new API. The deprecated API will be removed after quite 
> some time.

Is it possible to introduce a version number of the API in a #define to
be able to do some conditional compilations ?

BR,
-- 
Eric Leblond <eric@inl.fr>
INL

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 191 bytes --]