7:52 a.m.
On Thu, Jun 9, 2011 at 8:28 AM, Patrick McHardy <kaber(a)trash.net> wrote:
On 08.06.2011 21:39, Eric Paris wrote:
> On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb(a)redhat.com> wrote:
>> On Wednesday, June 08, 2011 03:08:38 PM Eric Paris wrote:
>>> On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four
>>>
>>> <mr.dash.four(a)googlemail.com> wrote:
>>>>> 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.
>>>>
>>>> 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.
>>
>> 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.
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.
-Eric
9:08 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
> 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.
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?
10:06 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Thu, Jun 9, 2011 at 10:08 AM, Mr Dash Four
<mr.dash.four(a)googlemail.com> wrote:
>> 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.
>
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?
Actually just make it a void function as I don't think anyone
would/could/should make use of the return value.
-Eric
10:16 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
> 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?
>
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?
3:36 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
>> #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?
>>
>
> 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?
If there are no objections,
I'll resubmit the patch at the weekend with
the above functionality implemented.
7:08 a.m.
New subject: [PATCH 4th revision] Add SELinux context support to AUDIT target
In this revision the conversion of secid to SELinux context and adding it to the audit log
is moved from xt_AUDIT.c to audit.c with the aid of a separate helper function -
audit_log_secctx - which does both the conversion and logging of SELinux context, thus
also preventing internal secid number being leaked to userspace. If conversion is not
successful an error is raised.
With the introduction of this helper function the work done in xt_AUDIT.c is much more
simplified. It also opens the possibility of this helper function being used by other
modules (including auditd itself), if desired. With this addition, typical (raw auditd)
output after applying the 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_client_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=22 dport=3561
obj=system_u:object_r:ssh_server_packet_t:s0
Signed-off-by: Mr Dash Four <mr.dash.four(a)googlemail.com>
---
include/linux/audit.h | 7 +++++++
kernel/audit.c | 29 +++++++++++++++++++++++++++++
net/netfilter/xt_AUDIT.c | 5 +++++
3 files changed, 41 insertions(+), 0 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9d339eb..3e47019 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -613,6 +613,12 @@ extern void audit_log_d_path(struct audit_buffer *ab,
extern void audit_log_key(struct audit_buffer *ab,
char *key);
extern void audit_log_lost(const char *message);
+#ifdef CONFIG_SECURITY
+extern void audit_log_secctx(struct audit_buffer *ab, u32 secid);
+#else
+#define audit_log_secctx(b,s) do { ; } while (0)
+#endif
+
extern int audit_update_lsm_rules(void);
/* Private API (for audit.c only) */
@@ -635,6 +641,7 @@ extern int audit_enabled;
#define audit_log_untrustedstring(a,s) do { ; } while (0)
#define audit_log_d_path(b, p, d) do { ; } while (0)
#define audit_log_key(b, k) do { ; } while (0)
+#define audit_log_secctx(b,s) do { ; } while (0)
#define audit_enabled 0
#endif
#endif
diff --git a/kernel/audit.c b/kernel/audit.c
index 9395003..4e0685e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -55,6 +55,9 @@
#include <net/sock.h>
#include <net/netlink.h>
#include <linux/skbuff.h>
+#ifdef CONFIG_SECURITY
+#include <linux/security.h>
+#endif
#include <linux/netlink.h>
#include <linux/freezer.h>
#include <linux/tty.h>
@@ -1502,6 +1505,32 @@ void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int
type,
}
}
+#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.
+ */
+void audit_log_secctx(struct audit_buffer *ab, u32 secid)
+{
+ u32 len;
+ char *secctx;
+
+ if (security_secid_to_secctx(secid, &secctx, &len)) {
+ audit_panic("Cannot convert secid to context");
+ } else {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
+ }
+}
+EXPORT_SYMBOL(audit_log_secctx);
+#endif
+
EXPORT_SYMBOL(audit_log_start);
EXPORT_SYMBOL(audit_log_end);
EXPORT_SYMBOL(audit_log_format);
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..4bca15a 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -163,6 +163,11 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
break;
}
+#ifdef CONFIG_NETWORK_SECMARK
+ if (skb->secmark)
+ audit_log_secctx(ab, skb->secmark);
+#endif
+
audit_log_end(ab);
errout:
--
1.7.3.4
7:20 a.m.
New subject: [PATCH 4th revision] Add SELinux context support to AUDIT target
On Saturday, June 18, 2011 08:08:05 AM Mr Dash Four wrote:
+#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.
+ */
+void audit_log_secctx(struct audit_buffer *ab, u32 secid)
+{
+ u32 len;
+ char *secctx;
+
+ if (security_secid_to_secctx(secid, &secctx, &len)) {
+ audit_panic("Cannot convert secid to context");
+ } else {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
Eric,
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?
-Steve
9:21 a.m.
New subject: [PATCH 4th revision] Add SELinux context support to AUDIT target
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);
9:27 a.m.
New subject: [PATCH 4th revision] Add SELinux context support to AUDIT target
On Mon, Jun 20, 2011 at 10:21 AM, Mr Dash Four
<mr.dash.four(a)googlemail.com> wrote:
> 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);
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 <eparis(a)redhat.com>
4933
days inactive
4944
days old
linux-audit@lists.linux-audit.osci.io
8 comments
3 participants
participants (3)
-
Eric Paris -
Mr Dash Four -
Steve Grubb