[PATCH] SMACK netfilter smacklabel socket match

[PATCH] SMACK netfilter smacklabel socket match

[etienne]

hello,


i was playing with smack, trying to do funny things
Alas, when I use a 'labelled process' and try to access internet, packet are dropped sooner or later (because of ip options)

I tried to 
echo 0.0.0.0/0 @ > /smack/netlabel 
with no success...


looking at security/smack/smack_lsm.c:smack_host_label
the following lines

bestmask.s_addr = 0;
...
if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
   continue;

if the dest we try to reach match the 0.0.0.0/0, this condition will be true (either because we have a better match or because, well (miap->s_addr | bestmask.s_addr) == bestmask.s_addr == 0

So let the 0.0.0.0/0 a chance!

I realize this patch is a little ugly, a cleaner way would be to insert  struct smk_netlbladdr sorted from longest to smallest mask and break the loop as soon as we have a match...
regards,
Etienne 



Signed-off-by: Etienne <etienne.basset@numericable.fr>
------
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0278bc0..9d2576d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1540,7 +1540,7 @@ static char *smack_host_label(struct sockaddr_in *sip)
                 * If the list entry mask is less specific than the best
                 * already found this entry is uninteresting.
                 */
-               if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
+               if ( ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr) &&  (miap->s_addr | bestmask.s_addr) != 0  )
                        continue;
                /*
                 * This is better than any entry found so far.

[PATCH] SMACK smacklabel : apply &MASK to IP inserted in /smack/netlabel

[etienne]

hello,

with current code it is possible to insert inconsistent IP/mask in /smack/netlabel

before patch :
==============

root@etienne-desktop:/home/etienne/linux-2.6# cat /smack/netlabel
12.67.3.2/15 @
12.67.3.1/15 @
12.67.2.1/15 @
12.67.2.1/16 @
12.67.1.1/16 @
0.0.0.0/0 @

the solution is to apply the mask to the IP inserted in /smack/netlabel

after the patch:
================
root@etienne-desktop:/home/etienne/linux-2.6# echo 12.67.3.2/15 @ > /smack/netlabel           
root@etienne-desktop:/home/etienne/linux-2.6# cat /smack/netlabel                             
12.67.0.0/15 @                                                                                
root@etienne-desktop:/home/etienne/linux-2.6# echo 12.67.3.1/15 @ > /smack/netlabel                             
root@etienne-desktop:/home/etienne/linux-2.6# cat /smack/netlabel                             
12.67.0.0/15 @                                                                                
root@etienne-desktop:/home/etienne/linux-2.6# echo 12.67.3.3/15 @ > /smack/netlabel                             
root@etienne-desktop:/home/etienne/linux-2.6# cat /smack/netlabel                             
12.67.0.0/15 @                                                  

regards,
Etienne

Signed-off-by: <etienen.basset@numericable.fr>
----
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 8e42800..5717150 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -765,6 +765,7 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
                mask.s_addr |= bebits;
                bebits <<= 1;
        }
+       newname.sin_addr.s_addr &= mask.s_addr;
        /*
         * Only allow one writer at a time. Writes should be
         * quite rare and small in any case.

Re: [PATCH] SMACK smacklabel : apply &MASK to IP inserted in /smack/netlabel

[Paul Moore]

On Tuesday 17 February 2009 03:32:15 pm etienne wrote:
> ----
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 8e42800..5717150 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -765,6 +765,7 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, mask.s_addr |= bebits;
>                 bebits <<= 1;
>         }
> +       newname.sin_addr.s_addr &= mask.s_addr;
>         /*
>          * Only allow one writer at a time. Writes should be
>          * quite rare and small in any case.

If you do this you can simplify some of the code in smack_host_label() by 
removing the code which applies the mask to the stored addresses when 
comparing addresses.  There may be other places as well.

-- 
paul moore
linux @ hp

Re: [PATCH] SMACK smacklabel : apply &MASK to IP inserted in /smack/netlabel

[Casey Schaufler]

Paul Moore wrote:
> On Tuesday 17 February 2009 03:32:15 pm etienne wrote:
>   
>> ----
>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>> index 8e42800..5717150 100644
>> --- a/security/smack/smackfs.c
>> +++ b/security/smack/smackfs.c
>> @@ -765,6 +765,7 @@ static ssize_t smk_write_netlbladdr(struct file *file,
>> const char __user *buf, mask.s_addr |= bebits;
>>                 bebits <<= 1;
>>         }
>> +       newname.sin_addr.s_addr &= mask.s_addr;
>>         /*
>>          * Only allow one writer at a time. Writes should be
>>          * quite rare and small in any case.
>>     
>
> If you do this you can simplify some of the code in smack_host_label() by 
> removing the code which applies the mask to the stored addresses when 
> comparing addresses.  There may be other places as well.
>
>   
1234567890123456789012345678901234567890123456789012345678901234567890

Thank all of you for your kind suggestions. I'm in the process
of cleaning up after a meltdown in the Smack test lab, but I will
look into this as soon as I can. Did I ever say how much I dislike
netmasks?

Re: [PATCH] SMACK smacklabel : apply &MASK to IP inserted in /smack/netlabel

[etienne]

Paul Moore wrote:
> On Tuesday 17 February 2009 03:32:15 pm etienne wrote:
>> ----
>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>> index 8e42800..5717150 100644
>> --- a/security/smack/smackfs.c
>> +++ b/security/smack/smackfs.c
>> @@ -765,6 +765,7 @@ static ssize_t smk_write_netlbladdr(struct file *file,
>> const char __user *buf, mask.s_addr |= bebits;
>>                 bebits <<= 1;
>>         }
>> +       newname.sin_addr.s_addr &= mask.s_addr;
>>         /*
>>          * Only allow one writer at a time. Writes should be
>>          * quite rare and small in any case.
> 
> If you do this you can simplify some of the code in smack_host_label() by 
> removing the code which applies the mask to the stored addresses when 
> comparing addresses.  There may be other places as well.
> 
you're right, in mk_write_netlbladdr also, i'll have a look
thanks,
Etienne

Re: [PATCH] SMACK netfilter smacklabel socket match

[David Miller]

From: etienne <etienne.basset@numericable.fr>
Date: Tue, 17 Feb 2009 21:01:15 +0100

> -               if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
> +               if ( ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr) &&  (miap->s_addr | bestmask.s_addr) != 0  )

Please don't add all of those new spaces and please add a newline
after the "&&" so that the line of code does not exceed 80
columns.

Re: [PATCH] SMACK netfilter smacklabel socket match

[Paul Moore]

On Tuesday 17 February 2009 03:01:15 pm etienne wrote:
> I realize this patch is a little ugly, a cleaner way would be to insert 
> struct smk_netlbladdr sorted from longest to smallest mask and break the
> loop as soon as we have a match... regards,

Agreed, the address matching code really should be improved; if you feel like 
you could contribute the changes I'm pretty sure Casey would welcome the 
patches :)

Regarding your fix below, I think a cleaner solution would be to do something 
like the following in place of the existing mask check ...

	if ((miap->s_addr & bestmask.s_addr) || (bestmask.s_addr == 0)) {
		bestmask.s_addr = miap->s_addr;
		bestlabel = snp->smk_label;
	}	

... however there is one small problem with this approach (your proposal 
suffers from the same issue): normally the smack_host_label() code prefers the 
first matching entry in the list, the change above preserves that with the 
exception of a 0.0.0.0/0 entry.  Granted, you shouldn't allow that in the 
first place but I believe it is possible so it is something that needs to be 
taken into consideration.

> Signed-off-by: Etienne <etienne.basset@numericable.fr>
> ------
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0278bc0..9d2576d 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1540,7 +1540,7 @@ static char *smack_host_label(struct sockaddr_in
> *sip) * If the list entry mask is less specific than the best * already
> found this entry is uninteresting.
>                  */
> -               if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
> +               if ( ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
> &&  (miap->s_addr | bestmask.s_addr) != 0  ) continue;
>                 /*
>                  * This is better than any entry found so far.
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in the body of a message to
> majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
paul moore
linux @ hp

Re: [PATCH] SMACK netfilter smacklabel socket match

[etienne]

Paul Moore wrote:
> On Tuesday 17 February 2009 03:01:15 pm etienne wrote:
>> I realize this patch is a little ugly, a cleaner way would be to insert 
>> struct smk_netlbladdr sorted from longest to smallest mask and break the
>> loop as soon as we have a match... regards,
> 
> Agreed, the address matching code really should be improved; if you feel like 
> you could contribute the changes I'm pretty sure Casey would welcome the 
> patches :)
> 
yes I could try that, this week-end maybe

> Regarding your fix below, I think a cleaner solution would be to do something 
> like the following in place of the existing mask check ...
> 
> 	if ((miap->s_addr & bestmask.s_addr) || (bestmask.s_addr == 0)) {
> 		bestmask.s_addr = miap->s_addr;
> 		bestlabel = snp->smk_label;
> 	}	
> 
> ... however there is one small problem with this approach (your proposal 
> suffers from the same issue): normally the smack_host_label() code prefers the 
> first matching entry in the list, the change above preserves that with the 
> exception of a 0.0.0.0/0 entry.  Granted, you shouldn't allow that in the 
> first place but I believe it is possible so it is something that needs to be 
> taken into consideration.
> 
hummm... I didn't see it that way; I think this function is basically a reimplementation of IPv4 classless routing (longest match first)?
anyway, I think the cleanest way would be to, well, sort smk_netlbladdr by mask on insertion (perf doesn't matter  here) and this way smack_host_label can stop the loop on first match.
Plus, it would give a nicer /smack/netlabel ouptut :)

so, how should we handle it? apply the patches (with whitespaces damages corrected ;) )  now (as it corrects a bug) an elaborate the cleaner way later?
I think this should go to stable too?

regards
Etienne

>> Signed-off-by: Etienne <etienne.basset@numericable.fr>
>> ------
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 0278bc0..9d2576d 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -1540,7 +1540,7 @@ static char *smack_host_label(struct sockaddr_in
>> *sip) * If the list entry mask is less specific than the best * already
>> found this entry is uninteresting.
>>                  */
>> -               if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
>> +               if ( ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
>> &&  (miap->s_addr | bestmask.s_addr) != 0  ) continue;
>>                 /*
>>                  * This is better than any entry found so far.

Re: [PATCH] SMACK netfilter smacklabel socket match

[Paul Moore]

On Wednesday 18 February 2009 02:23:24 am etienne wrote:
> ... anyway, I think the cleanest way would be to, well, sort smk_netlbladdr
> by mask on insertion (perf doesn't matter  here) and this way
> smack_host_label can stop the loop on first match. Plus, it would give a
> nicer /smack/netlabel ouptut :)

Agreed.

> so, how should we handle it? apply the patches (with whitespaces damages
> corrected ;) )  now (as it corrects a bug) an elaborate the cleaner way
> later?

Well, since you have some time and willingness to do things "the right way" I 
would recommend dropping these patches (which are really just band-aids) and 
working on the right solution to stored the addresses/masks in a sorted list 
with the mask already applied.

FWIW, the NetLabel code (net/netlabel) has to do very similar things with 
sorted address lists so I built an address list construct which builds on the 
list.h ideas and operates in a similar way.  You may find it helpful.

> I think this should go to stable too?

I would worry about getting the patches developed, tested and in an acceptable 
form first, then we can worry about where they should be applied ;)

-- 
paul moore
linux @ hp

Re: [PATCH] SMACK netfilter smacklabel socket match

[Casey Schaufler]

Paul Moore wrote:
> On Wednesday 18 February 2009 02:23:24 am etienne wrote:
>   
>> ... anyway, I think the cleanest way would be to, well, sort smk_netlbladdr
>> by mask on insertion (perf doesn't matter  here) and this way
>> smack_host_label can stop the loop on first match. Plus, it would give a
>> nicer /smack/netlabel ouptut :)
>>     
>
> Agreed.
>   

Yes, it would make it nicer. You'll need to do a better job
on the list management than I've been doing. It's probably well
past time to introduce the Standard list management scheme to
Smack, and you'll need to do so if you want to do insertions
and/or deletions.

>> so, how should we handle it? apply the patches (with whitespaces damages
>> corrected ;) )  now (as it corrects a bug) an elaborate the cleaner way
>> later?
>>     
>
> Well, since you have some time and willingness to do things "the right way" I 
> would recommend dropping these patches (which are really just band-aids) and 
> working on the right solution to stored the addresses/masks in a sorted list 
> with the mask already applied.
>
> FWIW, the NetLabel code (net/netlabel) has to do very similar things with 
> sorted address lists so I built an address list construct which builds on the 
> list.h ideas and operates in a similar way.  You may find it helpful.
>
>   
>> I think this should go to stable too?
>>     
>
> I would worry about getting the patches developed, tested and in an acceptable 
> form first, then we can worry about where they should be applied ;)
>
>   

I would be delighted to see these changes. When you have preliminary
versions I would be eager to see them and give them a try in the
Smack test laboratory.

Etienne, thank you very much for the work you've done so far. Paul,
thank you for your recommendations.

Re: [PATCH] SMACK netfilter smacklabel socket match

[etienne]

Casey Schaufler wrote:
> Paul Moore wrote:
>> On Wednesday 18 February 2009 02:23:24 am etienne wrote:
....
> Yes, it would make it nicer. You'll need to do a better job
> on the list management than I've been doing. It's probably well
> past time to introduce the Standard list management scheme to
> Smack, and you'll need to do so if you want to do insertions
> and/or deletions.
> 
well, we could maybe do that for smack_netlbladdrs.
for smk_rules, i don't know, depending to the use case, it could grow bigger and thus need a more efficient scheme than linked-list like hash-table.



[..]
>>   
> 
> I would be delighted to see these changes. When you have preliminary
> versions I would be eager to see them and give them a try in the
> Smack test laboratory.
>
OK, will send tonight

 
> Etienne, thank you very much for the work you've done so far. Paul,
> thank you for your recommendations.
> 

well, I'll try to explain my use case for SMACK, could you please tell me if this makes sense and if it is doable and sane with SMACK :

I have single-user computer that, for simplicity sake, do only web browsing with firefox; 
the attack vector i'm concerned with is malicious web pages, that could execute malicious code on my computer or worse erase some of my data; 

so i express the following security policy in a tool-agnostic way :
1. firefox can access internet
2. firefox can read/write it's configuration directory in my $HOME
3. firefox can read/write to a download directory
4. firefox can execute kpdf, okular, vlc etc...
5. firefox can read system files
6. firefox can write to temporary folder

pretty simple. So I expect the 'tool' to express this policy in very few line; (i had a look at selinux/refpolicy, and I'm ashamed I was too lazy to test/understand further). And if possible a mainline tool would be a big bonus.

So I decided to give  smack  a try, and here are my notes/interrogations :

rule 1. if i understand correctly, I have to load the following smack rule  
"firefox _ rwx" 
well, as '_' is the default objectlabel for all system files, it means that firefox will have smack 'w' access on system. 

So first issue : is it possible to express network access in another way?
Or maybe I have to relabel  /bin/, /sbin etc with a custom system label ?

rule 2-6 : easy to implement with smack, i label my $HOME with some label and download/cfg dir with other labels
Firefox won't have rw access to my $HOME hehe

Second issue : what is the simplest way to start firefox with the firefox label?
I used the following hack : write a small program (i used cap_mac_admin, could have been suid) that :
a) set /proc/self/attr/current
b) drop capabilities
c) start firefox
Is there a cleanest way, can a process be started with its objectlabel?

Third issue : there seems to be  no way to log/audit access violations, have you plans to implement that?

best regards,
Etienne

Re: [PATCH] SMACK netfilter smacklabel socket match

[Paul Moore]

On Wednesday 18 February 2009 02:35:00 pm etienne wrote:
> Casey Schaufler wrote:
> > Yes, it would make it nicer. You'll need to do a better job
> > on the list management than I've been doing. It's probably well
> > past time to introduce the Standard list management scheme to
> > Smack, and you'll need to do so if you want to do insertions
> > and/or deletions.
>
> well, we could maybe do that for smack_netlbladdrs.
> for smk_rules, i don't know, depending to the use case, it could grow
> bigger and thus need a more efficient scheme than linked-list like
> hash-table.

The code is easily changed because it is private to Smack and we don't have to 
worry about backwards compatibility issues.  I would focus on improving the 
linked list approach (masked, sorted, etc.) and when traversing the list 
becomes a bottleneck we can look at alternative approaches.  Others may have a 
better view, but from what I've seen the general approach taken in Linux 
Kernel optimization is to develop something that works then refine and 
optimize it once you have a better understanding of the common use cases.  

-- 
paul moore
linux @ hp

Re: [PATCH] SMACK netfilter smacklabel socket match

[Casey Schaufler]

etienne wrote:
> ...
>> Etienne, thank you very much for the work you've done so far. Paul,
>> thank you for your recommendations.
>>     
>
> well, I'll try to explain my use case for SMACK, could you please tell me if this makes sense and if it is doable and sane with SMACK :
>
> I have single-user computer that, for simplicity sake, do only web browsing with firefox; 
> the attack vector i'm concerned with is malicious web pages, that could execute malicious code on my computer or worse erase some of my data; 
>
> so i express the following security policy in a tool-agnostic way :
> 1. firefox can access internet
>   

In Smack terms then you want the process label of your browser
process to have access to hosts on the internet in general. The
easy way to do this is for it to run with the ambient label
(cat /smack/ambient to see it) which will be the floor label "_"
unless you change it. Note that your browser will need to talk
to the X11 server as well, so processes with the label of the
browser will need write access to processes with the label of the
X11 server, and visa versa.

> 2. firefox can read/write it's configuration directory in my $HOME
>   

The label of the process will have to match the label of that
directory for this to work.

> 3. firefox can read/write to a download directory
>   

Again, the label will have to match.

> 4. firefox can execute kpdf, okular, vlc etc...
>   

All these files will have the floor label "_" by design, so this
is easy,
> 5. firefox can read system files
>   

Again, system files will have the floor label, so this is easy.

> 6. firefox can write to temporary folder
>   

Do you need to use /tmp, or does firefox respect $TMPDIR?
You can set the label of /tmp to the star "*" label if worse
comes to worst.

> pretty simple. So I expect the 'tool' to express this policy in very few line; (i had a look at selinux/refpolicy, and I'm ashamed I was too lazy to test/understand further).

Don't be ashamed. I wrote Smack because I was too lazy to figure
out SELinux policy.

>  And if possible a mainline tool would be a big bonus.
>
> So I decided to give  smack  a try, and here are my notes/interrogations :
>
> rule 1. if i understand correctly, I have to load the following smack rule  
> "firefox _ rwx" 
> well, as '_' is the default objectlabel for all system files, it means that firefox will have smack 'w' access on system. 
>
> So first issue : is it possible to express network access in another way?
> Or maybe I have to relabel  /bin/, /sbin etc with a custom system label ?
>   

If you want firefox to talk on the internet, and have
no other processes talk on the internet including the X11 server,
you need to run firefox with a different label from everything
else. This will make it difficult to talk to the X11 server.

> rule 2-6 : easy to implement with smack, i label my $HOME with some label and download/cfg dir with other labels
> Firefox won't have rw access to my $HOME hehe
>
> Second issue : what is the simplest way to start firefox with the firefox label?
> I used the following hack : write a small program (i used cap_mac_admin, could have been suid) that :
> a) set /proc/self/attr/current
> b) drop capabilities
> c) start firefox
> Is there a cleanest way, can a process be started with its objectlabel?
>
>   

I have a newsmack program, but all that it does is what your "hack"
does.

> Third issue : there seems to be  no way to log/audit access violations, have you plans to implement that?
>   

Hmm. Audit should be working.

> best regards,
> Etienne
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>   

Re: [PATCH] SMACK netfilter smacklabel socket match

[etienne]

Casey Schaufler wrote:
> etienne wrote:
>> ...
>>> Etienne, thank you very much for the work you've done so far. Paul,
>>> thank you for your recommendations.
>>>     
>> well, I'll try to explain my use case for SMACK, could you please tell me if this makes sense and if it is doable and sane with SMACK :
>>
>> I have single-user computer that, for simplicity sake, do only web browsing with firefox; 
>> the attack vector i'm concerned with is malicious web pages, that could execute malicious code on my computer or worse erase some of my data; 
>>
>> so i express the following security policy in a tool-agnostic way :
>> 1. firefox can access internet
>>   
> 
> In Smack terms then you want the process label of your browser
> process to have access to hosts on the internet in general. The
> easy way to do this is for it to run with the ambient label
> (cat /smack/ambient to see it) which will be the floor label "_"
> unless you change it. Note that your browser will need to talk
> to the X11 server as well, so processes with the label of the
> browser will need write access to processes with the label of the
> X11 server, and visa versa.

OK
> 
>> 2. firefox can read/write it's configuration directory in my $HOME
>>   
[snip]
> 
> Do you need to use /tmp, or does firefox respect $TMPDIR?
> You can set the label of /tmp to the star "*" label if worse
> comes to worst.
>
i don't really know now, i label /tmp/ /var/tmp with *

 
>> pretty simple. So I expect the 'tool' to express this policy in very few line; (i had a look at selinux/refpolicy, and I'm ashamed I was too lazy to test/understand further).
> 
> Don't be ashamed. I wrote Smack because I was too lazy to figure
> out SELinux policy.
> 
:-)

> 
> I have a newsmack program, but all that it does is what your "hack"
> does.
>
OK then. If it's the only way
 
>> Third issue : there seems to be  no way to log/audit access violations, have you plans to implement that?
>>   
> 
> Hmm. Audit should be working.
> 
I see some "audit" hook in the  code, but i don't see a way to log _specific_ smack information ie  
"smack_subject  smack_object  smack_access drop"       (+of course process name, pid, path, and any relevant info)

like selinux would do in 'avc_audit' 

 
regards
Etienne

Re: [PATCH] SMACK netfilter smacklabel socket match

[etienne]

hello,

Paul Moore wrote:
..
> Well, since you have some time and willingness to do things "the right way" I 
> would recommend dropping these patches (which are really just band-aids) and 
> working on the right solution to stored the addresses/masks in a sorted list 
> with the mask already applied.
> 
OK, I'm about to send a new patch; but while testing my patches and reading code, I noticed another bug : 

In smackfs.c:smk_write_netlbladdr
the netmask mask.s_addr is not handled correctly, the netmask should be :
1- computed in u32
2- converted to be32 !!
with current code, a "pseudo u32 mask" is applied to a be32 ipaddr; it occurs to works for "common netmasks" (multiple of 8), not for "intermediate" mask (/15, /25)


> FWIW, the NetLabel code (net/netlabel) has to do very similar things with 
> sorted address lists so I built an address list construct which builds on the 
> list.h ideas and operates in a similar way.  You may find it helpful.
> 
OK, I tested some code in userspace and when i was confident enough coded it to kernel

>> I think this should go to stable too?
> 
> I would worry about getting the patches developed, tested and in an acceptable 
> form first, then we can worry about where they should be applied ;)
> 

OK :)

Re: [PATCH] SMACK netfilter smacklabel socket match

[Casey Schaufler]

etienne wrote:
> hello,
>
> Paul Moore wrote:
> ..
>   
>> Well, since you have some time and willingness to do things "the right way" I 
>> would recommend dropping these patches (which are really just band-aids) and 
>> working on the right solution to stored the addresses/masks in a sorted list 
>> with the mask already applied.
>>
>>     
> OK, I'm about to send a new patch; but while testing my patches and reading code, I noticed another bug : 
>
> In smackfs.c:smk_write_netlbladdr
> the netmask mask.s_addr is not handled correctly, the netmask should be :
> 1- computed in u32
> 2- converted to be32 !!
> with current code, a "pseudo u32 mask" is applied to a be32 ipaddr; it occurs to works for "common netmasks" (multiple of 8), not for "intermediate" mask (/15, /25)
>   

Well, that's embarrassing. I am really looking forward to your
fixes!

[PATCH] SMACK netlabel fixes

[etienne]

Hello,

the following patch (against 2.6.29rc5) fixes a few issues in the smack/netlabel area :
1) smack_host_label disregard a "0.0.0.0/0 @" rule (or other label), preventing 'tagged' tasks to access Internet (many systems drop packets with IP options)
2) netmasks were not handled correctly, they were stored in a way _not equivalent_  to conversion to be32 (it was equivalent for /0, /8, /16, /24, /32 masks but not other masks)
3) smack_netlbladdr prefixes (IP/mask) were not consistent (mask&IP was not done), so there could have been different list entries for the same IP prefix; if those entries had different labels, well ... 
4) they were not sorted 

1) 2) 3) are bugs, 4) is a more cosmetic issue.
The patch :
-creates a new helper smk_netlbladdr_insert to insert a smk_netlbladdr, sorted by netmask length
-use the new sorted nature of  smack_netlbladdrs list to simplify smack_host_label : 
 the first match _will_ be the more specific
-corrects endianness issues in smk_write_netlbladdr &  netlbladdr_seq_show

The patch are "tested" so that they no crash the system; cat /smack/netlabel is now sorted and always consistent.
Some basics ping tests to '@' and other label combination seems ok 
See   an extract of my tests bellow the patch 

regards,
Etienne

Signed-off-by: <etienne.basset@numericable.fr>
---
 security/smack/smack_lsm.c |   38 ++++++----------------------
 security/smack/smackfs.c   |   60 +++++++++++++++++++++++++++++++++----------
 2 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0278bc0..427595e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1498,10 +1498,7 @@ static int smack_socket_post_create(struct socket *sock, int family,
  * looks for host based access restrictions
  *
  * This version will only be appropriate for really small
- * sets of single label hosts. Because of the masking
- * it cannot shortcut out on the first match. There are
- * numerious ways to address the problem, but none of them
- * have been applied here.
+ * sets of single label hosts.
  *
  * Returns the label of the far end or NULL if it's not special.
  */
