Skip to content

Commit 4e638be

Browse files
jtnordjulieheard
authored andcommitted
[SECURITY-3103]
1 parent d2f32a1 commit 4e638be

File tree

6 files changed

+130
-29
lines changed

6 files changed

+130
-29
lines changed

.gitignore

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
target
22
work
3-
3+
/.classpath
4+
/.project
5+
/.settings/
46
.idea
57
*.iml
68
.*.sw*

README.md

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,17 @@ The first, and simpler, security system is to allow any kind of script to be run
2222
with an administrator’s approval. There is a globally maintained list of approved scripts
2323
which are judged to not perform any malicious actions.
2424

25-
When an administrator saves some kind of configuration (for example, a job), those scripts
26-
that were edited by admin are automatically approved and are ready to run with no further
27-
intervention. For scripts that were submitted by lower privileged users there will be
25+
When a user saves some kind of configuration (for example, a job), there will be
2826
appropriate warnings indicating that approval is required. Administrators may approve those
29-
scripts using the Script Approval configuration page or by editing the script and saving it.
30-
In previous versions of Script Security Plugin, administrators could automatically approve
31-
scripts submitted by unprivileged users by saving them without making any changes, but this
32-
functionality was disabled to prevent social engineering-based attacks. (“Saving” usually
33-
means from the web UI, but could also mean uploading a new XML configuration via REST or CLI.)
34-
35-
When a non-administrator saves a template configuration, a check is done whether any
27+
scripts using the Script Approval configuration page or following the approval link in the
28+
configuration.
29+
In previous versions of Script Security Plugin, scripts saved by administrators where
30+
automatically approved when saving them, but this functionality was disabled to prevent
31+
a variety of social engineering-based attacks. (“Saving” usually means from the web UI, but
32+
could also mean uploading a new XML configuration via REST or CLI.) or merely by creating a
33+
new item copying an existing one.
34+
35+
When a user saves a template configuration, a check is done whether any
3636
contained scripts have been edited from an approved text. (More precisely, whether the
3737
requested content has ever been approved before.) If it has not been approved, a request
3838
for approval of this script is added to a queue. (A warning is also displayed in the

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
2828
import hudson.model.BallColor;
29+
import hudson.model.PageDecorator;
2930
import hudson.security.ACLContext;
3031
import jenkins.model.GlobalConfiguration;
3132
import jenkins.model.GlobalConfigurationCategory;
@@ -82,7 +83,9 @@
8283
import org.kohsuke.accmod.restrictions.NoExternalUse;
8384
import org.kohsuke.stapler.DataBoundConstructor;
8485
import org.kohsuke.stapler.DataBoundSetter;
86+
import org.kohsuke.stapler.QueryParameter;
8587
import org.kohsuke.stapler.bind.JavaScriptMethod;
88+
import org.kohsuke.stapler.verb.POST;
8689

8790
/**
8891
* Manages approved scripts.
@@ -95,6 +98,10 @@ public class ScriptApproval extends GlobalConfiguration implements RootAction {
9598
public static /* non-final */ boolean ADMIN_AUTO_APPROVAL_ENABLED =
9699
SystemProperties.getBoolean(ScriptApproval.class.getName() + ".ADMIN_AUTO_APPROVAL_ENABLED");
97100

101+
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "for script console")
102+
public static /* non-final */ boolean ALLOW_ADMIN_APPROVAL_ENABLED =
103+
SystemProperties.getBoolean(ScriptApproval.class.getName() + ".ALLOW_ADMIN_APPROVAL_ENABLED");
104+
98105
private static final Logger LOG = Logger.getLogger(ScriptApproval.class.getName());
99106

100107
private static final XStream2 XSTREAM2 = new XStream2();
@@ -593,8 +600,9 @@ public synchronized String configuring(@NonNull String script, @NonNull Language
593600
final ConversionCheckResult result = checkAndConvertApprovedScript(script, language);
594601
if (!result.approved) {
595602
if (!Jenkins.get().isUseSecurity() ||
603+
(ALLOW_ADMIN_APPROVAL_ENABLED &&
596604
((Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER))
597-
&& (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin))) {
605+
&& (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin)))) {
598606
approvedScriptHashes.add(result.newHash);
599607
//Pending scripts are not stored with a precalculated hash, so no need to remove any old hashes
600608
removePendingScript(result.newHash);
@@ -768,16 +776,24 @@ public synchronized FormValidation checking(@NonNull String script, @NonNull Lan
768776
if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER)) {
769777
return FormValidation.warningWithMarkup("A Jenkins administrator will need to approve this script before it can be used");
770778
} else {
771-
if (willBeApproved || ADMIN_AUTO_APPROVAL_ENABLED) {
772-
return FormValidation.ok("The script has not yet been approved, but it will be approved on save");
779+
if ((ALLOW_ADMIN_APPROVAL_ENABLED && (willBeApproved || ADMIN_AUTO_APPROVAL_ENABLED)) || !Jenkins.get().isUseSecurity()) {
780+
return FormValidation.okWithMarkup("The script has not yet been approved, but it will be approved on save.");
773781
}
774-
782+
String approveScript = "<a class='jenkins-button script-approval-approve-link' data-base-url='" + Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "' data-hash='" + result.newHash + "'>Approve script</a>";
775783
return FormValidation.okWithMarkup("The script is not approved and will not be approved on save. " +
776-
"Modify the script to approve it on save, or approve it explicitly on the " +
777-
"<a target='blank' href='"+ Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "'>Script Approval Configuration</a> page");
784+
"Either modify the script to match an already approved script, approve it explicitly on the " +
785+
"<a target='blank' href='"+ Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "'>Script Approval Configuration</a> page after save, or approve this version of the script. " +
786+
approveScript);
778787
}
779788
}
780789

