Misc audit patches (resend again...); the most intrusive one is AUDIT_FD_PAIR,
allowing to log descriptor numbers from syscalls that do not return them in
usual way (i.e. pipe() and socketpair()). It took some massage of
the failure exits in sys_socketpair(); the rest is absolutely trivial.
Please, pull from
git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/ audit.b37
Al Viro (1):
AUDIT_FD_PAIR
Steve Grubb (2):
minor update to rule add/delete messages (ver 2)
audit config lockdown
fs/pipe.c | 7 ++
include/linux/audit.h | 9 ++
kernel/audit.c | 216 +++++++++++++++++++++++++++++++++++-------------
kernel/auditfilter.c | 9 +-
kernel/auditsc.c | 40 +++++++++
net/socket.c | 52 +++++++++---
6 files changed, 257 insertions(+), 76 deletions(-)
On Sun, 18 Feb 2007 04:01:27 +0000 Al Viro <[email protected]> wrote:
> Misc audit patches (resend again...); the most intrusive one is AUDIT_FD_PAIR,
> allowing to log descriptor numbers from syscalls that do not return them in
> usual way (i.e. pipe() and socketpair()). It took some massage of
> the failure exits in sys_socketpair(); the rest is absolutely trivial.
> Please, pull from
> git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/ audit.b37
Please send patches to the list for review if practical? In this case it
was. I trust davem has had a look at the non-trivial changes to
sys_socketpair().
Looking at the changes to audit_receive_msg():
if (sid) {
if (selinux_sid_to_string(
sid, &ctx, &len)) {
audit_log_format(ab,
" ssid=%u", sid);
/* Maybe call audit_panic? */
} else
audit_log_format(ab,
" subj=%s", ctx);
kfree(ctx);
}
This is assuming that selinux_sid_to_string() always initialises `ctx'.
But AFAICT there are two error paths in security_sid_to_context() which
forget to do that, so we end up doing kfree(uninitialised-local).
I'd consider that a shortcoming in security_sid_to_context(), so not a
problem in this patch, as long as people agree with my blaming above.
The coding style in there is a bit odd-looking.
The new __audit_fd_pair() has unneeded braces in it.
On Wed, 2007-02-21 at 16:03 -0800, Andrew Morton wrote:
> On Sun, 18 Feb 2007 04:01:27 +0000 Al Viro <[email protected]> wrote:
>
> > Misc audit patches (resend again...); the most intrusive one is AUDIT_FD_PAIR,
> > allowing to log descriptor numbers from syscalls that do not return them in
> > usual way (i.e. pipe() and socketpair()). It took some massage of
> > the failure exits in sys_socketpair(); the rest is absolutely trivial.
> > Please, pull from
> > git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/ audit.b37
>
> Please send patches to the list for review if practical? In this case it
> was. I trust davem has had a look at the non-trivial changes to
> sys_socketpair().
>
>
>
> Looking at the changes to audit_receive_msg():
>
>
> if (sid) {
> if (selinux_sid_to_string(
> sid, &ctx, &len)) {
> audit_log_format(ab,
> " ssid=%u", sid);
> /* Maybe call audit_panic? */
> } else
> audit_log_format(ab,
> " subj=%s", ctx);
> kfree(ctx);
> }
>
> This is assuming that selinux_sid_to_string() always initialises `ctx'.
>
> But AFAICT there are two error paths in security_sid_to_context() which
> forget to do that, so we end up doing kfree(uninitialised-local).
>
> I'd consider that a shortcoming in security_sid_to_context(), so not a
> problem in this patch, as long as people agree with my blaming above.
I wouldn't assume that the function initializes an argument if it
returns an error, and at least some of the callers (in auditsc.c) appear
to correctly initialize ctx to NULL themselves before calling
selinux_sid_to_string(). But if you'd prefer the function to always
handle it, we can do that.
>
> The coding style in there is a bit odd-looking.
>
> The new __audit_fd_pair() has unneeded braces in it.
--
Stephen Smalley
National Security Agency
> On Thu, 22 Feb 2007 08:22:47 -0500 Stephen Smalley <[email protected]> wrote:
> On Wed, 2007-02-21 at 16:03 -0800, Andrew Morton wrote:
> >
> > Looking at the changes to audit_receive_msg():
> >
> >
> > if (sid) {
> > if (selinux_sid_to_string(
> > sid, &ctx, &len)) {
> > audit_log_format(ab,
> > " ssid=%u", sid);
> > /* Maybe call audit_panic? */
> > } else
> > audit_log_format(ab,
> > " subj=%s", ctx);
> > kfree(ctx);
> > }
> >
> > This is assuming that selinux_sid_to_string() always initialises `ctx'.
> >
> > But AFAICT there are two error paths in security_sid_to_context() which
> > forget to do that, so we end up doing kfree(uninitialised-local).
> >
> > I'd consider that a shortcoming in security_sid_to_context(), so not a
> > problem in this patch, as long as people agree with my blaming above.
>
> I wouldn't assume that the function initializes an argument if it
> returns an error, and at least some of the callers (in auditsc.c) appear
> to correctly initialize ctx to NULL themselves before calling
> selinux_sid_to_string(). But if you'd prefer the function to always
> handle it, we can do that.
>
Well we now have (at least) one caller which assumes that *ctx is
initialied in error cases.
And I think it's sane to make it do that: safer, and will simplify coding
in the callers.
On Thu, 2007-02-22 at 13:19 -0800, Andrew Morton wrote:
> > On Thu, 22 Feb 2007 08:22:47 -0500 Stephen Smalley <[email protected]> wrote:
> > On Wed, 2007-02-21 at 16:03 -0800, Andrew Morton wrote:
> > >
> > > Looking at the changes to audit_receive_msg():
> > >
> > >
> > > if (sid) {
> > > if (selinux_sid_to_string(
> > > sid, &ctx, &len)) {
> > > audit_log_format(ab,
> > > " ssid=%u", sid);
> > > /* Maybe call audit_panic? */
> > > } else
> > > audit_log_format(ab,
> > > " subj=%s", ctx);
> > > kfree(ctx);
> > > }
> > >
> > > This is assuming that selinux_sid_to_string() always initialises `ctx'.
> > >
> > > But AFAICT there are two error paths in security_sid_to_context() which
> > > forget to do that, so we end up doing kfree(uninitialised-local).
> > >
> > > I'd consider that a shortcoming in security_sid_to_context(), so not a
> > > problem in this patch, as long as people agree with my blaming above.
> >
> > I wouldn't assume that the function initializes an argument if it
> > returns an error, and at least some of the callers (in auditsc.c) appear
> > to correctly initialize ctx to NULL themselves before calling
> > selinux_sid_to_string(). But if you'd prefer the function to always
> > handle it, we can do that.
> >
>
> Well we now have (at least) one caller which assumes that *ctx is
> initialied in error cases.
>
> And I think it's sane to make it do that: safer, and will simplify coding
> in the callers.
Ok, patch below.
Always initialize *scontext and *scontext_len in security_sid_to_context.
Signed-off-by: Stephen Smalley <[email protected]>
---
security/selinux/ss/services.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ca9154d..1e52356 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -609,6 +609,9 @@ int security_sid_to_context(u32 sid, char **scontext, u32 *scontext_len)
struct context *context;
int rc = 0;
+ *scontext = NULL;
+ *scontext_len = 0;
+
if (!ss_initialized) {
if (sid <= SECINITSID_NUM) {
char *scontextp;
--
Stephen Smalley
National Security Agency