@@ -1512,41 +1509,22 @@ static char *smack_host_label(struct sockaddr_in *sip)
 	struct in_addr *siap = &sip->sin_addr;
 	struct in_addr *liap;
 	struct in_addr *miap;
-	struct in_addr bestmask;
 
 	if (siap->s_addr == 0)
 		return NULL;
 
-	bestmask.s_addr = 0;
-
 	for (snp = smack_netlbladdrs; snp != NULL; snp = snp->smk_next) {
 		liap = &snp->smk_host.sin_addr;
 		miap = &snp->smk_mask;
 		/*
-		 * If the addresses match after applying the list entry mask
-		 * the entry matches the address. If it doesn't move along to
-		 * the next entry.
-		 */
-		if ((liap->s_addr & miap->s_addr) !=
-		    (siap->s_addr & miap->s_addr))
-			continue;
-		/*
-		 * If the list entry mask identifies a single address
-		 * it can't get any more specific.
-		 */
-		if (miap->s_addr == 0xffffffff)
-			return snp->smk_label;
-		/*
-		 * If the list entry mask is less specific than the best
-		 * already found this entry is uninteresting.
+		 * we break after finding the first match because
+		 * the list is sorted from longest to shortest mask
+		 * so we have found the most specific match
 		 */
-		if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
-			continue;
-		/*
-		 * This is better than any entry found so far.
-		 */
-		bestmask.s_addr = miap->s_addr;
-		bestlabel = snp->smk_label;
+		if (liap->s_addr  == (siap->s_addr & miap->s_addr)) {
+			bestlabel = snp->smk_label;
+			break;
+		}
 	}
 
 	return bestlabel;
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 8e42800..4d4332b 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -650,10 +650,6 @@ static void *netlbladdr_seq_next(struct seq_file *s, void *v, loff_t *pos)
 
 	return skp;
 }