790+
@Restricted(NoExternalUse.class)
791+
@POST
792+
// can not call this method doApproveScript as that collides with the javascript binding in #approveScript
793+
public synchronized void doApproveScriptHash(@QueryParameter(required=true) String hash) throws IOException {
794+
approveScript(hash);
795+
}
796+
781797
/**
782798
* @deprecated Use {@link #checking(String, Language, boolean)} instead
783799
*/
@@ -1235,4 +1251,7 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException {
12351251
return getClasspathRenderInfo();
12361252
}
12371253

1254+
@Restricted(NoExternalUse.class)
1255+
@Extension
1256+
public static class FormValidationPageDecorator extends PageDecorator {}
12381257
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?jelly escape-by-default='true'?>
2+
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
3+
<st:adjunct includes="org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.FormValidationPageDecorator.validate" />
4+
</j:jelly>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
Behaviour.specify('.script-approval-approve-link', 'ScriptApproval.FormValidationPageDecorator', 0, function (element) {
2+
element.onclick = function (ev) {
3+
const approvalUrl = ev.target.dataset.baseUrl;
4+
const hash = ev.target.dataset.hash;
5+
const xmlhttp = new XMLHttpRequest();
6+
xmlhttp.onload = function () {
7+
alert('Script approved');
8+
};
9+
xmlhttp.open('POST', approvalUrl + "/approveScriptHash");
10+
const data = new FormData();
11+
data.append('hash', hash);
12+
xmlhttp.setRequestHeader(crumb.fieldName, crumb.value);
13+
xmlhttp.send(data);
14+
}
15+
});

src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java

Lines changed: 73 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424

2525
package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;
2626

27+
28+
import com.gargoylesoftware.htmlunit.CollectingAlertHandler;
2729
import com.gargoylesoftware.htmlunit.html.HtmlCheckBoxInput;
2830
import com.gargoylesoftware.htmlunit.html.HtmlInput;
2931
import groovy.lang.Binding;
@@ -61,7 +63,12 @@
6163
import org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedUsageException;
6264

