On Monday 11 February 2008 7:00:33 pm Casey Schaufler wrote:
Nope, this approach is what I was talking about. There are some minor
issues discussed below but they should be easy/quick to fix.
I've looked at this before from a SELinux point of view to see if I
could get rid of this extra memory allocation/copy and there just isn't
a clean way to do it ("clean" being very subjective). The problem is
you either have to hold the policy lock while you make the NetLabel
call (not a good idea), move the selinux_netlbl_sock_setsid()
functionality back into security/selinux/ss/services.c (not desired by
the SELinux folks who like to keep the selinux/ss/ directory minimal),
or do the allocatin in security_netlbl_sid_to_secattr() and free it in
selinux_netlbl_sock_setsid(). Of those options, only the last is
really possible and it's so prone to memory leakage that I'm hesitant
to say it's better than what we currently have. Right now you do a
call to netlbl_secattr_init() to start, do what you want with the
secattr, and then you call netlbl_secattr_destroy() which will clean up
_everything_ so nothing is leaked. It works out really well because
all of the _secattr_init() calls are matched by _secattr_destroy()
calls within the same function; very easy to quickly scan and ensure
there are no problems (the memory leak for a few weeks ago was quickly
caught by looking at the code). I'm in no hurry to loose this handy
little property of the secattr, although I do agree that it isn't
optimal for SMACK at present. On the plus side this only happens once
per-socket and not per-packet so I don't expect the malloc/copy to be
fatal at this point.
You've got my mind revisiting this idea so give me a while and let me
see if I can think of something that should be palatable to everyone
involved. In the meantime, I think you should fixup the few little
nits in this patch and get it merged as it is a nice improvement and
something that I believe most SMACK users will want.
You should try and populate the 'audit_info' struct with actual info so
the generated audit record is more useful for people who care about
those things. It's only two fields, 'secid' and 'loginuid', which are
pretty self-explanatory.
There is still the potential for a race here between when you change
the 'smack_net_ambient' value and when you actually change the NetLabel
configuration as you still allow new sockets to be created/labeled
regardless of the 'smack_ambient_lock'. That said, considering that
this is a privileged operation and one that I don't expect to be very
frequent it's probably not a big deal, I just wanted to make sure you
were aware of that.
Should have noticed this sooner, how about replacing the above two lines
with the following?
nlsp->flags = NETLBL_SECATTR_DOMAIN | NETLBL_SECATTR_MLS_LVL;
Not quite ... NetLabel will take care of making sure things are labeled
correctly (default vs. ambient) but it can still fail for a variety of
reasons: memory pressure, labeling protocol cannot represent the
security attributes you passed, etc. Moral of the story, you still
need to check the return value of netlbl_sock_setattr() and deal with
failures in a sane manner.
You can keep the above if-statement if you like, but NetLabel handles
this case gracefully (if it doesn't let me know, it's a bug) so it
isn't necessary.
See above comments regarding return values.
Same thing.
Once more, but with feeling.
--
paul moore
linux security @ hp
--
