Add SELinux context support to AUDIT target

Add SELinux context support for AUDIT target. Typical (raw auditd) output after applying this patch would be:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 len=52 inif=? outif=eth0 subj=system_u:object_r:sshd_packet_t:s0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22

I think this new information should be added at the end of the string.

Quoted 2 lines

I think this new information should be added at the end of the string.

In other words:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 len=52 inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22 subj=system_u:object_r:sshd_packet_t:s0

As I am currently discussing this very issue (adding SELinux context to AUDIT) on the audit mail list, it was pointed out that "subj" should actually be "obj" as this is an object (i.e. a packet) on which this is applied, so that would ultimately mean:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 len=52 inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22 obj=system_u:object_r:sshd_packet_t:s0

I also need to check as I think the order is also important, otherwise ausearch/aureport may skip this due to "misconfiguration".

Add SELinux context support to AUDIT target (2nd revision). Typical (raw auditd) output after applying this patch would be:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=1 len=52 inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22 obj=system_u:object_r:ssh_packet_t:s0 type=NETFILTER_PKT msg=audit(1306772064.079:56): action=0 hook=3 len=48 inif=eth0 outif=? smac=00:05:5d:7c:27:0b dmac=00:02:b3:0a:7f:81 macproto=0x0800 saddr=10.1.2.1 daddr=10.1.1.7 ipid=462 proto=6 sport=3561 dport=22 obj=system_u:object_r:ssh_packet_t:s0

Signed-off-by Mr Dash Four <[email protected]>

---
 net/netfilter/xt_AUDIT.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

net/netfilter/xt_AUDIT.c +7 -0

index 363a99e..616cadc 100644
@@ -20,6 +20,9 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_AUDIT.h>
 #include <linux/netfilter_bridge/ebtables.h>
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+#include <linux/security.h>
+#endif
 #include <net/ipv6.h>
 #include <net/ip.h>
 

@@ -122,6 +125,10 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct xt_audit_info *info = par->targinfo;
 	struct audit_buffer *ab;
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+	u32 len;
+	char *secctx;
+#endif
 
 	ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
 	if (ab == NULL)

Minor nitpick. This 'if' needs one {

Quoted 3 lines

Minor nitpick. This 'if' needs one {

Is this a style-type requirement I wasn't aware of? Because from a syntax point of view the left/right braces aren't necessary.

Quoted 4 lines

Is this a style-type requirement I wasn't aware of? Because from a syntax point of view the left/right braces aren't necessary.

Aware of it, it's just coding style.

Quoted 7 lines

Quoted 4 lines

Is this a style-type requirement I wasn't aware of? Because from a syntax point of view the left/right braces aren't necessary.

Aware of it, it's just coding style.

Thanks for pointing it out (now I know)! Would you like me to resubmit the patch with the 2 braces included or are you happy with what I have provided?

Add SELinux context support to AUDIT target - 3rd revision (style-type changes made *only* since 2nd revision of this patch). Typical (raw auditd) output after applying this patch would be:

net/netfilter/xt_AUDIT.c +3 -0

+	}
+#endif
+
 	audit_log_end(ab);
 
 errout:

Logging the internal numerical representation of secctx is, as I have already stated about 3 times by now, exposing internal (private-to-the-kernel-only) information to userspace. That cannot be allowed.

Besides, this numerical representation isn't reliable - these numbers are dynamic and can change - another reason why they should not be allowed to be present in the audit log. What happens if I make changes to my security policy and then run ausearch/aureport? I am either going to see different (wrong!) context reported if ausearch/aureport attempts to "convert" those numbers into SELinux context, or, I am going to see meaningless numbers. Either way, useless or misleading information is going to be reported and we don't want that, do we?

else

	audit_log_format(ab, " osid=%u", skb->secmark);

_All_ audit code records the number on a failed conversion.

Quoted 4 lines · Mr Dash Four

Logging the internal numerical representation of secctx is, as I have already stated about 3 times by now, exposing internal (private-to-the-kernel-only) information to userspace. That cannot be allowed.

Quoted 3 lines

Besides, this numerical representation isn't reliable - these numbers are dynamic and can change - another reason why they should not be allowed to be present in the audit log.

Quoted 2 lines

What happens if I make changes to my security policy and then run ausearch/aureport?

Quoted 4 lines

I am either going to see different (wrong!) context reported if ausearch/aureport attempts to "convert" those numbers into SELinux context, or, I am going to see meaningless numbers. Either way, useless or misleading information is going to be reported and we don't want that, do we?

Quoted 5 lines

else

	audit_log_format(ab, " osid=%u", skb->secmark);

_All_ audit code records the number on a failed conversion.

I am assuming you haven't read the above. Show me one good reason why I should alter my patch to include that abomination of yours?

It doesn't matter if its private. If its important enough to log to the audit system, we can't let something like this slide.

Quoted 4 lines

Quoted 4 lines

Besides, this numerical representation isn't reliable - these numbers are dynamic and can change - another reason why they should not be allowed to be present in the audit log.

Doesn't matter.

Its the event that we want and all its attributes. If the label is not
correct, how else are we going to know?

Quoted 3 lines

Quoted 3 lines

What happens if I make changes to my security policy and then run ausearch/aureport?

Nothing.

Quoted 5 lines

Quoted 5 lines

I am either going to see different (wrong!) context reported if ausearch/aureport attempts to "convert" those numbers into SELinux context, or, I am going to see meaningless numbers. Either way, useless or misleading information is going to be reported and we don't want that, do we?

Yes, we do.

Quoted 3 lines · Mr Dash Four

Add SELinux context support to AUDIT target - 3rd revision (style-type changes made *only* since 2nd revision of this patch). Typical (raw auditd) output after applying this patch would be:

<snip>

net/netfilter/xt_AUDIT.c audit_tg(struct sk_buff *skb, const struct · line 178

Quoted 10 lines

net/netfilter/xt_AUDIT.c +6 -0

@@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
 xt_action_param *par) break;
 	}
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+	if (skb->secmark) {
+	  	if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+			audit_log_format(ab, " obj=%s", secctx);
+			security_release_secctx(secctx, len);
+		}

