linux-kernel - [patch 1/1] selinux: always initialize arguments to security_sid_to_context (Was: Re: [GIT PULL] audit patches)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 23 Feb 2007 09:00:18 -0500
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	viro@....linux.org.uk, torvalds@...ux-foundation.org,
	linux-kernel@...r.kernel.org, sgrubb@...hat.com, jmorris@...ei.org
Subject: [patch 1/1] selinux:  always initialize arguments to
	security_sid_to_context (Was: Re: [GIT PULL] audit patches)

On Thu, 2007-02-22 at 13:19 -0800, Andrew Morton wrote:
> > On Thu, 22 Feb 2007 08:22:47 -0500 Stephen Smalley <sds@...ho.nsa.gov> 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 <sds@...ho.nsa.gov>

---

 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/