[Fwd: [PATCH][RFC] SMACK : add logging support V1]

[Eric Paris]

On Mon, 2009-03-30 at 20:30 +0200, Etienne Basset wrote:

> > I didn't look much at all in the SMACK code only the more generic audit
> > code.  Are you sure you got the capability no audit stuff right?  I
> > guess you would know it pretty quickly since capabilities for memory
> > allocation are checked a LOT, but on OOM and FS full they are checked
> > too.
> > 
> I'm not quite sure what you're referring to. You mean a way to not log too much frequent events? 

SMACK doesn't have it's own capabilities enforcement (it just calls
cap_capable()) so this doesn't matter to you at all.   Just pretend I
didn't say anything.

> > You don't have to but I'd like to see some reworking if you are willing.
> > Any code that you stole line for line from SELinux, lets put that stuff
> > in a single place.  I think that's going to mean pretty much all of
> > smack_logging.h could be reasonably moved to a generic header for both
> > of you to use (just make it generic, i'm not asking you to port selinux
> > et al. to the new header.)   
> 
> OK, would it be something like include/linux/lsm_common_audit.h ?

James?  lsm_audit.h? security_audit.h?   Should I just shut up and not
try to work towards a generic lsm audit framework?  I'm just seeing a
lot of code that I've seen before.

> > I also feel like we could do better reusing
> > code from smack_log and avc_audit but maybe that's beyond what you are
> > willing to try to to take on
> >
> 
> you mean doing :
> smack:smack_log(...)
>    smack_audit_specific_data()
>    ...
>    common_security_audit(struct common_audit_data *)
> 
> selinux:avc_audit(...)
>    selinux_specific_data()
>    ...
>    common_security_audit(struct common_audit_data *);
> 
> It would make sense I guess

I believe they both output the same set of information right?  (except
selinux has a tclass smack doesn't?)  .  But since we can't really
fix/change SELinux logging right now maybe this isn't worth doing too
much work over.  If you see an easy way to make more of the code
generic, do it.  If not, don't worry about it.

The real difference between the two is in
security/selinux/avc.c::avc_dump_query()  vs the beginning of
smack_log().  If it's reasonable to somehow combine most/all of the
other code, i say we do it.  This might mean something like

struct selinux_audit_struct {
  u32 ssid
  u32 tsid
  u16 tclass
  u32 requested
  struct av_decision *avd
  int result
}

struct smack_audit_struct {
  char *function
  int result
  char *subj
  char *obj
  char *requested
}

common_audit_struct {
  [everything we have today]
  union {
    struct selinux_audit_struct selinux_audit;
    struct smack_audit_struct smack_audit;
  }
  void (*lsm_audit)(struct audit_buffer*, void *);
}

So we could call some new generic function that does all of the buffer
allocation and reporting, then call back into the private selinux/smack
function with the data in the union.  Maybe this isn't as easy as it
sounded in my head...  I just don't think we want a 3,4,5,6
implementations of lsm auditing.

> > I pretty strongly detest %s these days.  Using it on the left side of an
> > = is ok if you are REALLY careful.  Using it on the right makes me
> > cringe.  Can smack labels have characters which are not ascii letters
> > (spaces?)
> > 
> no, smack basically do the same tests when importing smack label than you do
> in kernel/audit.c:audit_string_contains_control
> except smack allows the '"' character

How did you plan to handle a SMACK label with a ' ?  Using the audit
string functions and being given a label with a " is going to give you
the hex output.  (which might someday turn into better encoding, but I'm
still waiting to see some code to do it better)

Can I suggest if you write userspace tools to do anything with these
audit records that you use libauparse?  So if we do make changes, SMACK
tools keep working (this is the main problem with changing how SELinux
uses audit, the userspace tools don't use libauparse so we can't make
changes in just the kernel+library...)

-Eric