-/*
-#define BEMASK	0x80000000
-*/
-#define BEMASK	0x00000001
 #define BEBITS	(sizeof(__be32) * 8)
 
 /*
@@ -663,12 +659,10 @@ static int netlbladdr_seq_show(struct seq_file *s, void *v)
 {
 	struct smk_netlbladdr *skp = (struct smk_netlbladdr *) v;
 	unsigned char *hp = (char *) &skp->smk_host.sin_addr.s_addr;
-	__be32 bebits;
 	int maskn = 0;
+	u32 temp_mask = be32_to_cpu(skp->smk_mask.s_addr);
 
-	for (bebits = BEMASK; bebits != 0; maskn++, bebits <<= 1)
-		if ((skp->smk_mask.s_addr & bebits) == 0)
-			break;
+	for ( ; temp_mask; temp_mask <<= 1, maskn++);
 
 	seq_printf(s, "%u.%u.%u.%u/%d %s\n",
 		hp[0], hp[1], hp[2], hp[3], maskn, skp->smk_label);
@@ -702,6 +696,40 @@ static int smk_open_netlbladdr(struct inode *inode, struct file *file)
 }
 
 /**
+ * smk_netlbladdr_insert
+ * @new : netlabel to insert
+ *
+ * This helper insert netlabel in the smack_netlbladdrs list
+ * sorted by netmask length (longest to smallest)
+ */
+static void smk_netlbladdr_insert(struct smk_netlbladdr *new)
+{
+	struct smk_netlbladdr *m;
+	if (smack_netlbladdrs == NULL) {
+		smack_netlbladdrs = new;
+		return;
+	}
+	/** the comparison '>' is a bit hacky, but works */
+	if (new->smk_mask.s_addr > smack_netlbladdrs->smk_mask.s_addr) {
+		new->smk_next = smack_netlbladdrs;
+		smack_netlbladdrs = new;
+		return;
+	}
+	for (m = smack_netlbladdrs; m != NULL; m = m->smk_next) {
+		if (m->smk_next == NULL) {
+			m->smk_next = new;
+			return;
+		}
+		if (new->smk_mask.s_addr > m->smk_next->smk_mask.s_addr) {
+			new->smk_next = m->smk_next;
+			m->smk_next = new;
+			return;
+		}
+	}
+}
+
+
+/**
  * smk_write_netlbladdr - write() for /smack/netlabel
  * @filp: file pointer, not actually used
  * @buf: where to get the data from
@@ -724,8 +752,9 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 	struct netlbl_audit audit_info;
 	struct in_addr mask;
 	unsigned int m;
-	__be32 bebits = BEMASK;
+	u32 mask_bits = (1<<31);
 	__be32 nsa;
+	u32 temp_mask;
 
 	/*
 	 * Must have privilege.
@@ -761,10 +790,13 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 	if (sp == NULL)
 		return -EINVAL;
 
-	for (mask.s_addr = 0; m > 0; m--) {
-		mask.s_addr |= bebits;
-		bebits <<= 1;
+	for (temp_mask = 0; m > 0; m--) {
+		temp_mask |= mask_bits;
+		mask_bits >>= 1;
 	}
+	mask.s_addr = cpu_to_be32(temp_mask);
+
+	newname.sin_addr.s_addr &= mask.s_addr;
 	/*
 	 * Only allow one writer at a time. Writes should be
 	 * quite rare and small in any case.
@@ -772,6 +804,7 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 	mutex_lock(&smk_netlbladdr_lock);
 
 	nsa = newname.sin_addr.s_addr;
+	/* try to find if the prefix is already in the list */
 	for (skp = smack_netlbladdrs; skp != NULL; skp = skp->smk_next)
 		if (skp->smk_host.sin_addr.s_addr == nsa &&
 		    skp->smk_mask.s_addr == mask.s_addr)