else

	audit_log_format(ab, " osid=%u", skb->secmark);

_All_ audit code records the number on a failed conversion.

-Steve

net/netfilter/xt_AUDIT.c audit_tg(struct sk_buff *skb, const struct xt_action_param *par) · line 184

Quoted 6 lines

net/netfilter/xt_AUDIT.c +3 -0

+	}
+#endif
+
 	audit_log_end(ab);
 
 errout:

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 19 lines · Steve Grubb

Quoted 3 lines · Mr Dash Four

Add SELinux context support to AUDIT target - 3rd revision (style-type changes made *only* since 2nd revision of this patch). Typical (raw auditd) output after applying this patch would be:

<snip>

net/netfilter/xt_AUDIT.c audit_tg(struct sk_buff *skb, const struct · line 178

Quoted 10 lines

net/netfilter/xt_AUDIT.c +6 -0

@@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
 xt_action_param *par) break;
      }
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+    if (skb->secmark) {
+            if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+                    audit_log_format(ab, " obj=%s", secctx);
+                    security_release_secctx(secctx, len);
+            }

else
      audit_log_format(ab, " osid=%u", skb->secmark);

Quoted 1 line

_All_  audit code records the number on a failed conversion.

But it really shouldn't. An unconvertible secid is indicative of a serious, unrecoverable failure within the LSM. It's every bit as bad as an invalid pointer.

int audit_log_secctx(struct auditbuffer *ab, u32 secid)

{
    int len, rc;
    char *ctx;

    rc = security_secid_to_secctx(sid, &ctx, &len);
    if (rc) {
        audit_panic("Cannot convert secid to context");
    } else {
            audit_log_format(ab, " subj=%s", ctx);
            security_release_secctx(ctx, len);
    }
    return rc;
}

Such a function could be used a couple of places in the audit code itself.

Quoted 16 lines

int audit_log_secctx(struct auditbuffer *ab, u32 secid) {
   int len, rc;
   char *ctx;

   rc = security_secid_to_secctx(sid, &ctx, &len); if (rc) {
       audit_panic("Cannot convert secid to context"); } else {
           audit_log_format(ab, " subj=%s", ctx); security_release_secctx(ctx, len); }
   return rc;
}

