Skip to content
Permalink
Browse files

[FIXED JENKINS-31610] User may view some information in credential-st…

…ore of other users

- Not really considered as a security issue as the user could not view the actual secrets of the credentials, but better to lock it down in any case
  • Loading branch information...
stephenc committed Mar 23, 2016
1 parent b3e7589 commit 3876043c2051fb1afdc9abab89bc6653b7f74ad3
@@ -43,18 +43,9 @@
import hudson.model.UserProperty;
import hudson.model.UserPropertyDescriptor;
import hudson.security.ACL;
import hudson.security.AccessDeniedException2;
import hudson.security.Permission;
import hudson.util.CopyOnWriteMap;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.export.ExportedBean;

import java.io.IOException;
import java.io.ObjectStreamException;
import java.util.ArrayList;
@@ -67,6 +58,17 @@
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.export.ExportedBean;

import static com.cloudbees.plugins.credentials.CredentialsMatchers.always;

@@ -142,14 +144,27 @@
if (user != null) {
UserCredentialsProperty property = user.getProperty(UserCredentialsProperty.class);
if (property != null) {
return DomainCredentials
.getCredentials(property.getDomainCredentialsMap(), type, domainRequirements, always());
// we need to impersonate if the requesting authentication is not the current authentication.
boolean needImpersonation = user.equals(User.current());
SecurityContext old = needImpersonation ? null : ACL.impersonate(user.impersonate());

This comment has been minimized.

Copy link
@jglick
try {
return DomainCredentials
.getCredentials(property.getDomainCredentialsMap(), type, domainRequirements, always());
} finally {
if (needImpersonation) {
// restore the current authentication if we impersonated.
SecurityContextHolder.setContext(old);
}
}
}
}
}
return new ArrayList<C>();
}

/**
* {@inheritDoc}
*/
@Override
public CredentialsStore getStore(@CheckForNull ModelObject object) {
if (object instanceof User) {
@@ -223,6 +238,7 @@ private Object readResolve() throws ObjectStreamException {
* @return the subset of the user's credentials that are of the specified type.
*/
public <C extends Credentials> List<C> getCredentials(Class<C> type) {
checkPermission(CredentialsProvider.VIEW);
List<C> result = new ArrayList<C>();
for (Credentials credential : getCredentials()) {
if (type.isInstance(credential)) {
@@ -239,6 +255,7 @@ private Object readResolve() throws ObjectStreamException {
*/
@SuppressWarnings("unused") // used by stapler
public List<Credentials> getCredentials() {
checkPermission(CredentialsProvider.VIEW);
return domainCredentialsMap.get(Domain.global());
}

@@ -250,6 +267,7 @@ private Object readResolve() throws ObjectStreamException {
*/
@SuppressWarnings("unused") // used by stapler
public List<DomainCredentials> getDomainCredentials() {
checkPermission(CredentialsProvider.VIEW);
return DomainCredentials.asList(getDomainCredentialsMap());
}

@@ -261,6 +279,7 @@ private Object readResolve() throws ObjectStreamException {
@SuppressWarnings("deprecation")
@NonNull
public synchronized Map<Domain, List<Credentials>> getDomainCredentialsMap() {
checkPermission(CredentialsProvider.VIEW);
return domainCredentialsMap = DomainCredentials.migrateListToMap(domainCredentialsMap, credentials);
}

@@ -271,6 +290,7 @@ private Object readResolve() throws ObjectStreamException {
* @since 1.5
*/
public synchronized void setDomainCredentialsMap(Map<Domain, List<Credentials>> domainCredentialsMap) {
checkPermission(CredentialsProvider.MANAGE_DOMAINS);
this.domainCredentialsMap = DomainCredentials.toCopyOnWriteMap(domainCredentialsMap);
}

@@ -408,7 +428,11 @@ private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull
}

private void checkPermission(Permission p) {
user.checkPermission(p);
if (user.equals(User.current())) {
user.checkPermission(p);
} else {
throw new AccessDeniedException2(Jenkins.getAuthentication(), p);
}
}

private void save() throws IOException {
@@ -33,15 +33,21 @@
import hudson.model.User;
import hudson.security.ACL;
import hudson.util.FormValidation;
import static hudson.util.FormValidation.Kind.*;
import java.io.IOException;
import java.util.Iterator;
import org.junit.Test;
import static org.junit.Assert.*;
import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockFolder;

import static hudson.util.FormValidation.Kind.ERROR;
import static hudson.util.FormValidation.Kind.OK;
import static hudson.util.FormValidation.Kind.WARNING;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class BaseStandardCredentialsTest {

@Rule public JenkinsRule r = new JenkinsRule();
@@ -57,16 +63,26 @@
// First set up two users, each of which has an existing credentials named ‘per-user’.
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
final User alice = User.get("alice");
CredentialsStore store = lookupStore(alice);
addCreds(store, CredentialsScope.USER, "alice");
addCreds(store, CredentialsScope.USER, "per-user");
SecurityContext ctx = ACL.impersonate(alice.impersonate());
try {
CredentialsStore store = lookupStore(alice);
addCreds(store, CredentialsScope.USER, "alice");
addCreds(store, CredentialsScope.USER, "per-user");
} finally {
SecurityContextHolder.setContext(ctx);
}
User bob = User.get("bob");
store = lookupStore(bob);
addCreds(store, CredentialsScope.USER, "bob");
addCreds(store, CredentialsScope.USER, "per-user");
ctx = ACL.impersonate(bob.impersonate());
try {
CredentialsStore store = lookupStore(bob);
addCreds(store, CredentialsScope.USER, "bob");
addCreds(store, CredentialsScope.USER, "per-user");
} finally {
SecurityContextHolder.setContext(ctx);
}

// Now set up a folder tree with some masking of credentials.
store = lookupStore(r.jenkins);
CredentialsStore store = lookupStore(r.jenkins);
addCreds(store, CredentialsScope.GLOBAL, "masked");
addCreds(store, CredentialsScope.GLOBAL, "root");
// TODO not currently testing SYSTEM; should this make any difference to behavior here?

0 comments on commit 3876043

Please sign in to comment.
You can’t perform that action at this time.