@@ -787,9 +820,8 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 			rc = 0;
 			skp->smk_host.sin_addr.s_addr = newname.sin_addr.s_addr;
 			skp->smk_mask.s_addr = mask.s_addr;
-			skp->smk_next = smack_netlbladdrs;
 			skp->smk_label = sp;
-			smack_netlbladdrs = skp;
+			smk_netlbladdr_insert(skp);
 		}
 	} else {
 		rc = netlbl_cfg_unlbl_static_del(&init_net, NULL,


=============================================================
TESTS
=============================================================

root@etienne-desktop:/home/etienne/linux-2.6# cat /smack/netlabel
212.180.1.0/26 toto
212.180.1.0/25 @
217.146.186.0/24 _
212.180.0.0/23 tit
212.180.0.0/15 @

root@etienne-desktop:/home/etienne/linux-2.6# cat /proc/self/attr/current
_root@etienne-desktop:/home/etienne/linux-2.6#
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.1.65 -c 1
PING 212.180.1.65 (212.180.1.65) 56(84) bytes of data.
64 bytes from 212.180.1.65: icmp_seq=1 ttl=56 time=7.04 ms

--- 212.180.1.65 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 7.040/7.040/7.040/0.000 ms
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.1.1 -c 1
Do you want to ping broadcast? Then -b
root@etienne-desktop:/home/etienne/linux-2.6# echo toto > /proc/self/attr/current
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.1.1 -c 1
PING 212.180.1.1 (212.180.1.1) 56(84) bytes of data.
64 bytes from 212.180.1.1: icmp_seq=1 ttl=248 time=13.6 ms

--- 212.180.1.1 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 13.632/13.632/13.632/0.000 ms
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.1.65 -c 1
PING 212.180.1.65 (212.180.1.65) 56(84) bytes of data.
64 bytes from 212.180.1.65: icmp_seq=1 ttl=56 time=9.87 ms

--- 212.180.1.65 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 9.876/9.876/9.876/0.000 ms
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.0.1 -c 1
Do you want to ping broadcast? Then -b
root@etienne-desktop:/home/etienne/linux-2.6# echo titi > /proc/self/attr/current
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.0.1 -c 1
Do you want to ping broadcast? Then -b
root@etienne-desktop:/home/etienne/linux-2.6# echo tit > /proc/self/attr/current
root@etienne-desktop:/home/etienne/linux-2.6# ping 212.180.0.1 -c 1
PING 212.180.0.1 (212.180.0.1) 56(84) bytes of data.
^C
--- 212.180.0.1 ping statistics ---
1 packets transmitted, 0 received, 100% packet loss, time 0ms




  

Re: [PATCH] SMACK netlabel fixes

[Casey Schaufler]

etienne wrote:
> Hello,
>
> the following patch (against 2.6.29rc5) fixes a few issues in the smack/netlabel area :
> 1) smack_host_label disregard a "0.0.0.0/0 @" rule (or other label), preventing 'tagged' tasks to access Internet (many systems drop packets with IP options)
> 2) netmasks were not handled correctly, they were stored in a way _not equivalent_  to conversion to be32 (it was equivalent for /0, /8, /16, /24, /32 masks but not other masks)
> 3) smack_netlbladdr prefixes (IP/mask) were not consistent (mask&IP was not done), so there could have been different list entries for the same IP prefix; if those entries had different labels, well ... 
> 4) they were not sorted 
>
> 1) 2) 3) are bugs, 4) is a more cosmetic issue.
> The patch :
> -creates a new helper smk_netlbladdr_insert to insert a smk_netlbladdr, sorted by netmask length
> -use the new sorted nature of  smack_netlbladdrs list to simplify smack_host_label : 
>  the first match _will_ be the more specific
> -corrects endianness issues in smk_write_netlbladdr &  netlbladdr_seq_show
>
> The patch are "tested" so that they no crash the system; cat /smack/netlabel is now sorted and always consistent.
> Some basics ping tests to '@' and other label combination seems ok 
> See   an extract of my tests bellow the patch 
>
> regards,
> Etienne
>   