Quoted 2 lines

Such a function could be used a couple of places in the audit code itself.

My view on this is that LSM error-handling should be part of LSM.

I presume security_secid_to_secctx is going to be called from quite a few places (well, I know of at least two now and they have nothing to do with the LSM) and in my opinion it would be better if that error handling, if adopted, is implemented within the function itself - whether by calling another function, like the one you proposed above, or as part of the secctx retrieval - this could be open to interpretation, but the point I am trying to make is that whichever code security_secid_to_secctx is invoked from shouldn't be involved in reporting/handling (internal LSM) errors at all.

I think I made that point in my previous post, but just wanted to make sure that is the case.

The LSM might report and error. It's up to the caller to figure out how to deal with that error. In this case we want to use the audit system so it's up to the audit system how to handle that error. This helper function says the audit system should log it if it work and should audit_panic() if it doesn't. audit_panic() will just call printk for most people and can actually panic the box for nutters who really care. In this way we always log the information and if we don't it's up to audit how audit handles it's inability to log info.

It's not netfilter's job to handle the error. It's not the LSMs job to know how it's caller wants to handle the error. Audit is who has special requirements and the code to handle the error should be in audit code. (Maybe it wasn't clear, but I think this function should go in kernel/audit.c, not the netfilter code. The netfilter code should call this helper function.

On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four

Quoted 30 lines · <[email protected]> wrote:

Quoted 15 lines

int audit_log_secctx(struct auditbuffer *ab, u32 secid)

{
   int len, rc;
   char *ctx;

   rc = security_secid_to_secctx(sid, &ctx, &len);
   if (rc) {
       audit_panic("Cannot convert secid to context");
   } else {
           audit_log_format(ab, " subj=%s", ctx);
           security_release_secctx(ctx, len);
   }
   return rc;
}

Quoted 2 lines

Such a function could be used a couple of places in the audit code itself.

My view on this is that LSM error-handling should be part of LSM.

I presume security_secid_to_secctx is going to be called from quite a few places (well, I know of at least two now and they have nothing to do with the LSM) and in my opinion it would be better if that error handling, if adopted, is implemented within the function itself - whether by calling another function, like the one you proposed above, or as part of the secctx retrieval - this could be open to interpretation, but the point I am trying to make is that whichever code security_secid_to_secctx is invoked from shouldn't be involved in reporting/handling (internal LSM) errors at all.

Quoted 2 lines

I think I made that point in my previous post, but just wanted to make sure that is the case.

The LSM might report and error. It's up to the caller to figure out how to deal with that error. In this case we want to use the audit system so it's up to the audit system how to handle that error.

Quoted 36 lines · Eric Paris

On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four

Quoted 32 lines · <[email protected]> wrote:

Quoted 15 lines

int audit_log_secctx(struct auditbuffer *ab, u32 secid) {
   int len, rc;
   char *ctx;

   rc = security_secid_to_secctx(sid, &ctx, &len); if (rc) {
       audit_panic("Cannot convert secid to context"); } else {
           audit_log_format(ab, " subj=%s", ctx); security_release_secctx(ctx, len); }
   return rc;
}

Quoted 2 lines

Such a function could be used a couple of places in the audit code itself.

My view on this is that LSM error-handling should be part of LSM.

I presume security_secid_to_secctx is going to be called from quite a few places (well, I know of at least two now and they have nothing to do with the LSM) and in my opinion it would be better if that error handling, if adopted, is implemented within the function itself - whether by calling another function, like the one you proposed above, or as part of the secctx retrieval - this could be open to interpretation, but the point I am trying to make is that whichever code security_secid_to_secctx is invoked from shouldn't be involved in reporting/handling (internal LSM) errors at all.

I think I made that point in my previous post, but just wanted to make sure that is the case.

Quoted 3 lines

The LSM might report and error.  It's up to the caller to figure out how to deal with that error.  In this case we want to use the audit system so it's up to the audit system how to handle that error.

We are happy recording the failed number. Its the LSM people that say nuke the system. So, I would put that in security_secid_to_secctx() so that everyone knows whose requirements it was to do the nuclear option.

Quoted 37 lines · Steve Grubb

Quoted 36 lines · Eric Paris

On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four

Quoted 32 lines · <[email protected]> wrote:

Quoted 15 lines

int audit_log_secctx(struct auditbuffer *ab, u32 secid)

{
   int len, rc;
   char *ctx;

   rc = security_secid_to_secctx(sid, &ctx, &len);
   if (rc) {
       audit_panic("Cannot convert secid to context");
   } else {
           audit_log_format(ab, " subj=%s", ctx);
           security_release_secctx(ctx, len);
   }
   return rc;
}

Quoted 2 lines

Such a function could be used a couple of places in the audit code itself.

My view on this is that LSM error-handling should be part of LSM.

I presume security_secid_to_secctx is going to be called from quite a few places (well, I know of at least two now and they have nothing to do with the LSM) and in my opinion it would be better if that error handling, if adopted, is implemented within the function itself - whether by calling another function, like the one you proposed above, or as part of the secctx retrieval - this could be open to interpretation, but the point I am trying to make is that whichever code security_secid_to_secctx is invoked from shouldn't be involved in reporting/handling (internal LSM) errors at all.

I think I made that point in my previous post, but just wanted to make sure that is the case.

Quoted 7 lines

Quoted 3 lines

The LSM might report and error. It's up to the caller to figure out how to deal with that error. In this case we want to use the audit system so it's up to the audit system how to handle that error.

We are happy recording the failed number. Its the LSM people that say nuke the system. So, I would put that in security_secid_to_secctx() so that everyone knows whose requirements it was to do the nuclear option.

If the number meets your requirements then the requirements are total shit. The number has NO relation to the label on the object as understood by the system. If audit has a requirement to always log the label or call audit_panic(), its only option is to call audit_panic().

Exposing secids and internal representations of information to userspace is always wrong. Full stop.

I'd be willing to take a patch which caused security_secid_to_secctx() to BUG() if it got an invalid secid. But on ENOMEM I'm going to just push the error back up the stack. In that case audit has to decide how to handle the situation. That secid is NOT the label associated with the object and printing it to userspace is meaningless garbage.

Just because audit did it wrong yesterday doesn't mean I'm going to ACK more patches that do it wrong tomorrow. I don't care what some arbitrary and obviously poorly thought out requirement document says.

Quoted 61 lines · Eric Paris

Quoted 37 lines · Steve Grubb

Quoted 36 lines · Eric Paris

On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four

Quoted 32 lines · <[email protected]> wrote:

Quoted 15 lines

int audit_log_secctx(struct auditbuffer *ab, u32 secid) {
   int len, rc;
   char *ctx;

   rc = security_secid_to_secctx(sid, &ctx, &len); if (rc) {
       audit_panic("Cannot convert secid to context"); } else {
           audit_log_format(ab, " subj=%s", ctx); security_release_secctx(ctx, len); }
   return rc;
}

Quoted 2 lines

Such a function could be used a couple of places in the audit code itself.

My view on this is that LSM error-handling should be part of LSM.

I presume security_secid_to_secctx is going to be called from quite a few places (well, I know of at least two now and they have nothing to do with the LSM) and in my opinion it would be better if that error handling, if adopted, is implemented within the function itself - whether by calling another function, like the one you proposed above, or as part of the secctx retrieval - this could be open to interpretation, but the point I am trying to make is that whichever code security_secid_to_secctx is invoked from shouldn't be involved in reporting/handling (internal LSM) errors at all.

I think I made that point in my previous post, but just wanted to make sure that is the case.

Quoted 7 lines

Quoted 3 lines

The LSM might report and error.  It's up to the caller to figure out how to deal with that error.  In this case we want to use the audit system so it's up to the audit system how to handle that error.

We are happy recording the failed number. Its the LSM people that say nuke the system. So, I would put that in security_secid_to_secctx() so that everyone knows whose requirements it was to do the nuclear option.

If the number meets your requirements then the requirements are total shit.  The number has NO relation to the label on the object as understood by the system.  If audit has a requirement to always log the label or call audit_panic(), its only option is to call audit_panic().

Exposing secids and internal representations of information to userspace is always wrong.  Full stop.

I'd be willing to take a patch which caused security_secid_to_secctx() to BUG() if it got an invalid secid.  But on ENOMEM I'm going to just push the error back up the stack.  In that case audit has to decide how to handle the situation.  That secid is NOT the label associated with the object and printing it to userspace is meaningless garbage.

Quoted 3 lines

Just because audit did it wrong yesterday doesn't mean I'm going to ACK more patches that do it wrong tomorrow.  I don't care what some arbitrary and obviously poorly thought out requirement document says.

Just to make sure, so the conclusion is that the patch is fine as it is and anything related to unconvertible secids will be handled by SELinux internally?

Quoted 4 lines · Patrick McHardy

Just to make sure, so the conclusion is that the patch is fine as it is and anything related to unconvertible secids will be handled by SELinux internally?

No. This patch does not get my ACK. Steve is right that silently dropping information is a big big no no for the audit system and that's what this patch does. This cannot be wholly handled properly inside the LSM either. I don't see any patch meeting everyone's requirements outside of a new one that includes the audit helper I suggested.

Quoted 6 lines

Just to make sure, so the conclusion is that the patch is fine as it is and anything related to unconvertible secids will be handled by SELinux internally?

No. This patch does not get my ACK. Steve is right that silently dropping information is a big big no no for the audit system and that's what this patch does. This cannot be wholly handled properly inside the LSM either. I don't see any patch meeting everyone's requirements outside of a new one that includes the audit helper I suggested.

Quoted 7 lines

Quoted 6 lines

Just to make sure, so the conclusion is that the patch is fine as it is and anything related to unconvertible secids will be handled by SELinux internally?

Quoted 7 lines

No.  This patch does not get my ACK.  Steve is right that silently dropping information is a big big no no for the audit system and that's what this patch does.  This cannot be wholly handled properly inside the LSM either.  I don't see any patch meeting everyone's requirements outside of a new one that includes the audit helper I suggested.

Right, so the function you suggested yesterday (audit_log_secctx) should be added in audit.c in its entirety, and xt_AUDIT.c should just have something like:

#ifdef CONFIG_NF_CONNTRACK_SECMARK
  if (skb->secmark)
              audit_log_secctx(ab,skb->secmark);
#endif

Thus, discarding the result (rc), unless we are interested in the error code, which I don't think is the case here. Would everyone be happy with this?

Quoted 10 lines

Right, so the function you suggested yesterday (audit_log_secctx) should be added in audit.c in its entirety, and xt_AUDIT.c should just have something like:

#ifdef CONFIG_NF_CONNTRACK_SECMARK
if (skb->secmark)

              audit_log_secctx(ab,skb->secmark);

#endif

Quoted 4 lines

Thus, discarding the result (rc), unless we are interested in the error code, which I don't think is the case here. Would everyone be happy with this?

Actually just make it a void function as I don't think anyone would/could/should make use of the return value.

Quoted 6 lines

Quoted 6 lines

#ifdef CONFIG_NF_CONNTRACK_SECMARK
if (skb->secmark)

              audit_log_secctx(ab,skb->secmark);

#endif

Quoted 8 lines

Quoted 4 lines

Thus, discarding the result (rc), unless we are interested in the error code, which I don't think is the case here. Would everyone be happy with this?

Actually just make it a void function as I don't think anyone would/could/should make use of the return value.

In other words (audit.c) - N.B. the change from "subj" to "obj" as per Steve's suggestion a while ago:

void audit_log_secctx(struct auditbuffer *ab, u32 secid)

{
   int len;
   char *ctx;

   if (security_secid_to_secctx(sid, &ctx, &len)) {
       audit_panic("Cannot convert secid to context");
   } else {
           audit_log_format(ab, " obj=%s", ctx);
           security_release_secctx(ctx, len);
   }
}

And xt_AUDIT.c stays as per my suggestion above. Should I assume that gets the "go" from everyone concerned?

kernel/audit.c +7 -0

+#ifdef CONFIG_SECURITY
+/**
+ * audit_log_secctx - Converts and logs SELinux context
+ * @ab: audit_buffer
+ * @secid: security number
+ *
+ * This is a helper function that calls security_secid_to_secctx to

convert secid to secctx + * and then adds the (converted) SELinux context to the audit log + * by calling audit_log_format, thus also preventing leak of internal secid to userspace. + * If secid cannot be converted audit_panic is called.
+ */

Do you think this should be hardcoded to be obj? Would we ever log the subj? Or should obj be part of the function name to make it clear which piece is getting logged?

Quoted 5 lines

Do you think this should be hardcoded to be obj? Would we ever log the subj? Or should obj be part of the function name to make it clear which piece is getting logged?

I thought of that, though I don't know what variety of option names would be there to be used with that function.

If there is a need to use something other than "obj", like, "subj" or even "tcontext" or "scontext" for example, then I would favour passing the option name as function parameter - something like "void audit_log_secctx(struct audit_buffer *ab, char *secname, u32 secid)" or even "void audit_log_secctx(struct audit_buffer *ab, int secname, u32 secid)" (secname here being one of 0, 1, 2 ... corresponding to "obj", "subj" etc).

Similar approach is already used in audit.c - in audit_log_config_change for example:

static int audit_log_config_change(char *function_name, int new, int old, uid_t loginuid, u32 sessionid, u32 sid, int allow_changes)
{
  struct audit_buffer *ab;
  int rc = 0;

  ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); audit_log_format(ab, "%s=%d old=%d auid=%u ses=%u", function_name, new, old, loginuid, sessionid);

Quoted 25 lines · Mr Dash Four

Quoted 25 lines

Quoted 5 lines

Quoted 5 lines

Do you think this should be hardcoded to be obj? Would we ever log the subj? Or should obj be part of the function name to make it clear which piece is getting logged?

I thought of that, though I don't know what variety of option names would be there to be used with that function.

If there is a need to use something other than "obj", like, "subj" or even "tcontext" or "scontext" for example, then I would favour passing the option name as function parameter - something like "void audit_log_secctx(struct audit_buffer *ab, char *secname, u32 secid)" or even "void audit_log_secctx(struct audit_buffer *ab, int secname, u32 secid)" (secname here being one of 0, 1, 2 ... corresponding to "obj", "subj" etc).

Similar approach is already used in audit.c - in audit_log_config_change for example:

static int audit_log_config_change(char *function_name, int new, int old, uid_t loginuid, u32 sessionid, u32 sid, int allow_changes)
{
struct audit_buffer *ab;
int rc = 0;

Quoted 3 lines

Quoted 3 lines

ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); audit_log_format(ab, "%s=%d old=%d auid=%u ses=%u", function_name, new,
old, loginuid, sessionid);

Hard code for now. %s in audit record building is the devil since there is no enforcement of audit's rather 'special' string encoding rules. If we need another name later we'll cross that bridge when we get there, possibly with another helper function and pushing the %s to a static function inside audit. I will not willing expose %s to code outside of audit.c.

Acked-by Eric Paris <[email protected]>

Quoted 21 lines · Steve Grubb

Quoted 4 lines · Mr Dash Four

Add SELinux context support to AUDIT target - 3rd revision (style-type changes made *only* since 2nd revision of this patch). Typical (raw

auditd) output after applying this patch would be:

<snip>

net/netfilter/xt_AUDIT.c audit_tg(struct sk_buff *skb, const struct · line 179

Quoted 11 lines

net/netfilter/xt_AUDIT.c +6 -0

@@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
 xt_action_param *par) break;
 
 	}
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+	if (skb->secmark) {
+	  	if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+			audit_log_format(ab, " obj=%s", secctx);
+			security_release_secctx(secctx, len);
+		}

