Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add threadgroup isolation. #14353

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@rmuir
Copy link
Contributor

commented Oct 29, 2015

Code with modifyThread and modifyThreadGroup may only modify
its own threadgroup (or an ancestor of that). This enforces
what is intended by the ThreadGroup class.

Today there are only two thread groups: "system" (Java) and "main" (ES).

Adding this isolation has two new immediate implications:

  1. Code without permissions (scripts) may not create or mess with threads at all.
  2. ES application threads cannot mess with Java system threads at all.

ES puts all application threads in one single group today, but in the future
this can be organized better, and we will have more isolation in the system.

NOTE: implementation-wise, this means our SM impl needs to be privileged, so that
it can properly do access checks. I organized this here (https://github.com/rmuir/securesm/), we can move it to an elastic repository. It has its own tests. It needs to be a separate jar just because how security checks work in java, and being completely separate means no problems with IDEs, "reactor builds" or what have you. See the javadocs there for more details.

Add threadgroup isolation.
Code with `modifyThread` and `modifyThreadGroup` may only modify
its own threadgroup (or an ancestor of that). This enforces
what is intended by the ThreadGroup class.

This has two immediate implications:
1. Code without these permissions (scripts) may not create or mess with threads
2. ES application threads cannot mess with Java system threads

ES puts all application threads in one single group today, but in the future
this can be organized better, and we will have more isolation in the system.
@jpountz

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2015

LGTM

we can move it to an elastic repository

+1

@jasontedor

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Because of the POM changes, the addition of the securesm artifact and the imminent Gradle transition, I think that @rjernst will want to coordinate with you on integrating to master?

LGTM. This is incredibly important work.

@@ -107,6 +107,9 @@ public void testEvilGroovyScripts() throws Exception {
// AccessControlException[access denied ("java.io.FilePermission" "<<ALL FILES>>" "execute")]
assertFailure("def methodName = 'ex'; Runtime.\"${'get' + 'Runtime'}\"().\"${methodName}ec\"(\"touch /tmp/gotcha2\")");

// AccessControlException[access denied ("java.lang.RuntimePermission" "modifyThreadGroup")]
assertFailure("t = new Thread({ println 3 }); t.start(); t.join()");

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 29, 2015

Contributor

can you add a comment the we are not allowed to start a thread anymore and maybe remove the t.join() since it's confusing to me why it needs this

This comment has been minimized.

Copy link
@rmuir

rmuir Oct 29, 2015

Author Contributor

sure, i took the test from the old groovy sandboxing tests. but it will fail in the Thread ctor (which checks for access to modify the thread group) so we don't even need the start() either...

@rjernst

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

This should be easy to merge into the gradle branch right away. LGTM, push away.

@rmuir rmuir closed this in 1194cd0 Oct 29, 2015

rmuir added a commit that referenced this pull request Oct 29, 2015

Add threadgroup isolation.
Closes #14353

Squashed commit of the following:

commit edae0729f71ea3d3f9fa9c0d27c9effc042eb5a9
Author: Robert Muir <rmuir@apache.org>
Date:   Thu Oct 29 14:13:42 2015 -0400

    update sha1 and simplify test

commit 635c4f2
Author: Robert Muir <rmuir@apache.org>
Date:   Thu Oct 29 07:01:26 2015 -0400

    Add threadgroup isolation.

    Code with `modifyThread` and `modifyThreadGroup` may only modify
    its own threadgroup (or an ancestor of that). This enforces
    what is intended by the ThreadGroup class.

    This has two immediate implications:
    1. Code without these permissions (scripts) may not create or mess with threads
    2. ES application threads cannot mess with Java system threads

    ES puts all application threads in one single group today, but in the future
    this can be organized better, and we will have more isolation in the system.

Conflicts:
	core/pom.xml
	plugins/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.