6365
import static org.hamcrest.MatcherAssert.assertThat;
66+
import static org.hamcrest.Matchers.arrayWithSize;
67+
import static org.hamcrest.Matchers.contains;
6468
import static org.hamcrest.Matchers.containsString;
69+
import static org.hamcrest.Matchers.empty;
70+
import static org.hamcrest.Matchers.emptyArray;
71+
import static org.hamcrest.Matchers.is;
6572
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted;
6673
import static org.junit.Assert.assertEquals;
6774
import static org.junit.Assert.assertFalse;
@@ -163,7 +170,7 @@ public class SecureGroovyScriptTest {
163170

164171

165172
/**
166-
* Test where the user has ADMINISTER privs, default to non sandbox mode.
173+
* Test where the user has ADMINISTER privs, default to non sandbox mode, but require approval
167174
*/
168175
@Test public void testSandboxDefault_with_ADMINISTER_privs() throws Exception {
169176
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
@@ -198,12 +205,12 @@ public class SecureGroovyScriptTest {
198205
// The user has ADMINISTER privs => should default to non sandboxed
199206
assertFalse(publisher.getScript().isSandbox());
200207

201-
// Because it has ADMINISTER privs, the script should not have ended up pending approval
208+
// even though it has ADMINISTER privs, the script should still require approval
202209
Set<ScriptApproval.PendingScript> pendingScripts = ScriptApproval.get().getPendingScripts();
203-
assertEquals(0, pendingScripts.size());
210+
assertEquals(1, pendingScripts.size());
204211

205212
// Test that the script is executable. If it's not, we will get an UnapprovedUsageException
206-
assertEquals(groovy, ScriptApproval.get().using(groovy, GroovyLanguage.get()));
213+
assertThrows(UnapprovedUsageException.class, () -> ScriptApproval.get().using(groovy, GroovyLanguage.get()));
207214
}
208215

209216
/**
@@ -892,7 +899,7 @@ public void testScriptApproval() throws Exception {
892899

893900
JenkinsRule.WebClient wc = r.createWebClient();
894901

895-
// If configured by a user with ADMINISTER script is approved if edited by that user
902+
// If configured by a user with ADMINISTER script is not approved and approval is requested
896903
{
897904
wc.login("admin");
898905
HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
@@ -903,8 +910,9 @@ public void testScriptApproval() throws Exception {
903910
script.setText(groovy);
904911
r.submit(config);
905912

906-
assertTrue(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
907-
913+
assertFalse(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
914+
assertEquals(1, ScriptApproval.get().getPendingScripts().size());
915+
908916
// clean up for next tests
909917
ScriptApproval.get().preapproveAll();
910918
ScriptApproval.get().clearApprovedScripts();
@@ -929,11 +937,14 @@ public void testScriptApproval() throws Exception {
929937
ScriptApproval.get().clearApprovedScripts();
930938
}
931939

932-
// If configured by a user with ADMINISTER while escape hatch is on script is approved upon save
940+
// If configured by a user with ADMINISTER while escape hatches are on script is approved upon save
933941
{
934942
wc.login("admin");
935-
boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
943+
boolean originalAdminAutoApprove = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
936944
ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true;
945+
boolean originalAllowAdminApproval = ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED;
946+
ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = true;
947+
937948
try {
938949
HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
939950
List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
@@ -948,15 +959,18 @@ public void testScriptApproval() throws Exception {
948959
ScriptApproval.get().preapproveAll();
949960
ScriptApproval.get().clearApprovedScripts();
950961
} finally {
951-
ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original;
962+
ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = originalAdminAutoApprove;
963+
ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = originalAllowAdminApproval;
952964
}
953965
}
954966

955967
// If configured by a user without ADMINISTER while escape hatch is on script is not approved
956968
{
957969
wc.login("devel");
958-
boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
970+
boolean originalAdminAutoApprove = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
959971
ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true;
972+
boolean originalAllowAdminApproval = ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED;
973+
ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = true;
960974
try {
961975
r.submit(wc.getPage(p, "configure").getFormByName("config"));
962976

@@ -966,7 +980,8 @@ public void testScriptApproval() throws Exception {
966980
ScriptApproval.get().preapproveAll();
967981
ScriptApproval.get().clearApprovedScripts();
968982
} finally {
969-
ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original;
983+
ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = originalAdminAutoApprove;
984+
ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = originalAllowAdminApproval;
970985
}
971986
}
972987
}
@@ -1271,6 +1286,52 @@ public void testScriptAtFieldInitializers() throws Exception {
12711286
r.assertLogContains("new java.io.File java.lang.String", b);
12721287
}
12731288

1289+
@Test public void testApprovalFromFormValidation() throws Exception {
1290+
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
1291+
MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
1292+
mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
1293+
for (Permission p : Item.PERMISSIONS.getPermissions()) {
1294+
mockStrategy.grant(p).everywhere().to("admin");
1295+
}
1296+
r.jenkins.setAuthorizationStrategy(mockStrategy);
1297+
1298+
FreeStyleProject p = r.createFreeStyleProject("p");
1299+
try (JenkinsRule.WebClient wc = r.createWebClient()) {
1300+
CollectingAlertHandler altertHandler = new CollectingAlertHandler();
1301+
wc.setAlertHandler(altertHandler);
1302+
1303+
wc.login("admin");
1304+
HtmlPage page = wc.getPage(p, "configure");
1305+
HtmlForm config = page.getFormByName("config");
1306+
HtmlFormUtil.getButtonByCaption(config, "Add post-build action").click(); // lib/hudson/project/config-publishers2.jelly
1307+
page.getAnchorByText(r.jenkins.getExtensionList(BuildStepDescriptor.class).get(TestGroovyRecorder.DescriptorImpl.class).getDisplayName()).click();
1308+
wc.waitForBackgroundJavaScript(10000);
1309+
List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
1310+
// Get the last one, because previous ones might be from Lockable Resources during PCT.
1311+
HtmlTextArea script = scripts.get(scripts.size() - 1);
1312+
String groovy = "build.externalizableId";
1313+
script.setText(groovy);
1314+
// nothing is approved or pending (no save)
1315+
assertThat(ScriptApproval.get().getPendingScripts(), is(empty()));
1316+
assertThat(ScriptApproval.get().getApprovedScriptHashes(), is(emptyArray()));
1317+
1318+
wc.waitForBackgroundJavaScript(10_000); // FormValidation to display
1319+
1320+
page.getAnchorByText("Approve script").click();
1321+
1322+
wc.waitForBackgroundJavaScript(10_000); // wait for the ajax approval to complete
1323+
1324+
assertThat(altertHandler.getCollectedAlerts(), contains("Script approved"));
1325+
// script is approved
1326+
assertThat(ScriptApproval.get().getPendingScripts(), is(empty()));
1327+
assertThat(ScriptApproval.get().getApprovedScriptHashes(), is(arrayWithSize(1)));
1328+
1329+
r.submit(config);
1330+
1331+
FreeStyleBuild b = r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0));
1332+
}
1333+
}
1334+
12741335
public static class HasMainMethod {
12751336
@Whitelisted
12761337
public HasMainMethod() { }

0 commit comments

Comments
 (0)