I am in the process of configuring the Smack test lab so that I
can bang on this a little. It looks good so far. Thank you.

Re: [PATCH] SMACK netlabel fixes

[Paul Moore]

On Wednesday 18 February 2009 04:16:08 pm etienne wrote:
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0278bc0..427595e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1498,10 +1498,7 @@ static int smack_socket_post_create(struct socket
> *sock, int family, * looks for host based access restrictions
>   *
>   * This version will only be appropriate for really small
> - * sets of single label hosts. Because of the masking
> - * it cannot shortcut out on the first match. There are
> - * numerious ways to address the problem, but none of them
> - * have been applied here.
> + * sets of single label hosts.
>   *
>   * Returns the label of the far end or NULL if it's not special.
>   */
> @@ -1512,41 +1509,22 @@ static char *smack_host_label(struct sockaddr_in
> *sip) struct in_addr *siap = &sip->sin_addr;
>  	struct in_addr *liap;
>  	struct in_addr *miap;
> -	struct in_addr bestmask;
>
>  	if (siap->s_addr == 0)
>  		return NULL;
>
> -	bestmask.s_addr = 0;
> -
>  	for (snp = smack_netlbladdrs; snp != NULL; snp = snp->smk_next) {
>  		liap = &snp->smk_host.sin_addr;
>  		miap = &snp->smk_mask;

Unless I'm mistaken the 'liap' and 'miap' variables are only used once inside 
the for loop, why not remove them and simply reference 'snp' directly?

>  		/*
> -		 * If the addresses match after applying the list entry mask
> -		 * the entry matches the address. If it doesn't move along to
> -		 * the next entry.
> -		 */
> -		if ((liap->s_addr & miap->s_addr) !=
> -		    (siap->s_addr & miap->s_addr))
> -			continue;
> -		/*
> -		 * If the list entry mask identifies a single address
> -		 * it can't get any more specific.
> -		 */
> -		if (miap->s_addr == 0xffffffff)
> -			return snp->smk_label;
> -		/*
> -		 * If the list entry mask is less specific than the best
> -		 * already found this entry is uninteresting.
> +		 * we break after finding the first match because
> +		 * the list is sorted from longest to shortest mask
> +		 * so we have found the most specific match
>  		 */
> -		if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
> -			continue;
> -		/*
> -		 * This is better than any entry found so far.
> -		 */
> -		bestmask.s_addr = miap->s_addr;
> -		bestlabel = snp->smk_label;
> +		if (liap->s_addr  == (siap->s_addr & miap->s_addr)) {
> +			bestlabel = snp->smk_label;
> +			break;

This is being a bit nit-picky, but why not get rid of 'bestlabel' completely 
and instead of breaking here simply reutn 'snp->smk_label'?  If we ever reach 
the end of the function (no match) just return NULL.

> +		}
>  	}
>
>  	return bestlabel;

...

> @@ -663,12 +659,10 @@ static int netlbladdr_seq_show(struct seq_file *s,
> void *v) {
>  	struct smk_netlbladdr *skp = (struct smk_netlbladdr *) v;
>  	unsigned char *hp = (char *) &skp->smk_host.sin_addr.s_addr;
> -	__be32 bebits;
>  	int maskn = 0;
> +	u32 temp_mask = be32_to_cpu(skp->smk_mask.s_addr);
>
> -	for (bebits = BEMASK; bebits != 0; maskn++, bebits <<= 1)
> -		if ((skp->smk_mask.s_addr & bebits) == 0)
> -			break;
> +	for ( ; temp_mask; temp_mask <<= 1, maskn++);

More nit-picky stuff :)  Why not set 'maskn' to zero inside the for loop 
construct instead of at the top of the function?  There is less chance for 
error this way if someone else touches the code.

>  	seq_printf(s, "%u.%u.%u.%u/%d %s\n",
>  		hp[0], hp[1], hp[2], hp[3], maskn, skp->smk_label);

...

> @@ -702,6 +696,40 @@ static int smk_open_netlbladdr(struct inode *inode,
> struct file *file) }
>
>  /**
> + * smk_netlbladdr_insert
> + * @new : netlabel to insert
> + *
> + * This helper insert netlabel in the smack_netlbladdrs list
> + * sorted by netmask length (longest to smallest)
> + */
> +static void smk_netlbladdr_insert(struct smk_netlbladdr *new)
> +{
> +	struct smk_netlbladdr *m;

An empty line here might be nice.

> +	if (smack_netlbladdrs == NULL) {
> +		smack_netlbladdrs = new;
> +		return;
> +	}

I would prefer another one here, if that is okay with you.

> +	/** the comparison '>' is a bit hacky, but works */

Why start the comment with '/**', using a single '*' works just fine.

> +	if (new->smk_mask.s_addr > smack_netlbladdrs->smk_mask.s_addr) {
> +		new->smk_next = smack_netlbladdrs;
> +		smack_netlbladdrs = new;
> +		return;
> +	}
> +	for (m = smack_netlbladdrs; m != NULL; m = m->smk_next) {
> +		if (m->smk_next == NULL) {
> +			m->smk_next = new;
> +			return;
> +		}
> +		if (new->smk_mask.s_addr > m->smk_next->smk_mask.s_addr) {
> +			new->smk_next = m->smk_next;
> +			m->smk_next = new;
> +			return;
> +		}
> +	}

As Casey mentioned earlier (and has been brough up in the past), you should 
heavily consider using the list.h constructs, it would make life much easier 
here and elsewhere.

Bonus points if you convert the other Smack lists to using the list.h bits.

> +}

