[BUG] security_release_secctx seems broken

[BUG] security_release_secctx seems broken

[Konstantin Khlebnikov]

I've got this kmemleak splat

unreferenced object 0xffff880f687ff6a8 (size 32):
   comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
   hex dump (first 32 bytes):
     01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00  ................
     00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  .....kkkkkkkkkk.
   backtrace:
     [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
     [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
     [<ffffffff812125c7>] krealloc+0xa7/0xc0
     [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
     [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
     [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
     [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
     [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
     [<ffffffff812ab27e>] getxattr+0x9e/0x170
     [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
     [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
     [<ffffffffffffffff>] 0xffffffffffffffff

allocated in  xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
should be freed by security_release_secctx() but there is no release_secctx hook.

I don't know details about modern security models stack but it
seems security_release_secctx() have to know which layer owns
secdata to free it property: selinux just calls kfree,
capability_hooks should do the same, but other modules returns
non-freeable pointers. Currently security_release_secctx() calls
release_secctx for all models, this is obviously wrong.

[BUG] security_release_secctx seems broken

[Konstantin Khlebnikov]

I've got this kmemleak splat

unreferenced object 0xffff880f687ff6a8 (size 32):
   comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
   hex dump (first 32 bytes):
     01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00  ................
     00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  .....kkkkkkkkkk.
   backtrace:
     [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
     [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
     [<ffffffff812125c7>] krealloc+0xa7/0xc0
     [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
     [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
     [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
     [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
     [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
     [<ffffffff812ab27e>] getxattr+0x9e/0x170
     [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
     [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
     [<ffffffffffffffff>] 0xffffffffffffffff

allocated in  xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
should be freed by security_release_secctx() but there is no release_secctx hook.

I don't know details about modern security models stack but it
seems security_release_secctx() have to know which layer owns
secdata to free it property: selinux just calls kfree,
capability_hooks should do the same, but other modules returns
non-freeable pointers. Currently security_release_secctx() calls
release_secctx for all models, this is obviously wrong.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] security_release_secctx seems broken

[Casey Schaufler]

On 9/16/2017 11:18 AM, Konstantin Khlebnikov wrote:
> I've got this kmemleak splat
>
> unreferenced object 0xffff880f687ff6a8 (size 32):
>   comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
>   hex dump (first 32 bytes):
>     01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  .....kkkkkkkkkk.
>   backtrace:
>     [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
>     [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
>     [<ffffffff812125c7>] krealloc+0xa7/0xc0
>     [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
>     [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
>     [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
>     [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
>     [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
>     [<ffffffff812ab27e>] getxattr+0x9e/0x170
>     [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
>     [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
>     [<ffffffffffffffff>] 0xffffffffffffffff
>
> allocated in  xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
> should be freed by security_release_secctx() but there is no release_secctx hook.

There is a bug here, but the fix suggested is not correct.
security_inode_getsecurity() provides the security attribute
with a specified name. In the past there would be exactly one
use for this call, which was to return the value of the
attribute that happened to match the security "context" of
the inode. Freeing the value with security_release_secctx()
works coincidentally. Because security_inode_getsecurity() is
called with the "true" value for the alloc parameter the correct
fix is:

	- fix smack_inode_getsecurity() to honor the alloc flag
	- change the security_release_secctx() to kfree here.

I can provide a patch in a day or so.

>
> I don't know details about modern security models stack but it
> seems security_release_secctx() have to know which layer owns
> secdata to free it property: selinux just calls kfree,
> capability_hooks should do the same, but other modules returns
> non-freeable pointers. Currently security_release_secctx() calls
> release_secctx for all models, this is obviously wrong.
>


.

[BUG] security_release_secctx seems broken

[Casey Schaufler]

On 9/16/2017 11:18 AM, Konstantin Khlebnikov wrote:
> I've got this kmemleak splat
>
> unreferenced object 0xffff880f687ff6a8 (size 32):
> ? comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
> ? hex dump (first 32 bytes):
> ??? 01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00? ................
> ??? 00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5? .....kkkkkkkkkk.
> ? backtrace:
> ??? [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
> ??? [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
> ??? [<ffffffff812125c7>] krealloc+0xa7/0xc0
> ??? [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
> ??? [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
> ??? [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
> ??? [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
> ??? [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
> ??? [<ffffffff812ab27e>] getxattr+0x9e/0x170
> ??? [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
> ??? [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
> ??? [<ffffffffffffffff>] 0xffffffffffffffff
>
> allocated in? xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
> should be freed by security_release_secctx() but there is no release_secctx hook.

There is a bug here, but the fix suggested is not correct.
security_inode_getsecurity() provides the security attribute
with a specified name. In the past there would be exactly one
use for this call, which was to return the value of the
attribute that happened to match the security "context" of
the inode. Freeing the value with security_release_secctx()
works coincidentally. Because security_inode_getsecurity() is
called with the "true" value for the alloc parameter the correct
fix is:

	- fix smack_inode_getsecurity() to honor the alloc flag
	- change the security_release_secctx() to kfree here.

I can provide a patch in a day or so.

>
> I don't know details about modern security models stack but it
> seems security_release_secctx() have to know which layer owns
> secdata to free it property: selinux just calls kfree,
> capability_hooks should do the same, but other modules returns
> non-freeable pointers. Currently security_release_secctx() calls
> release_secctx for all models, this is obviously wrong.
>


.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] security_release_secctx seems broken

[Casey Schaufler]

On 9/18/2017 1:25 PM, Casey Schaufler wrote:
> On 9/16/2017 11:18 AM, Konstantin Khlebnikov wrote:
>> I've got this kmemleak splat

Do you have a convenient test case? I'd like to 
verify my patch with your case if it is reasonable
to do so.

>>
>> unreferenced object 0xffff880f687ff6a8 (size 32):
>>   comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
>>   hex dump (first 32 bytes):
>>     01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00  ................
>>     00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  .....kkkkkkkkkk.
>>   backtrace:
>>     [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
>>     [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
>>     [<ffffffff812125c7>] krealloc+0xa7/0xc0
>>     [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
>>     [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
>>     [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
>>     [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
>>     [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
>>     [<ffffffff812ab27e>] getxattr+0x9e/0x170
>>     [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
>>     [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
>>     [<ffffffffffffffff>] 0xffffffffffffffff
>>
>> allocated in  xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
>> should be freed by security_release_secctx() but there is no release_secctx hook.
> There is a bug here, but the fix suggested is not correct.
> security_inode_getsecurity() provides the security attribute
> with a specified name. In the past there would be exactly one
> use for this call, which was to return the value of the
> attribute that happened to match the security "context" of
> the inode. Freeing the value with security_release_secctx()
> works coincidentally. Because security_inode_getsecurity() is
> called with the "true" value for the alloc parameter the correct
> fix is:
>
> 	- fix smack_inode_getsecurity() to honor the alloc flag
> 	- change the security_release_secctx() to kfree here.
>
> I can provide a patch in a day or so.
>
>> I don't know details about modern security models stack but it
>> seems security_release_secctx() have to know which layer owns
>> secdata to free it property: selinux just calls kfree,
>> capability_hooks should do the same, but other modules returns
>> non-freeable pointers. Currently security_release_secctx() calls
>> release_secctx for all models, this is obviously wrong.
>>
>
> .
>

[BUG] security_release_secctx seems broken

[Casey Schaufler]

On 9/18/2017 1:25 PM, Casey Schaufler wrote:
> On 9/16/2017 11:18 AM, Konstantin Khlebnikov wrote:
>> I've got this kmemleak splat

Do you have a convenient test case? I'd like to 
verify my patch with your case if it is reasonable
to do so.

>>
>> unreferenced object 0xffff880f687ff6a8 (size 32):
>> ? comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
>> ? hex dump (first 32 bytes):
>> ??? 01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00? ................
>> ??? 00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5? .....kkkkkkkkkk.
>> ? backtrace:
>> ??? [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
>> ??? [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
>> ??? [<ffffffff812125c7>] krealloc+0xa7/0xc0
>> ??? [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
>> ??? [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
>> ??? [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
>> ??? [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
>> ??? [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
>> ??? [<ffffffff812ab27e>] getxattr+0x9e/0x170
>> ??? [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
>> ??? [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
>> ??? [<ffffffffffffffff>] 0xffffffffffffffff
>>
>> allocated in? xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
>> should be freed by security_release_secctx() but there is no release_secctx hook.
> There is a bug here, but the fix suggested is not correct.
> security_inode_getsecurity() provides the security attribute
> with a specified name. In the past there would be exactly one
> use for this call, which was to return the value of the
> attribute that happened to match the security "context" of
> the inode. Freeing the value with security_release_secctx()
> works coincidentally. Because security_inode_getsecurity() is
> called with the "true" value for the alloc parameter the correct
> fix is:
>
> 	- fix smack_inode_getsecurity() to honor the alloc flag
> 	- change the security_release_secctx() to kfree here.
>
> I can provide a patch in a day or so.
>
>> I don't know details about modern security models stack but it
>> seems security_release_secctx() have to know which layer owns
>> secdata to free it property: selinux just calls kfree,
>> capability_hooks should do the same, but other modules returns
>> non-freeable pointers. Currently security_release_secctx() calls
>> release_secctx for all models, this is obviously wrong.
>>
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] fix security_release_secctx seems broken

[Casey Schaufler]

Subject: [PATCH] fix security_release_secctx seems broken

security_inode_getsecurity() provides the text string value
of a security attribute. It does not provide a "secctx".
The code in xattr_getsecurity() that calls security_inode_getsecurity()
and then calls security_release_secctx() happened to work because
SElinux and Smack treat the attribute and the secctx the same way.
It fails for cap_inode_getsecurity(), because that module has no
secctx that ever needs releasing. It turns out that Smack is the
one that's doing things wrong by not allocating memory when instructed
to do so by the "alloc" parameter.

The fix is simple enough. Change the security_release_secctx() to
kfree() because it isn't a secctx being returned by
security_inode_getsecurity(). Change Smack to allocate the string when
told to do so.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

---
 fs/xattr.c                 |  2 +-
 security/smack/smack_lsm.c | 55 +++++++++++++++++++++-------------------------
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 4424f7fecf14..61cd28ba25f3 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -250,7 +250,7 @@ xattr_getsecurity(struct inode *inode, const char *name, void *value,
 	}
 	memcpy(value, buffer, len);
 out:
-	security_release_secctx(buffer, len);
+	kfree(buffer);
 out_noalloc:
 	return len;
 }
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 319add31b4a4..286171a16ed2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1473,7 +1473,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
  * @inode: the object
  * @name: attribute name
  * @buffer: where to put the result
- * @alloc: unused
+ * @alloc: duplicate memory
  *
  * Returns the size of the attribute or an error code
  */
@@ -1486,43 +1486,38 @@ static int smack_inode_getsecurity(struct inode *inode,
 	struct super_block *sbp;
 	struct inode *ip = (struct inode *)inode;
 	struct smack_known *isp;
-	int ilen;
-	int rc = 0;
 
-	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
+	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0)
 		isp = smk_of_inode(inode);
-		ilen = strlen(isp->smk_known);
-		*buffer = isp->smk_known;
-		return ilen;
-	}
+	else {
+		/*
+		 * The rest of the Smack xattrs are only on sockets.
+		 */
+		sbp = ip->i_sb;
+		if (sbp->s_magic != SOCKFS_MAGIC)
+			return -EOPNOTSUPP;
 
-	/*
-	 * The rest of the Smack xattrs are only on sockets.
-	 */
-	sbp = ip->i_sb;
-	if (sbp->s_magic != SOCKFS_MAGIC)
-		return -EOPNOTSUPP;
+		sock = SOCKET_I(ip);
+		if (sock == NULL || sock->sk == NULL)
+			return -EOPNOTSUPP;
 
-	sock = SOCKET_I(ip);
-	if (sock == NULL || sock->sk == NULL)
-		return -EOPNOTSUPP;
-
-	ssp = sock->sk->sk_security;
+		ssp = sock->sk->sk_security;
 
-	if (strcmp(name, XATTR_SMACK_IPIN) == 0)
-		isp = ssp->smk_in;
-	else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
-		isp = ssp->smk_out;
-	else
-		return -EOPNOTSUPP;
+		if (strcmp(name, XATTR_SMACK_IPIN) == 0)
+			isp = ssp->smk_in;
+		else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
+			isp = ssp->smk_out;
+		else
+			return -EOPNOTSUPP;
+	}
 
-	ilen = strlen(isp->smk_known);
-	if (rc == 0) {
-		*buffer = isp->smk_known;
-		rc = ilen;
+	if (alloc) {
+		*buffer = kstrdup(isp->smk_known, GFP_KERNEL);
+		if (*buffer == NULL)
+			return -ENOMEM;
 	}
 
-	return rc;
+	return strlen(isp->smk_known);
 }
 
 

[PATCH] fix security_release_secctx seems broken

[Casey Schaufler]

Subject: [PATCH] fix security_release_secctx seems broken

security_inode_getsecurity() provides the text string value
of a security attribute. It does not provide a "secctx".
The code in xattr_getsecurity() that calls security_inode_getsecurity()
and then calls security_release_secctx() happened to work because
SElinux and Smack treat the attribute and the secctx the same way.
It fails for cap_inode_getsecurity(), because that module has no
secctx that ever needs releasing. It turns out that Smack is the
one that's doing things wrong by not allocating memory when instructed
to do so by the "alloc" parameter.

The fix is simple enough. Change the security_release_secctx() to
kfree() because it isn't a secctx being returned by
security_inode_getsecurity(). Change Smack to allocate the string when
told to do so.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

---
 fs/xattr.c                 |  2 +-
 security/smack/smack_lsm.c | 55 +++++++++++++++++++++-------------------------
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 4424f7fecf14..61cd28ba25f3 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -250,7 +250,7 @@ xattr_getsecurity(struct inode *inode, const char *name, void *value,
 	}
 	memcpy(value, buffer, len);
 out:
-	security_release_secctx(buffer, len);
+	kfree(buffer);
 out_noalloc:
 	return len;
 }
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 319add31b4a4..286171a16ed2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1473,7 +1473,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
  * @inode: the object
  * @name: attribute name
  * @buffer: where to put the result
- * @alloc: unused
+ * @alloc: duplicate memory
  *
  * Returns the size of the attribute or an error code
  */
@@ -1486,43 +1486,38 @@ static int smack_inode_getsecurity(struct inode *inode,
 	struct super_block *sbp;
 	struct inode *ip = (struct inode *)inode;
 	struct smack_known *isp;
-	int ilen;
-	int rc = 0;
 
-	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
+	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0)
 		isp = smk_of_inode(inode);
-		ilen = strlen(isp->smk_known);
-		*buffer = isp->smk_known;
-		return ilen;
-	}
+	else {
+		/*
+		 * The rest of the Smack xattrs are only on sockets.
+		 */
+		sbp = ip->i_sb;
+		if (sbp->s_magic != SOCKFS_MAGIC)
+			return -EOPNOTSUPP;
 
-	/*
-	 * The rest of the Smack xattrs are only on sockets.
-	 */
-	sbp = ip->i_sb;
-	if (sbp->s_magic != SOCKFS_MAGIC)
-		return -EOPNOTSUPP;
+		sock = SOCKET_I(ip);
+		if (sock == NULL || sock->sk == NULL)
+			return -EOPNOTSUPP;
 
-	sock = SOCKET_I(ip);
-	if (sock == NULL || sock->sk == NULL)
-		return -EOPNOTSUPP;
-
-	ssp = sock->sk->sk_security;
+		ssp = sock->sk->sk_security;
 
-	if (strcmp(name, XATTR_SMACK_IPIN) == 0)
-		isp = ssp->smk_in;
-	else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
-		isp = ssp->smk_out;
-	else
-		return -EOPNOTSUPP;
+		if (strcmp(name, XATTR_SMACK_IPIN) == 0)
+			isp = ssp->smk_in;
+		else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
+			isp = ssp->smk_out;
+		else
+			return -EOPNOTSUPP;
+	}
 
-	ilen = strlen(isp->smk_known);
-	if (rc == 0) {
-		*buffer = isp->smk_known;
-		rc = ilen;
+	if (alloc) {
+		*buffer = kstrdup(isp->smk_known, GFP_KERNEL);
+		if (*buffer == NULL)
+			return -ENOMEM;
 	}
 
-	return rc;
+	return strlen(isp->smk_known);
 }
 
 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix security_release_secctx seems broken

[Konstantin Khlebnikov]

On 19.09.2017 19:39, Casey Schaufler wrote:
> Subject: [PATCH] fix security_release_secctx seems broken
> 
> security_inode_getsecurity() provides the text string value
> of a security attribute. It does not provide a "secctx".
> The code in xattr_getsecurity() that calls security_inode_getsecurity()
> and then calls security_release_secctx() happened to work because
> SElinux and Smack treat the attribute and the secctx the same way.
> It fails for cap_inode_getsecurity(), because that module has no
> secctx that ever needs releasing. It turns out that Smack is the
> one that's doing things wrong by not allocating memory when instructed
> to do so by the "alloc" parameter.
> 
> The fix is simple enough. Change the security_release_secctx() to
> kfree() because it isn't a secctx being returned by
> security_inode_getsecurity(). Change Smack to allocate the string when
> told to do so.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good for me.

Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>


> 
> ---
>   fs/xattr.c                 |  2 +-
>   security/smack/smack_lsm.c | 55 +++++++++++++++++++++-------------------------
>   2 files changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4424f7fecf14..61cd28ba25f3 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -250,7 +250,7 @@ xattr_getsecurity(struct inode *inode, const char *name, void *value,
>   	}
>   	memcpy(value, buffer, len);
>   out:
> -	security_release_secctx(buffer, len);
> +	kfree(buffer);
>   out_noalloc:
>   	return len;
>   }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 319add31b4a4..286171a16ed2 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1473,7 +1473,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
>    * @inode: the object
>    * @name: attribute name
>    * @buffer: where to put the result
> - * @alloc: unused
> + * @alloc: duplicate memory
>    *
>    * Returns the size of the attribute or an error code
>    */
> @@ -1486,43 +1486,38 @@ static int smack_inode_getsecurity(struct inode *inode,
>   	struct super_block *sbp;
>   	struct inode *ip = (struct inode *)inode;
>   	struct smack_known *isp;
> -	int ilen;
> -	int rc = 0;
>   
> -	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
> +	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0)
>   		isp = smk_of_inode(inode);
> -		ilen = strlen(isp->smk_known);
> -		*buffer = isp->smk_known;
> -		return ilen;
> -	}
> +	else {
> +		/*
> +		 * The rest of the Smack xattrs are only on sockets.
> +		 */
> +		sbp = ip->i_sb;
> +		if (sbp->s_magic != SOCKFS_MAGIC)
> +			return -EOPNOTSUPP;
>   
> -	/*
> -	 * The rest of the Smack xattrs are only on sockets.
> -	 */
> -	sbp = ip->i_sb;
> -	if (sbp->s_magic != SOCKFS_MAGIC)
> -		return -EOPNOTSUPP;
> +		sock = SOCKET_I(ip);
> +		if (sock == NULL || sock->sk == NULL)
> +			return -EOPNOTSUPP;
>   
> -	sock = SOCKET_I(ip);
> -	if (sock == NULL || sock->sk == NULL)
> -		return -EOPNOTSUPP;
> -
> -	ssp = sock->sk->sk_security;
> +		ssp = sock->sk->sk_security;
>   
> -	if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> -		isp = ssp->smk_in;
> -	else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
> -		isp = ssp->smk_out;
> -	else
> -		return -EOPNOTSUPP;
> +		if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> +			isp = ssp->smk_in;
> +		else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
> +			isp = ssp->smk_out;
> +		else
> +			return -EOPNOTSUPP;
> +	}
>   
> -	ilen = strlen(isp->smk_known);
> -	if (rc == 0) {
> -		*buffer = isp->smk_known;
> -		rc = ilen;
> +	if (alloc) {
> +		*buffer = kstrdup(isp->smk_known, GFP_KERNEL);
> +		if (*buffer == NULL)
> +			return -ENOMEM;
>   	}
>   
> -	return rc;
> +	return strlen(isp->smk_known);
>   }
>   
>   
> 

[PATCH] fix security_release_secctx seems broken

[Konstantin Khlebnikov]

On 19.09.2017 19:39, Casey Schaufler wrote:
> Subject: [PATCH] fix security_release_secctx seems broken
> 
> security_inode_getsecurity() provides the text string value
> of a security attribute. It does not provide a "secctx".
> The code in xattr_getsecurity() that calls security_inode_getsecurity()
> and then calls security_release_secctx() happened to work because
> SElinux and Smack treat the attribute and the secctx the same way.
> It fails for cap_inode_getsecurity(), because that module has no
> secctx that ever needs releasing. It turns out that Smack is the
> one that's doing things wrong by not allocating memory when instructed
> to do so by the "alloc" parameter.
> 
> The fix is simple enough. Change the security_release_secctx() to
> kfree() because it isn't a secctx being returned by
> security_inode_getsecurity(). Change Smack to allocate the string when
> told to do so.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good for me.

Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>


> 
> ---
>   fs/xattr.c                 |  2 +-
>   security/smack/smack_lsm.c | 55 +++++++++++++++++++++-------------------------
>   2 files changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4424f7fecf14..61cd28ba25f3 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -250,7 +250,7 @@ xattr_getsecurity(struct inode *inode, const char *name, void *value,
>   	}
>   	memcpy(value, buffer, len);
>   out:
> -	security_release_secctx(buffer, len);
> +	kfree(buffer);
>   out_noalloc:
>   	return len;
>   }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 319add31b4a4..286171a16ed2 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1473,7 +1473,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
>    * @inode: the object
>    * @name: attribute name
>    * @buffer: where to put the result
> - * @alloc: unused
> + * @alloc: duplicate memory
>    *
>    * Returns the size of the attribute or an error code
>    */
> @@ -1486,43 +1486,38 @@ static int smack_inode_getsecurity(struct inode *inode,
>   	struct super_block *sbp;
>   	struct inode *ip = (struct inode *)inode;
>   	struct smack_known *isp;
> -	int ilen;
> -	int rc = 0;
>   
> -	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
> +	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0)
>   		isp = smk_of_inode(inode);
> -		ilen = strlen(isp->smk_known);
> -		*buffer = isp->smk_known;
> -		return ilen;
> -	}
> +	else {
> +		/*
> +		 * The rest of the Smack xattrs are only on sockets.
> +		 */
> +		sbp = ip->i_sb;
> +		if (sbp->s_magic != SOCKFS_MAGIC)
> +			return -EOPNOTSUPP;
>   
> -	/*
> -	 * The rest of the Smack xattrs are only on sockets.
> -	 */
> -	sbp = ip->i_sb;
> -	if (sbp->s_magic != SOCKFS_MAGIC)
> -		return -EOPNOTSUPP;
> +		sock = SOCKET_I(ip);
> +		if (sock == NULL || sock->sk == NULL)
> +			return -EOPNOTSUPP;
>   
> -	sock = SOCKET_I(ip);
> -	if (sock == NULL || sock->sk == NULL)
> -		return -EOPNOTSUPP;
> -
> -	ssp = sock->sk->sk_security;
> +		ssp = sock->sk->sk_security;
>   
> -	if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> -		isp = ssp->smk_in;
> -	else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
> -		isp = ssp->smk_out;
> -	else
> -		return -EOPNOTSUPP;
> +		if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> +			isp = ssp->smk_in;
> +		else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
> +			isp = ssp->smk_out;
> +		else
> +			return -EOPNOTSUPP;
> +	}
>   
> -	ilen = strlen(isp->smk_known);
> -	if (rc == 0) {
> -		*buffer = isp->smk_known;
> -		rc = ilen;
> +	if (alloc) {
> +		*buffer = kstrdup(isp->smk_known, GFP_KERNEL);
> +		if (*buffer == NULL)
> +			return -ENOMEM;
>   	}
>   
> -	return rc;
> +	return strlen(isp->smk_known);
>   }
>   
>   
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix security_release_secctx seems broken

[James Morris]

On Tue, 19 Sep 2017, Casey Schaufler wrote:

> Subject: [PATCH] fix security_release_secctx seems broken
> 
> security_inode_getsecurity() provides the text string value
> of a security attribute. It does not provide a "secctx".
> The code in xattr_getsecurity() that calls security_inode_getsecurity()
> and then calls security_release_secctx() happened to work because
> SElinux and Smack treat the attribute and the secctx the same way.
> It fails for cap_inode_getsecurity(), because that module has no
> secctx that ever needs releasing. It turns out that Smack is the
> one that's doing things wrong by not allocating memory when instructed
> to do so by the "alloc" parameter.
> 
> The fix is simple enough. Change the security_release_secctx() to
> kfree() because it isn't a secctx being returned by
> security_inode_getsecurity(). Change Smack to allocate the string when
> told to do so.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good to me.  I wonder why security_release_secctx was used in the 
first place? (it arrived via commit 42492594)

Konstantin: how did you trigger this?

I plan to send this to Linus for -rc4 unless anyone has objections.


-- 
James Morris
<jmorris@namei.org>

[PATCH] fix security_release_secctx seems broken

[James Morris]

On Tue, 19 Sep 2017, Casey Schaufler wrote:

> Subject: [PATCH] fix security_release_secctx seems broken
> 
> security_inode_getsecurity() provides the text string value
> of a security attribute. It does not provide a "secctx".
> The code in xattr_getsecurity() that calls security_inode_getsecurity()
> and then calls security_release_secctx() happened to work because
> SElinux and Smack treat the attribute and the secctx the same way.
> It fails for cap_inode_getsecurity(), because that module has no
> secctx that ever needs releasing. It turns out that Smack is the
> one that's doing things wrong by not allocating memory when instructed
> to do so by the "alloc" parameter.
> 
> The fix is simple enough. Change the security_release_secctx() to
> kfree() because it isn't a secctx being returned by
> security_inode_getsecurity(). Change Smack to allocate the string when
> told to do so.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good to me.  I wonder why security_release_secctx was used in the 
first place? (it arrived via commit 42492594)

Konstantin: how did you trigger this?

I plan to send this to Linus for -rc4 unless anyone has objections.


-- 
James Morris
<jmorris@namei.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix security_release_secctx seems broken

[Konstantin Khlebnikov]

On 04.10.2017 09:17, James Morris wrote:
> On Tue, 19 Sep 2017, Casey Schaufler wrote:
> 
>> Subject: [PATCH] fix security_release_secctx seems broken
>>
>> security_inode_getsecurity() provides the text string value
>> of a security attribute. It does not provide a "secctx".
>> The code in xattr_getsecurity() that calls security_inode_getsecurity()
>> and then calls security_release_secctx() happened to work because
>> SElinux and Smack treat the attribute and the secctx the same way.
>> It fails for cap_inode_getsecurity(), because that module has no
>> secctx that ever needs releasing. It turns out that Smack is the
>> one that's doing things wrong by not allocating memory when instructed
>> to do so by the "alloc" parameter.
>>
>> The fix is simple enough. Change the security_release_secctx() to
>> kfree() because it isn't a secctx being returned by
>> security_inode_getsecurity(). Change Smack to allocate the string when
>> told to do so.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> 
> Looks good to me.  I wonder why security_release_secctx was used in the
> first place? (it arrived via commit 42492594)
>  > Konstantin: how did you trigger this?

Just "getcap /bin/ping" is enough to tigger leak if file has capabilities.
Selinux shouldn't be loaded because its release_secctx hook call kfree.

But sometimes it takes some time for kmemleak to find leak. Presumably
because stale poiner stays on stack which could be reused nowdays.

> 
> I plan to send this to Linus for -rc4 unless anyone has objections.
> 
> 

[PATCH] fix security_release_secctx seems broken

[Konstantin Khlebnikov]

On 04.10.2017 09:17, James Morris wrote:
> On Tue, 19 Sep 2017, Casey Schaufler wrote:
> 
>> Subject: [PATCH] fix security_release_secctx seems broken
>>
>> security_inode_getsecurity() provides the text string value
>> of a security attribute. It does not provide a "secctx".
>> The code in xattr_getsecurity() that calls security_inode_getsecurity()
>> and then calls security_release_secctx() happened to work because
>> SElinux and Smack treat the attribute and the secctx the same way.
>> It fails for cap_inode_getsecurity(), because that module has no
>> secctx that ever needs releasing. It turns out that Smack is the
>> one that's doing things wrong by not allocating memory when instructed
>> to do so by the "alloc" parameter.
>>
>> The fix is simple enough. Change the security_release_secctx() to
>> kfree() because it isn't a secctx being returned by
>> security_inode_getsecurity(). Change Smack to allocate the string when
>> told to do so.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> 
> Looks good to me.  I wonder why security_release_secctx was used in the
> first place? (it arrived via commit 42492594)
>  > Konstantin: how did you trigger this?

Just "getcap /bin/ping" is enough to tigger leak if file has capabilities.
Selinux shouldn't be loaded because its release_secctx hook call kfree.

But sometimes it takes some time for kmemleak to find leak. Presumably
because stale poiner stays on stack which could be reused nowdays.

> 
> I plan to send this to Linus for -rc4 unless anyone has objections.
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix security_release_secctx seems broken

[James Morris]

On Wed, 4 Oct 2017, Konstantin Khlebnikov wrote:

> Just "getcap /bin/ping" is enough to tigger leak if file has capabilities.
> Selinux shouldn't be loaded because its release_secctx hook call kfree.

Ahh, makes sense.

> 
> But sometimes it takes some time for kmemleak to find leak. Presumably
> because stale poiner stays on stack which could be reused nowdays.

Thanks for finding this!


-- 
James Morris
<jmorris@namei.org>

[PATCH] fix security_release_secctx seems broken

[James Morris]

On Wed, 4 Oct 2017, Konstantin Khlebnikov wrote:

> Just "getcap /bin/ping" is enough to tigger leak if file has capabilities.
> Selinux shouldn't be loaded because its release_secctx hook call kfree.

Ahh, makes sense.

> 
> But sometimes it takes some time for kmemleak to find leak. Presumably
> because stale poiner stays on stack which could be reused nowdays.

Thanks for finding this!


-- 
James Morris
<jmorris@namei.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html