SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] selinux: fix context string corruption in convert_context()
@ 2019-10-03 13:59 Ondrej Mosnacek
  2019-10-03 16:52 ` Stephen Smalley
  2019-10-03 18:54 ` Paul Moore
  0 siblings, 2 replies; 3+ messages in thread
From: Ondrej Mosnacek @ 2019-10-03 13:59 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Stephen Smalley, Milos Malik

string_to_context_struct() may garble the context string, so we need to
copy back the contents again from the old context struct to avoid
storing the corrupted context.

Since string_to_context_struct() tokenizes (and therefore truncates) the
context string and we are later potentially copying it with kstrdup(),
this may eventually cause pieces of uninitialized kernel memory to be
disclosed to userspace (when copying to userspace based on the stored
length and not the null character).

How to reproduce on Fedora and similar:
    # dnf install -y memcached
    # systemctl start memcached
    # semodule -d memcached
    # load_policy
    # load_policy
    # systemctl stop memcached
    # ausearch -m AVC
    type=AVC msg=audit(1570090572.648:313): avc:  denied  { signal } for  pid=1 comm="systemd" scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:unlabeled_t:s0 tclass=process permissive=0 trawcon=73797374656D5F75007400000000000070BE6E847296FFFF726F6D000096FFFF76

Reported-by: Milos Malik <mmalik@redhat.com>
Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/services.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 3a29e7c24ba9..a5813c7629c1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1946,7 +1946,14 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
 		rc = string_to_context_struct(args->newp, NULL, s,
 					      newc, SECSID_NULL);
 		if (rc == -EINVAL) {
-			/* Retain string representation for later mapping. */
+			/*
+			 * Retain string representation for later mapping.
+			 *
+			 * IMPORTANT: We need to copy the contents of oldc->str
+			 * back into s again because string_to_context_struct()
+			 * may have garbled it.
+			 */
+			memcpy(s, oldc->str, oldc->len);
 			context_init(newc);
 			newc->str = s;
 			newc->len = oldc->len;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] selinux: fix context string corruption in convert_context()
  2019-10-03 13:59 [PATCH] selinux: fix context string corruption in convert_context() Ondrej Mosnacek
@ 2019-10-03 16:52 ` Stephen Smalley
  2019-10-03 18:54 ` Paul Moore
  1 sibling, 0 replies; 3+ messages in thread
From: Stephen Smalley @ 2019-10-03 16:52 UTC (permalink / raw)
  To: Ondrej Mosnacek, selinux, Paul Moore; +Cc: Milos Malik

On 10/3/19 9:59 AM, Ondrej Mosnacek wrote:
> string_to_context_struct() may garble the context string, so we need to
> copy back the contents again from the old context struct to avoid
> storing the corrupted context.
> 
> Since string_to_context_struct() tokenizes (and therefore truncates) the
> context string and we are later potentially copying it with kstrdup(),
> this may eventually cause pieces of uninitialized kernel memory to be
> disclosed to userspace (when copying to userspace based on the stored
> length and not the null character).
> 
> How to reproduce on Fedora and similar:
>      # dnf install -y memcached
>      # systemctl start memcached
>      # semodule -d memcached
>      # load_policy
>      # load_policy
>      # systemctl stop memcached
>      # ausearch -m AVC
>      type=AVC msg=audit(1570090572.648:313): avc:  denied  { signal } for  pid=1 comm="systemd" scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:unlabeled_t:s0 tclass=process permissive=0 trawcon=73797374656D5F75007400000000000070BE6E847296FFFF726F6D000096FFFF76
> 
> Reported-by: Milos Malik <mmalik@redhat.com>
> Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/ss/services.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 3a29e7c24ba9..a5813c7629c1 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1946,7 +1946,14 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
>   		rc = string_to_context_struct(args->newp, NULL, s,
>   					      newc, SECSID_NULL);
>   		if (rc == -EINVAL) {
> -			/* Retain string representation for later mapping. */
> +			/*
> +			 * Retain string representation for later mapping.
> +			 *
> +			 * IMPORTANT: We need to copy the contents of oldc->str
> +			 * back into s again because string_to_context_struct()
> +			 * may have garbled it.
> +			 */
> +			memcpy(s, oldc->str, oldc->len);
>   			context_init(newc);
>   			newc->str = s;
>   			newc->len = oldc->len;
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] selinux: fix context string corruption in convert_context()
  2019-10-03 13:59 [PATCH] selinux: fix context string corruption in convert_context() Ondrej Mosnacek
  2019-10-03 16:52 ` Stephen Smalley
@ 2019-10-03 18:54 ` Paul Moore
  1 sibling, 0 replies; 3+ messages in thread
From: Paul Moore @ 2019-10-03 18:54 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley, Milos Malik

On Thu, Oct 3, 2019 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> string_to_context_struct() may garble the context string, so we need to
> copy back the contents again from the old context struct to avoid
> storing the corrupted context.
>
> Since string_to_context_struct() tokenizes (and therefore truncates) the
> context string and we are later potentially copying it with kstrdup(),
> this may eventually cause pieces of uninitialized kernel memory to be
> disclosed to userspace (when copying to userspace based on the stored
> length and not the null character).
>
> How to reproduce on Fedora and similar:
>     # dnf install -y memcached
>     # systemctl start memcached
>     # semodule -d memcached
>     # load_policy
>     # load_policy
>     # systemctl stop memcached
>     # ausearch -m AVC
>     type=AVC msg=audit(1570090572.648:313): avc:  denied  { signal } for  pid=1 comm="systemd" scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:unlabeled_t:s0 tclass=process permissive=0 trawcon=73797374656D5F75007400000000000070BE6E847296FFFF726F6D000096FFFF76
>
> Reported-by: Milos Malik <mmalik@redhat.com>
> Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/services.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Thanks for finding and fixing this.  This looks like a good candidate
for stable so I went ahead and merged it into selinux/stable-5.4; if
any one has any objections to that, let me know by the end of the
week.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03 13:59 [PATCH] selinux: fix context string corruption in convert_context() Ondrej Mosnacek
2019-10-03 16:52 ` Stephen Smalley
2019-10-03 18:54 ` Paul Moore

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git