...

> +/**
>   * smk_write_netlbladdr - write() for /smack/netlabel
>   * @filp: file pointer, not actually used
>   * @buf: where to get the data from
> @@ -724,8 +752,9 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, struct netlbl_audit audit_info;
>  	struct in_addr mask;
>  	unsigned int m;
> -	__be32 bebits = BEMASK;
> +	u32 mask_bits = (1<<31);

Why not just enter the value directly here?  It would be much clearer I think.

-- 
paul moore
linux @ hp

[PATCH] SMACK netlabel fixes v2

[etienne]

Hi Paul,

thanks for your comments, please find below an updated version of the patch, I hope it is fine with you and Casey.
(i didnt remove the 'mask_bits = (1<<31)' of your last comment :
=> i think it's much obvious that it represent binary 1000..00 than 0x80000000, but that may be just me :)

I redid some basics tests, it seems i've not messed things too much :)

For the moving to standard list, I guess I could try to do that; (even other smack list, yes want my bonus points! ;) )
After a quick list.h grep i think i'll have to implement a 'sorted insert' anyway. 
But please allow me to do that on top of my patch


regards
Etienne

--

the following patch (against 2.6.29rc5) fixes a few issues in the smack/netlabel area :
1) smack_host_label disregard a "0.0.0.0/0 @" rule (or other label), preventing 'tagged' tasks to access Internet (many systems drop packets with IP options)
2) netmasks were not handled correctly, they were stored in a way _not equivalent_  to conversion to be32 (it was equivalent for /0, /8, /16, /24, /32 masks but not other masks)
3) smack_netlbladdr prefixes (IP/mask) were not consistent (mask&IP was not done), so there could have been different list entries for the same IP prefix; if those entries had different labels, well ... 
4) they were not sorted 

1) 2) 3) are bugs, 4) is a more cosmetic issue.
The patch :
-creates a new helper smk_netlbladdr_insert to insert a smk_netlbladdr, sorted by netmask length
-use the new sorted nature of  smack_netlbladdrs list to simplify smack_host_label : 
 the first match _will_ be the more specific
-corrects endianness issues in smk_write_netlbladdr &  netlbladdr_seq_show

The patch are "tested" so that they no crash the system; cat /smack/netlabel is now sorted and always consistent.
Some basics ping tests to '@' and other label combination seems ok 
See   an extract of my tests bellow the patch 

regards,
Etienne

Signed-off-by: <etienne.basset@numericable.fr>
---


security/smack/smack_lsm.c |   43 +++++------------------------
 security/smack/smackfs.c   |   64 +++++++++++++++++++++++++++++++++----------
 2 files changed, 57 insertions(+), 50 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0278bc0..e7ded13 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1498,58 +1498,31 @@ static int smack_socket_post_create(struct socket *sock, int family,
  * looks for host based access restrictions
  *
  * This version will only be appropriate for really small
- * sets of single label hosts. Because of the masking
- * it cannot shortcut out on the first match. There are
- * numerious ways to address the problem, but none of them
- * have been applied here.
+ * sets of single label hosts.
  *
  * Returns the label of the far end or NULL if it's not special.
  */
 static char *smack_host_label(struct sockaddr_in *sip)
 {
 	struct smk_netlbladdr *snp;
-	char *bestlabel = NULL;
 	struct in_addr *siap = &sip->sin_addr;
-	struct in_addr *liap;
-	struct in_addr *miap;
-	struct in_addr bestmask;
 
 	if (siap->s_addr == 0)
 		return NULL;
 
-	bestmask.s_addr = 0;
-
 	for (snp = smack_netlbladdrs; snp != NULL; snp = snp->smk_next) {
-		liap = &snp->smk_host.sin_addr;
-		miap = &snp->smk_mask;
-		/*
-		 * If the addresses match after applying the list entry mask
-		 * the entry matches the address. If it doesn't move along to
-		 * the next entry.
-		 */
-		if ((liap->s_addr & miap->s_addr) !=
-		    (siap->s_addr & miap->s_addr))
-			continue;
 		/*
-		 * If the list entry mask identifies a single address
-		 * it can't get any more specific.
+		 * we break after finding the first match because
+		 * the list is sorted from longest to shortest mask
+		 * so we have found the most specific match
 		 */
-		if (miap->s_addr == 0xffffffff)
+		if ((&snp->smk_host.sin_addr)->s_addr  ==
+			(siap->s_addr & (&snp->smk_mask)->s_addr)) {
 			return snp->smk_label;
-		/*
-		 * If the list entry mask is less specific than the best
-		 * already found this entry is uninteresting.
-		 */
-		if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
-			continue;
-		/*
-		 * This is better than any entry found so far.
-		 */
-		bestmask.s_addr = miap->s_addr;
-		bestlabel = snp->smk_label;
+		}
 	}
 
-	return bestlabel;
+	return NULL;
 }
 
 /**
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 8e42800..51f0efc 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -650,10 +650,6 @@ static void *netlbladdr_seq_next(struct seq_file *s, void *v, loff_t *pos)
 
 	return skp;
 }
-/*
-#define BEMASK	0x80000000
-*/
-#define BEMASK	0x00000001
 #define BEBITS	(sizeof(__be32) * 8)
 
 /*
@@ -663,12 +659,10 @@ static int netlbladdr_seq_show(struct seq_file *s, void *v)
 {
 	struct smk_netlbladdr *skp = (struct smk_netlbladdr *) v;
 	unsigned char *hp = (char *) &skp->smk_host.sin_addr.s_addr;
-	__be32 bebits;
-	int maskn = 0;
+	int maskn;
+	u32 temp_mask = be32_to_cpu(skp->smk_mask.s_addr);
 
-	for (bebits = BEMASK; bebits != 0; maskn++, bebits <<= 1)
-		if ((skp->smk_mask.s_addr & bebits) == 0)
-			break;
+	for (maskn = 0; temp_mask; temp_mask <<= 1, maskn++);
 
 	seq_printf(s, "%u.%u.%u.%u/%d %s\n",
 		hp[0], hp[1], hp[2], hp[3], maskn, skp->smk_label);
@@ -702,6 +696,42 @@ static int smk_open_netlbladdr(struct inode *inode, struct file *file)
 }
 
 /**
+ * smk_netlbladdr_insert
+ * @new : netlabel to insert
+ *
+ * This helper insert netlabel in the smack_netlbladdrs list
+ * sorted by netmask length (longest to smallest)
+ */
+static void smk_netlbladdr_insert(struct smk_netlbladdr *new)
+{
+	struct smk_netlbladdr *m;
+
+	if (smack_netlbladdrs == NULL) {
+		smack_netlbladdrs = new;
+		return;
+	}
+
+	/* the comparison '>' is a bit hacky, but works */
+	if (new->smk_mask.s_addr > smack_netlbladdrs->smk_mask.s_addr) {
+		new->smk_next = smack_netlbladdrs;
+		smack_netlbladdrs = new;
+		return;
+	}
+	for (m = smack_netlbladdrs; m != NULL; m = m->smk_next) {
+		if (m->smk_next == NULL) {
+			m->smk_next = new;
+			return;
+		}
+		if (new->smk_mask.s_addr > m->smk_next->smk_mask.s_addr) {
+			new->smk_next = m->smk_next;
+			m->smk_next = new;
+			return;
+		}
+	}
+}
+
+
+/**
  * smk_write_netlbladdr - write() for /smack/netlabel
  * @filp: file pointer, not actually used
  * @buf: where to get the data from
@@ -724,8 +754,9 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 	struct netlbl_audit audit_info;
 	struct in_addr mask;
 	unsigned int m;
-	__be32 bebits = BEMASK;
+	u32 mask_bits = (1<<31);
 	__be32 nsa;
+	u32 temp_mask;
 
 	/*
 	 * Must have privilege.
@@ -761,10 +792,13 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 	if (sp == NULL)
 		return -EINVAL;
 
-	for (mask.s_addr = 0; m > 0; m--) {
-		mask.s_addr |= bebits;
-		bebits <<= 1;
+	for (temp_mask = 0; m > 0; m--) {
+		temp_mask |= mask_bits;
+		mask_bits >>= 1;
 	}
+	mask.s_addr = cpu_to_be32(temp_mask);
+
+	newname.sin_addr.s_addr &= mask.s_addr;
 	/*
 	 * Only allow one writer at a time. Writes should be
 	 * quite rare and small in any case.
@@ -772,6 +806,7 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 	mutex_lock(&smk_netlbladdr_lock);
 
 	nsa = newname.sin_addr.s_addr;
+	/* try to find if the prefix is already in the list */
 	for (skp = smack_netlbladdrs; skp != NULL; skp = skp->smk_next)
 		if (skp->smk_host.sin_addr.s_addr == nsa &&
 		    skp->smk_mask.s_addr == mask.s_addr)