else

Quoted 3 lines

	audit_log_format(ab, " osid=%u", skb->secmark);

_All_ audit code records the number on a failed conversion.

But it really shouldn't. An unconvertible secid is indicative of a serious, unrecoverable failure within the LSM. It's every bit as bad as an invalid pointer.

how is this error preserved in the audit trail?

Add SELinux context support to AUDIT target (2nd revision). Typical (raw auditd) output after applying this patch would be:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=1 len=52 inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22 obj=system_u:object_r:ssh_packet_t:s0 type=NETFILTER_PKT msg=audit(1306772064.079:56): action=0 hook=3 len=48 inif=eth0 outif=? smac=00:05:5d:7c:27:0b dmac=00:02:b3:0a:7f:81 macproto=0x0800 saddr=10.1.2.1 daddr=10.1.1.7 ipid=462 proto=6 sport=3561 dport=22 obj=system_u:object_r:ssh_packet_t:s0

Signed-off-by Mr Dash Four <[email protected]>

---
 net/netfilter/xt_AUDIT.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

net/netfilter/xt_AUDIT.c +7 -0

index 363a99e..616cadc 100644
@@ -20,6 +20,9 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_AUDIT.h>
 #include <linux/netfilter_bridge/ebtables.h>
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+#include <linux/security.h>
+#endif
 #include <net/ipv6.h>
 #include <net/ip.h>
 

