Re: [RFC 0/4] NetLabel

[Paul Moore <paul.moore@hp.com>]

James Morris wrote:
> On Thu, 25 May 2006, Paul Moore wrote:
>>This patch introduces a new kernel feature designed to support labeled
>>networking protocols such as RIPSO and CIPSO.  These protocols are required to
>>interoperate with existing "trusted" operating systems such as Trusted
>>Solaris.
> 
> A few initial comments.
> 
> - Did you decide that you definitely need to verify labels on fragments?  
> 
> I can see the code's been added to do that, but wonder about a comment 
> made during earlier discussion that mislabeled fragments could only come 
> from a misbehaving trusted system.  What is the threat model here?
> 

This is one part of the patch that I really don't have a strong feeling
for either way.  There was some concern on the LSM list that not
checking the fragment options might be an issue so I added some code to
check the fragment options.  Personally I think we are probably okay
without it as the un-autenticated/un-verified nature of these labeling
protocols more or less requires either a trusted network/hosts.

If the community decides that this check is not required then I can
simply drop all of the changes in ip_fragment.c.

> - Can you explain how module loading and module refcounting for these 
> modules work?  (e.g. what causes netlabel_cipso_v4 to be loaded, is it 
> always safe to unload if the refcount is zero?)

Heh, not really :)

This is part of the "not ready for submission" qualifier I mentioned at
the top of my post.  Honestly I'm not sure it makes sense to have this
code as a loadable module anyway I just used some of the module bits as
it was the first example of code that I saw in the kernel which would
call init/exit functions.

If people think that this code should be made into a fully
loadable/unloadable module then there is definitely more work to be done
in this area as I really haven't looked into it too deeply.  However, if
people are okay with it not being a module then I will find a proper way
of doing initialization without using the module bits.

Sorry for the confusion, I forgot to mention it earlier.

> - What about user APIs for setting and retrieving labels?

The NetLabel mechanism supports getting the labels off of a packet
directly or from the top most packet on the incoming socket queue.  I
have left it up to the LSM to decide how to expose that functionality to
the user.  In SELinux this is done by using the SO_PEERSEC option
similar to how you would do it if you were using the IPsec SA labeling.
 It works by looking at the top most packet in the socket receive queue
for TCP and at the packet itself for UDP.

You can set/reset the label by calling the NetLabel function to set the
label of a socket; right now the label of outgoing packets is tightly
tied to the label of the socket from which they were sent.  The NetLabel
code does support changing the label of a socket but I have not added
the code to SELinux to support that because I am not clear that is a
good thing to do from a SELinux point of view, currently the label is
set when the socket is created.  However, should people decide this is a
good thing, one possibility would be to enable the SO_PEERSEC option for
setsockopt().

> - What about labeling of kernel-generated packets?

Kernel generated packets which are created in response to an incoming
packet, i.e. ICMP errors, get the label of the packet which caused the
response.  This seems to be correct from the draft's point of view as
well as several people on the LSM list.

There may be an issue with packets generated by the kernel directly and
not as a result of an incoming packet but I can't think of a case where
this would happen (although I suspect I am just not thinking hard
enough).  Do you have a scenario in mind?

> - Don't put #ifdef'd code into mainline code.
> 
> e.g. in net/ipv4/ip_fragment.c
> 
> +#ifdef CONFIG_NETLABEL_CIPSOV4
> +       if (sopt->cipso) {
> 
> This needs to be a function which is compiled away as a static inline when 
> not selected.  This stuff should have zero impact on the networking code 
> if not enabled.

Okay.  I suspect this code will go away, but just for my own education
were you thinking of something like this?

static inline int my_func(void)
#ifdef CONFIG_NETLABEL_CIPSOV4
	/* real stuff */
#else
	/* compile away into a zero */
	return 0;
#endif
}

... or something else?

> - Try and add entries for security/selinux/nlmsgtab.c for the new Netlink 
> protocol.
>
> - This does not follow normal kernel coding practices:
> 
> {snip}
> 
> - This kind of stuff should be removed before merging:
> 
> {snip}
> 

Okay.

> - Why does this module have a version number?
> 
> +       printk(KERN_INFO "NetLabel: Initializing (v%s %s)\n",
> +              NETLBL_VER_STR, NETLBL_VER_DATE);
> 

The version number is there primarily to help signal possible
differences in the NetLabel netlink protocol.

-- 
paul moore
linux security @ hp