@@ -787,9 +822,8 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
 			rc = 0;
 			skp->smk_host.sin_addr.s_addr = newname.sin_addr.s_addr;
 			skp->smk_mask.s_addr = mask.s_addr;
-			skp->smk_next = smack_netlbladdrs;
 			skp->smk_label = sp;
-			smack_netlbladdrs = skp;
+			smk_netlbladdr_insert(skp);
 		}
 	} else {
 		rc = netlbl_cfg_unlbl_static_del(&init_net, NULL,

Re: [PATCH] SMACK netlabel fixes v2

[Paul Moore]

On Thursday 19 February 2009 06:22:11 pm etienne wrote:
> Hi Paul,
>
> thanks for your comments, please find below an updated version of the
> patch, I hope it is fine with you and Casey. (i didnt remove the 'mask_bits
> = (1<<31)' of your last comment :
> => i think it's much obvious that it represent binary 1000..00 than
> 0x80000000, but that may be just me :)

Fair enough, it was a pretty nit-picky comment :)

> I redid some basics tests, it seems i've not messed things too much :)
>
> For the moving to standard list, I guess I could try to do that; (even
> other smack list, yes want my bonus points! ;) ) After a quick list.h grep
> i think i'll have to implement a 'sorted insert' anyway. But please allow
> me to do that on top of my patch

That is fine, the patch below is still a good improvement, no need to hold it 
up for the list conversion.

Reviewed-by: Paul Moore <paul.moore@hp.com>