@@ -122,6 +125,10 @@ audit_tg(struct sk_buff *skb, const struct
 xt_action_param *par) {
 	const struct xt_audit_info *info = par->targinfo;
 	struct audit_buffer *ab;
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+	u32 len;
+	char *secctx;
+#endif
 
 	ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
 	if (ab == NULL)

net/netfilter/xt_AUDIT.c +2 -0

+#endif
+
 	audit_log_end(ab);
 
 errout:

Normally there would be an else here to do something like audit_log_format(ab, " osid=%u", skb->secmark);
so that its recorded numerically if the context could not be looked up.

Quoted 3 lines

Normally there would be an else here to do something like audit_log_format(ab, " osid=%u", skb->secmark);
so that its recorded numerically if the context could not be looked up.

I disagree! That approach was dropped long ago when the secctx was first introduced to prevent kernel information leaking into userspace (Eric would know more about this as he designed that aspect of it a couple of months ago).

This is not any more leak than leaking the context string to user space as this patch attempts to do. The rest of the audit code does log the numeric representation when text fails.

Quoted 3 lines

This is not any more leak than leaking the context string to user space as this patch attempts to do. The rest of the audit code does log the numeric representation when text fails.

There is no "leak" when the secctx is recorded in the audit log - it is supposed to be there, if present (and retrievable).

As for exposing the (internal) numerical representation of the secctx - this was discussed previously and the approach you are suggesting was dropped. To quote Eric on this very issue "[It] exports the internal secid to userspace. These are dynamic, can change on lsm changes, and have no meaning in userspace. We should instead be sending lsm contexts to userspace instead.".

Exactly my point. There is no leak if its text or numeric.

Quoted 7 lines

As for exposing the (internal) numerical representation of the secctx - this was discussed previously and the approach you are suggesting was dropped. To quote Eric on this very issue "[It] exports the internal secid to userspace. These are dynamic, can change on lsm changes, and have no meaning in userspace. We should instead be sending lsm contexts to userspace instead.".

Doesn't matter. The requirements of the protection profiles say to log the object's label. It does not care if its text or numeric. It also does not say sometimes or only when its convenient. :)

Its either important enough to log even if text conversion
fails or its not important enough to log at all.

Quoted 3 lines

Exactly my point. There is no leak if its text or numeric.

No, there is no leak if it is a text, but there *is* a leak if it is a numeric. I think I've made that quite clear.

Quoted 4 lines · Mr Dash Four

Quoted 1 line

Exactly my point. There is no leak if its text or numeric.

No, there is no leak if it is a text, but there *is* a leak if it is a numeric. I think I've made that quite clear.

We don't use numeric secmark anymore in nf_conntrack. Not very familiar with SELinux, but I remember that the convention was not to provide internal numeric values.

Quoted 7 lines · Pablo Neira Ayuso

Quoted 3 lines · Mr Dash Four

Quoted 1 line

Exactly my point. There is no leak if its text or numeric.

No, there is no leak if it is a text, but there *is* a leak if it is a numeric. I think I've made that quite clear.

We don't use numeric secmark anymore in nf_conntrack. Not very familiar with SELinux, but I remember that the convention was not to provide internal numeric values.

All of the audit system records the numbers if conversion fails.

We want it as
forensic evidence or troubleshooting information as the case may be.

-Steve

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit