[PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()

[Paul Moore]

From: Paul Moore <***@paul-moore.com>

The syzbot/syzkaller automated tests found a problem in
security_context_to_sid_core() during early boot (before we load the
SELinux policy) where we could potentially feed context strings without
NULL terminators into the strcmp() function.

We already guard against this during normal operation (after the SELinux
policy has been loaded) by making a copy of the context strings and
explicitly adding a NULL terminator to the end. The patch extends this
protection to the early boot case (no loaded policy) by moving the context
copy earlier in security_context_to_sid_core().

Reported-by: syzbot <***@googlegroups.com>
Signed-off-by: Paul Moore <***@paul-moore.com>
---
security/selinux/ss/services.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 33cfe5d3d6cb..6ec4cdc8af8f 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1413,27 +1413,27 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
if (!scontext_len)
return -EINVAL;

+ /* Copy the string to allow changes and ensure a NULL terminator */
+ scontext2 = kmalloc(scontext_len + 1, gfp_flags);
+ if (!scontext2)
+ return -ENOMEM;
+ memcpy(scontext2, scontext, scontext_len);
+ scontext2[scontext_len] = 0;
+
if (!ss_initialized) {
int i;

for (i = 1; i < SECINITSID_NUM; i++) {
- if (!strcmp(initial_sid_to_string[i], scontext)) {
+ if (!strcmp(initial_sid_to_string[i], scontext2)) {
*sid = i;
- return 0;
+ goto out;
}
}
*sid = SECINITSID_KERNEL;
- return 0;
+ goto out;
}
*sid = SECSID_NULL;

- /* Copy the string so that we can modify the copy as we parse it. */
- scontext2 = kmalloc(scontext_len + 1, gfp_flags);
- if (!scontext2)
- return -ENOMEM;
- memcpy(scontext2, scontext, scontext_len);
- scontext2[scontext_len] = 0;
-
if (force) {
/* Save another copy for storing in uninterpreted form */
rc = -ENOMEM;

[William Roberts]

Post by Paul Moore
The syzbot/syzkaller automated tests found a problem in
security_context_to_sid_core() during early boot (before we load the
SELinux policy) where we could potentially feed context strings without
NULL terminators into the strcmp() function.
We already guard against this during normal operation (after the SELinux
policy has been loaded) by making a copy of the context strings and
explicitly adding a NULL terminator to the end. The patch extends this
protection to the early boot case (no loaded policy) by moving the context
copy earlier in security_context_to_sid_core().
---
security/selinux/ss/services.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 33cfe5d3d6cb..6ec4cdc8af8f 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1413,27 +1413,27 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
if (!scontext_len)
return -EINVAL;
+ /* Copy the string to allow changes and ensure a NULL terminator */
+ scontext2 = kmalloc(scontext_len + 1, gfp_flags);
+ if (!scontext2)
+ return -ENOMEM;
+ memcpy(scontext2, scontext, scontext_len);
+ scontext2[scontext_len] = 0;

Call me crazy, but can't we use kmemdup_nul() here?

/**
* kmemdup_nul - Create a NUL-terminated string from unterminated data
* @s: The data to stringify
* @len: The size of the data
* @gfp: the GFP mask used in the kmalloc() call when allocating memory
*/
char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)

Post by Paul Moore
+
if (!ss_initialized) {
int i;
for (i = 1; i < SECINITSID_NUM; i++) {
- if (!strcmp(initial_sid_to_string[i], scontext)) {
+ if (!strcmp(initial_sid_to_string[i], scontext2)) {
*sid = i;
- return 0;
+ goto out;
}
}
*sid = SECINITSID_KERNEL;
- return 0;
+ goto out;
}
*sid = SECSID_NULL;
- /* Copy the string so that we can modify the copy as we parse it. */
- scontext2 = kmalloc(scontext_len + 1, gfp_flags);
- if (!scontext2)
- return -ENOMEM;
- memcpy(scontext2, scontext, scontext_len);
- scontext2[scontext_len] = 0;
-
if (force) {
/* Save another copy for storing in uninterpreted form */
rc = -ENOMEM;

[Paul Moore]

On Thu, Nov 30, 2017 at 6:44 PM, William Roberts

Post by William Roberts

Post by Paul Moore
The syzbot/syzkaller automated tests found a problem in
security_context_to_sid_core() during early boot (before we load the
SELinux policy) where we could potentially feed context strings without
NULL terminators into the strcmp() function.
We already guard against this during normal operation (after the SELinux
policy has been loaded) by making a copy of the context strings and
explicitly adding a NULL terminator to the end. The patch extends this
protection to the early boot case (no loaded policy) by moving the context
copy earlier in security_context_to_sid_core().
---
security/selinux/ss/services.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 33cfe5d3d6cb..6ec4cdc8af8f 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1413,27 +1413,27 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
if (!scontext_len)
return -EINVAL;
+ /* Copy the string to allow changes and ensure a NULL terminator */
+ scontext2 = kmalloc(scontext_len + 1, gfp_flags);
+ if (!scontext2)
+ return -ENOMEM;
+ memcpy(scontext2, scontext, scontext_len);
+ scontext2[scontext_len] = 0;

Call me crazy, but can't we use kmemdup_nul() here?

Crazy good idea ;)

I didn't realize that function existed, I'll respin. Thanks.

Post by William Roberts
/**
* kmemdup_nul - Create a NUL-terminated string from unterminated data
*/
char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)

[James Morris]

Post by Paul Moore
The syzbot/syzkaller automated tests found a problem in
security_context_to_sid_core() during early boot (before we load the
SELinux policy) where we could potentially feed context strings without
NULL terminators into the strcmp() function.
We already guard against this during normal operation (after the SELinux
policy has been loaded) by making a copy of the context strings and
explicitly adding a NULL terminator to the end. The patch extends this
protection to the early boot case (no loaded policy) by moving the context
copy earlier in security_context_to_sid_core().

Reviewed-by: James Morris <***@oracle.com>