> --
>
> the following patch (against 2.6.29rc5) fixes a few issues in the
> smack/netlabel area : 1) smack_host_label disregard a "0.0.0.0/0 @" rule
> (or other label), preventing 'tagged' tasks to access Internet (many
> systems drop packets with IP options) 2) netmasks were not handled
> correctly, they were stored in a way _not equivalent_  to conversion to
> be32 (it was equivalent for /0, /8, /16, /24, /32 masks but not other
> masks) 3) smack_netlbladdr prefixes (IP/mask) were not consistent (mask&IP
> was not done), so there could have been different list entries for the same
> IP prefix; if those entries had different labels, well ... 4) they were not
> sorted
>
> 1) 2) 3) are bugs, 4) is a more cosmetic issue.
> The patch :
> -creates a new helper smk_netlbladdr_insert to insert a smk_netlbladdr,
> sorted by netmask length -use the new sorted nature of  smack_netlbladdrs
> list to simplify smack_host_label : the first match _will_ be the more
> specific
> -corrects endianness issues in smk_write_netlbladdr &  netlbladdr_seq_show
>
> The patch are "tested" so that they no crash the system; cat
> /smack/netlabel is now sorted and always consistent. Some basics ping tests
> to '@' and other label combination seems ok See   an extract of my tests
> bellow the patch
>
> regards,
> Etienne
>
> Signed-off-by: <etienne.basset@numericable.fr>
> ---
>
>
> security/smack/smack_lsm.c |   43 +++++------------------------
>  security/smack/smackfs.c   |   64
> +++++++++++++++++++++++++++++++++---------- 2 files changed, 57
> insertions(+), 50 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0278bc0..e7ded13 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1498,58 +1498,31 @@ static int smack_socket_post_create(struct socket
> *sock, int family, * looks for host based access restrictions
>   *
>   * This version will only be appropriate for really small
> - * sets of single label hosts. Because of the masking
> - * it cannot shortcut out on the first match. There are
> - * numerious ways to address the problem, but none of them
> - * have been applied here.
> + * sets of single label hosts.
>   *
>   * Returns the label of the far end or NULL if it's not special.
>   */
>  static char *smack_host_label(struct sockaddr_in *sip)
>  {
>  	struct smk_netlbladdr *snp;
> -	char *bestlabel = NULL;
>  	struct in_addr *siap = &sip->sin_addr;
> -	struct in_addr *liap;
> -	struct in_addr *miap;
> -	struct in_addr bestmask;
>
>  	if (siap->s_addr == 0)
>  		return NULL;
>
> -	bestmask.s_addr = 0;
> -
>  	for (snp = smack_netlbladdrs; snp != NULL; snp = snp->smk_next) {
> -		liap = &snp->smk_host.sin_addr;
> -		miap = &snp->smk_mask;
> -		/*
> -		 * If the addresses match after applying the list entry mask
> -		 * the entry matches the address. If it doesn't move along to
> -		 * the next entry.
> -		 */
> -		if ((liap->s_addr & miap->s_addr) !=
> -		    (siap->s_addr & miap->s_addr))
> -			continue;
>  		/*
> -		 * If the list entry mask identifies a single address
> -		 * it can't get any more specific.
> +		 * we break after finding the first match because
> +		 * the list is sorted from longest to shortest mask
> +		 * so we have found the most specific match
>  		 */
> -		if (miap->s_addr == 0xffffffff)
> +		if ((&snp->smk_host.sin_addr)->s_addr  ==
> +			(siap->s_addr & (&snp->smk_mask)->s_addr)) {
>  			return snp->smk_label;
> -		/*
> -		 * If the list entry mask is less specific than the best
> -		 * already found this entry is uninteresting.
> -		 */
> -		if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
> -			continue;
> -		/*
> -		 * This is better than any entry found so far.
> -		 */
> -		bestmask.s_addr = miap->s_addr;
> -		bestlabel = snp->smk_label;
> +		}
>  	}
>
> -	return bestlabel;
> +	return NULL;
>  }
>
>  /**
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 8e42800..51f0efc 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -650,10 +650,6 @@ static void *netlbladdr_seq_next(struct seq_file *s,
> void *v, loff_t *pos)
>
>  	return skp;
>  }
> -/*
> -#define BEMASK	0x80000000
> -*/
> -#define BEMASK	0x00000001
>  #define BEBITS	(sizeof(__be32) * 8)
>
>  /*
> @@ -663,12 +659,10 @@ static int netlbladdr_seq_show(struct seq_file *s,
> void *v) {
>  	struct smk_netlbladdr *skp = (struct smk_netlbladdr *) v;
>  	unsigned char *hp = (char *) &skp->smk_host.sin_addr.s_addr;
> -	__be32 bebits;
> -	int maskn = 0;
> +	int maskn;
> +	u32 temp_mask = be32_to_cpu(skp->smk_mask.s_addr);
>
> -	for (bebits = BEMASK; bebits != 0; maskn++, bebits <<= 1)
> -		if ((skp->smk_mask.s_addr & bebits) == 0)
> -			break;
> +	for (maskn = 0; temp_mask; temp_mask <<= 1, maskn++);
>
>  	seq_printf(s, "%u.%u.%u.%u/%d %s\n",
>  		hp[0], hp[1], hp[2], hp[3], maskn, skp->smk_label);
> @@ -702,6 +696,42 @@ static int smk_open_netlbladdr(struct inode *inode,
> struct file *file) }
>
>  /**
> + * smk_netlbladdr_insert
> + * @new : netlabel to insert
> + *
> + * This helper insert netlabel in the smack_netlbladdrs list
> + * sorted by netmask length (longest to smallest)
> + */
> +static void smk_netlbladdr_insert(struct smk_netlbladdr *new)
> +{
> +	struct smk_netlbladdr *m;
> +
> +	if (smack_netlbladdrs == NULL) {
> +		smack_netlbladdrs = new;
> +		return;
> +	}
> +
> +	/* the comparison '>' is a bit hacky, but works */
> +	if (new->smk_mask.s_addr > smack_netlbladdrs->smk_mask.s_addr) {
> +		new->smk_next = smack_netlbladdrs;
> +		smack_netlbladdrs = new;
> +		return;
> +	}
> +	for (m = smack_netlbladdrs; m != NULL; m = m->smk_next) {
> +		if (m->smk_next == NULL) {
> +			m->smk_next = new;
> +			return;
> +		}
> +		if (new->smk_mask.s_addr > m->smk_next->smk_mask.s_addr) {
> +			new->smk_next = m->smk_next;
> +			m->smk_next = new;
> +			return;
> +		}
> +	}
> +}
> +
> +
> +/**
>   * smk_write_netlbladdr - write() for /smack/netlabel
>   * @filp: file pointer, not actually used
>   * @buf: where to get the data from
> @@ -724,8 +754,9 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, struct netlbl_audit audit_info;
>  	struct in_addr mask;
>  	unsigned int m;
> -	__be32 bebits = BEMASK;
> +	u32 mask_bits = (1<<31);
>  	__be32 nsa;
> +	u32 temp_mask;
>
>  	/*
>  	 * Must have privilege.
> @@ -761,10 +792,13 @@ static ssize_t smk_write_netlbladdr(struct file
> *file, const char __user *buf, if (sp == NULL)
>  		return -EINVAL;
>
> -	for (mask.s_addr = 0; m > 0; m--) {
> -		mask.s_addr |= bebits;
> -		bebits <<= 1;
> +	for (temp_mask = 0; m > 0; m--) {
> +		temp_mask |= mask_bits;
> +		mask_bits >>= 1;
>  	}
> +	mask.s_addr = cpu_to_be32(temp_mask);
> +
> +	newname.sin_addr.s_addr &= mask.s_addr;
>  	/*
>  	 * Only allow one writer at a time. Writes should be
>  	 * quite rare and small in any case.
> @@ -772,6 +806,7 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, mutex_lock(&smk_netlbladdr_lock);
>
>  	nsa = newname.sin_addr.s_addr;
> +	/* try to find if the prefix is already in the list */
>  	for (skp = smack_netlbladdrs; skp != NULL; skp = skp->smk_next)
>  		if (skp->smk_host.sin_addr.s_addr == nsa &&
>  		    skp->smk_mask.s_addr == mask.s_addr)
> @@ -787,9 +822,8 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, rc = 0;
>  			skp->smk_host.sin_addr.s_addr = newname.sin_addr.s_addr;
>  			skp->smk_mask.s_addr = mask.s_addr;
> -			skp->smk_next = smack_netlbladdrs;
>  			skp->smk_label = sp;
> -			smack_netlbladdrs = skp;
> +			smk_netlbladdr_insert(skp);
>  		}
>  	} else {
>  		rc = netlbl_cfg_unlbl_static_del(&init_net, NULL,

-- 
paul moore
linux @ hp

Re: [PATCH] SMACK netfilter smacklabel socket match

[Paul Moore]

On Wednesday 18 February 2009 01:29:11 pm etienne wrote:
> OK, I'm about to send a new patch; but while testing my patches and reading
> code, I noticed another bug :
>
> In smackfs.c:smk_write_netlbladdr
> the netmask mask.s_addr is not handled correctly, the netmask should be :
> 1- computed in u32
> 2- converted to be32 !!
> with current code, a "pseudo u32 mask" is applied to a be32 ipaddr; it
> occurs to works for "common netmasks" (multiple of 8), not for
> "intermediate" mask (/15, /25)

Heh, back when Casey was first drafting this code I mentioned the same issue 
regarding byte ordering but Casey assured me that everything was correct.  I 
didn't have a Smack test system at the time so I couldn't verify the behavior.  
I'm glad you had a chance to test it, needless to say you should fix that when 
you submit your patch.

-- 
paul moore
linux @ hp