Skip to content
Permalink
Browse files Browse the repository at this point in the history
[SECURITY-49] Deprecating Jenkins.getSecretKey()
We are replacing it by the ConfidentialStore class and the
ConfidentialKey class, which provides purpose-specific confidential
information that are separated from each other.

In this way, not all eggs are in one basket, and in case of a
compromise, the impact will contained.

Also replaced several insecure use of digest(secret|messsage) or
digest(message|secret) by HMAC.
  • Loading branch information
kohsuke committed Jan 5, 2013
1 parent 31d2e03 commit a9aff08
Show file tree
Hide file tree
Showing 32 changed files with 1,066 additions and 155 deletions.
29 changes: 29 additions & 0 deletions core/pom.xml
Expand Up @@ -768,6 +768,35 @@ THE SOFTWARE.
</archive>
</configuration>
</plugin>
<plugin><!-- run unit test in src/test/java -->
<groupId>org.kohsuke.gmaven</groupId>
<artifactId>gmaven-plugin</artifactId>
<!-- version specified in grandparent pom -->
<executions>
<execution>
<goals>
<goal>testCompile</goal>
</goals>
<configuration>
<sources>
<fileset>
<directory>${project.basedir}/src/test/java</directory>
<includes>
<include>**/*.groovy</include>
</includes>
</fileset>
</sources>
</configuration>
</execution>
</executions>
<dependencies>
<dependency>
<groupId>org.codehaus.groovy</groupId>
<artifactId>groovy-all</artifactId>
<version>1.8.5</version>
</dependency>
</dependencies>
</plugin>
</plugins>
</build>

Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/DNSMultiCast.java
Expand Up @@ -46,7 +46,7 @@ public Object call() {
if (tal!=null)
props.put("slave-port",String.valueOf(tal.getPort()));

props.put("server-id", Util.getDigestOf(jenkins.getSecretKey()));
props.put("server-id", jenkins.getLegacyInstanceId());

URL jenkins_url = new URL(rootURL);
int jenkins_port = jenkins_url.getPort();
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/UDPBroadcastThread.java
Expand Up @@ -86,7 +86,7 @@ public void run() {
StringBuilder rsp = new StringBuilder("<hudson>");
tag(rsp,"version", Jenkins.VERSION);
tag(rsp,"url", jenkins.getRootUrl());
tag(rsp,"server-id", Util.getDigestOf(jenkins.getSecretKey()));
tag(rsp,"server-id", jenkins.getLegacyInstanceId());
tag(rsp,"slave-port",tal==null?null:tal.getPort());

for (UDPBroadcastFragment f : UDPBroadcastFragment.all())
Expand Down
33 changes: 15 additions & 18 deletions core/src/main/java/hudson/console/AnnotatedLargeText.java
Expand Up @@ -29,6 +29,7 @@
import hudson.util.IOException2;
import hudson.util.Secret;
import hudson.util.TimeUnit2;
import jenkins.security.CryptoConfidentialKey;
import org.apache.commons.io.output.ByteArrayOutputStream;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
Expand Down Expand Up @@ -116,8 +117,7 @@ private ConsoleAnnotator createAnnotator(StaplerRequest req) throws IOException
try {
String base64 = req!=null ? req.getHeader("X-ConsoleAnnotator") : null;
if (base64!=null) {
Cipher sym = Secret.getCipher("AES");
sym.init(Cipher.DECRYPT_MODE, Jenkins.getInstance().getSecretKeyAsAES128());
Cipher sym = PASSING_ANNOTATOR.decrypt();

ObjectInputStream ois = new ObjectInputStreamEx(new GZIPInputStream(
new CipherInputStream(new ByteArrayInputStream(Base64.decode(base64.toCharArray())),sym)),
Expand All @@ -131,8 +131,6 @@ private ConsoleAnnotator createAnnotator(StaplerRequest req) throws IOException
ois.close();
}
}
} catch (GeneralSecurityException e) {
throw new IOException2(e);
} catch (ClassNotFoundException e) {
throw new IOException2(e);
}
Expand All @@ -158,21 +156,20 @@ public long writeHtmlTo(long start, Writer w) throws IOException {
w, createAnnotator(Stapler.getCurrentRequest()), context, charset);
long r = super.writeLogTo(start,caw);

try {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
Cipher sym = Secret.getCipher("AES");
sym.init(Cipher.ENCRYPT_MODE, Jenkins.getInstance().getSecretKeyAsAES128());
ObjectOutputStream oos = new ObjectOutputStream(new GZIPOutputStream(new CipherOutputStream(baos,sym)));
oos.writeLong(System.currentTimeMillis()); // send timestamp to prevent a replay attack
oos.writeObject(caw.getConsoleAnnotator());
oos.close();
StaplerResponse rsp = Stapler.getCurrentResponse();
if (rsp!=null)
rsp.setHeader("X-ConsoleAnnotator", new String(Base64.encode(baos.toByteArray())));
} catch (GeneralSecurityException e) {
throw new IOException2(e);
}
ByteArrayOutputStream baos = new ByteArrayOutputStream();
Cipher sym = PASSING_ANNOTATOR.encrypt();
ObjectOutputStream oos = new ObjectOutputStream(new GZIPOutputStream(new CipherOutputStream(baos,sym)));
oos.writeLong(System.currentTimeMillis()); // send timestamp to prevent a replay attack
oos.writeObject(caw.getConsoleAnnotator());
oos.close();
StaplerResponse rsp = Stapler.getCurrentResponse();
if (rsp!=null)
rsp.setHeader("X-ConsoleAnnotator", new String(Base64.encode(baos.toByteArray())));
return r;
}

/**
* Used for sending the state of ConsoleAnnotator to the client, because we are deserializing this object later.
*/
private static final CryptoConfidentialKey PASSING_ANNOTATOR = new CryptoConfidentialKey(AnnotatedLargeText.class,"consoleAnnotator");
}
7 changes: 5 additions & 2 deletions core/src/main/java/hudson/model/Job.java
Expand Up @@ -62,6 +62,7 @@
import hudson.widgets.Widget;
import jenkins.model.Jenkins;
import jenkins.model.ProjectNamingStrategy;
import jenkins.security.HexStringConfidentialKey;
import jenkins.util.io.OnMaster;
import net.sf.json.JSONException;
import net.sf.json.JSONObject;
Expand Down Expand Up @@ -308,8 +309,8 @@ public int getNextBuildNumber() {
*/
public EnvVars getCharacteristicEnvVars() {
EnvVars env = new EnvVars();
env.put("JENKINS_SERVER_COOKIE",Util.getDigestOf("ServerID:"+ Jenkins.getInstance().getSecretKey()));
env.put("HUDSON_SERVER_COOKIE",Util.getDigestOf("ServerID:"+ Jenkins.getInstance().getSecretKey())); // Legacy compatibility
env.put("JENKINS_SERVER_COOKIE",SERVER_COOKIE.get());
env.put("HUDSON_SERVER_COOKIE",SERVER_COOKIE.get()); // Legacy compatibility
env.put("JOB_NAME",getFullName());
return env;
}
Expand Down Expand Up @@ -1313,4 +1314,6 @@ public ACL getACL() {
public BuildTimelineWidget getTimeline() {
return new BuildTimelineWidget(getBuilds());
}

private final static HexStringConfidentialKey SERVER_COOKIE = new HexStringConfidentialKey(Job.class,"serverCookie",16);
}
16 changes: 8 additions & 8 deletions core/src/main/java/hudson/model/UsageStatistics.java
Expand Up @@ -121,31 +121,31 @@ private RSAPublicKey getKey() {
* Gets the encrypted usage stat data to be sent to the Hudson server.
*/
public String getStatData() throws IOException {
Jenkins h = Jenkins.getInstance();
Jenkins j = Jenkins.getInstance();

JSONObject o = new JSONObject();
o.put("stat",1);
o.put("install", Util.getDigestOf(h.getSecretKey()));
o.put("servletContainer",h.servletContext.getServerInfo());
o.put("install", j.getLegacyInstanceId());
o.put("servletContainer", j.servletContext.getServerInfo());
o.put("version", Jenkins.VERSION);

List<JSONObject> nodes = new ArrayList<JSONObject>();
for( Computer c : h.getComputers() ) {
for( Computer c : j.getComputers() ) {
JSONObject n = new JSONObject();
if(c.getNode()==h) {
if(c.getNode()==j) {
n.put("master",true);
n.put("jvm-vendor", System.getProperty("java.vm.vendor"));
n.put("jvm-version", System.getProperty("java.version"));
}
n.put("executors",c.getNumExecutors());
DescriptorImpl descriptor = h.getDescriptorByType(DescriptorImpl.class);
DescriptorImpl descriptor = j.getDescriptorByType(DescriptorImpl.class);
n.put("os", descriptor.get(c));
nodes.add(n);
}
o.put("nodes",nodes);

List<JSONObject> plugins = new ArrayList<JSONObject>();
for( PluginWrapper pw : h.getPluginManager().getPlugins() ) {
for( PluginWrapper pw : j.getPluginManager().getPlugins() ) {
if(!pw.isActive()) continue; // treat disabled plugins as if they are uninstalled
JSONObject p = new JSONObject();
p.put("name",pw.getShortName());
Expand All @@ -155,7 +155,7 @@ public String getStatData() throws IOException {
o.put("plugins",plugins);

JSONObject jobs = new JSONObject();
List<TopLevelItem> items = h.getItems();
List<TopLevelItem> items = j.getItems();
for (TopLevelItemDescriptor d : Items.all()) {
int cnt=0;
for (TopLevelItem item : items) {
Expand Down
Expand Up @@ -23,6 +23,8 @@
*/
package hudson.security;

import jenkins.model.Jenkins;
import jenkins.security.ConfidentialStore;
import org.acegisecurity.ui.rememberme.RememberMeServices;
import org.acegisecurity.Authentication;

Expand All @@ -33,9 +35,9 @@
* {@link RememberMeServices} proxy.
*
* <p>
* In Hudson, we need {@link jenkins.model.Jenkins} instance to perform remember-me service,
* because it relies on {@link jenkins.model.Jenkins#getSecretKey()}. However, security
* filters can be initialized before Hudson is initialized.
* In Jenkins, we need {@link Jenkins} instance to perform remember-me service,
* because it relies on {@link ConfidentialStore}. However, security
* filters can be initialized before Jenkins is initialized.
* (See #1210 for example.)
*
* <p>
Expand Down
11 changes: 11 additions & 0 deletions core/src/main/java/hudson/security/SecurityRealm.java
Expand Up @@ -516,10 +516,21 @@ public SecurityComponents(AuthenticationManager manager, UserDetailsService user
this.rememberMe = rememberMe;
}

@SuppressWarnings("deprecation")
private static RememberMeServices createRememberMeService(UserDetailsService uds) {
// create our default TokenBasedRememberMeServices, which depends on the availability of the secret key
TokenBasedRememberMeServices2 rms = new TokenBasedRememberMeServices2();
rms.setUserDetailsService(uds);
/*
TokenBasedRememberMeServices needs to be used in conjunction with RememberMeAuthenticationProvider,
and both needs to use the same key (this is a reflection of a poor design in AcgeiSecurity, if you ask me)
and various security plugins have its own groovy script that configures them.
So if we change this, it creates a painful situation for those plugins by forcing them to choose
to work with earlier version of Jenkins or newer version of Jenkins, and not both.
So we keep this here.
*/
rms.setKey(Jenkins.getInstance().getSecretKey());
rms.setParameter("remember_me"); // this is the form field name in login.jelly
return rms;
Expand Down
Expand Up @@ -23,10 +23,10 @@
*/
package hudson.security;

import jenkins.security.HMACConfidentialKey;
import org.acegisecurity.ui.rememberme.TokenBasedRememberMeServices;
import org.acegisecurity.userdetails.UserDetails;
import org.acegisecurity.Authentication;
import org.apache.commons.codec.digest.DigestUtils;

/**
* {@link TokenBasedRememberMeServices} with modification so as not to rely
Expand All @@ -41,7 +41,7 @@
public class TokenBasedRememberMeServices2 extends TokenBasedRememberMeServices {
@Override
protected String makeTokenSignature(long tokenExpiryTime, UserDetails userDetails) {
String expectedTokenSignature = DigestUtils.md5Hex(userDetails.getUsername() + ":" + tokenExpiryTime + ":"
String expectedTokenSignature = MAC.mac(userDetails.getUsername() + ":" + tokenExpiryTime + ":"
+ "N/A" + ":" + getKey());
return expectedTokenSignature;
}
Expand All @@ -50,4 +50,9 @@ protected String makeTokenSignature(long tokenExpiryTime, UserDetails userDetail
protected String retrievePassword(Authentication successfulAuthentication) {
return "N/A";
}

/**
* Used to compute the token signature securely.
*/
private static final HMACConfidentialKey MAC = new HMACConfidentialKey(TokenBasedRememberMeServices.class,"mac");
}
Expand Up @@ -118,7 +118,8 @@ private String getClientIP(HttpServletRequest req) {
public static final class DescriptorImpl extends CrumbIssuerDescriptor<DefaultCrumbIssuer> implements ModelObject {

public DescriptorImpl() {
super(Jenkins.getInstance().getSecretKey(), System.getProperty("hudson.security.csrf.requestfield", ".crumb"));
// salt just needs to be unique, and it doesn't have to be a secret
super(Jenkins.getInstance().getLegacyInstanceId(), System.getProperty("hudson.security.csrf.requestfield", ".crumb"));
load();
}

Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/hudson/slaves/SlaveComputer.java
Expand Up @@ -59,6 +59,7 @@

import hudson.util.io.ReopenableFileOutputStream;
import jenkins.model.Jenkins;
import jenkins.slaves.JnlpSlaveAgentProtocol;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.kohsuke.stapler.QueryParameter;
Expand Down Expand Up @@ -128,6 +129,10 @@ public boolean isAcceptingTasks() {
return acceptingTasks;
}

public String getJnlpMac() {
return JnlpSlaveAgentProtocol.SLAVE_SECRET.mac(getName());
}

/**
* Allows a {@linkplain hudson.slaves.ComputerLauncher} or a {@linkplain hudson.slaves.RetentionStrategy} to
* suspend tasks being accepted by the slave computer.
Expand Down
48 changes: 36 additions & 12 deletions core/src/main/java/hudson/util/Secret.java
Expand Up @@ -31,8 +31,11 @@
import com.trilead.ssh2.crypto.Base64;
import jenkins.model.Jenkins;
import hudson.Util;
import jenkins.security.CryptoConfidentialKey;
import org.kohsuke.stapler.Stapler;

import javax.crypto.BadPaddingException;
import javax.crypto.IllegalBlockSizeException;
import javax.crypto.SecretKey;
import javax.crypto.Cipher;
import java.io.Serializable;
Expand All @@ -44,8 +47,8 @@
* Glorified {@link String} that uses encryption in the persisted form, to avoid accidental exposure of a secret.
*
* <p>
* Note that since the cryptography relies on {@link jenkins.model.Jenkins#getSecretKey()}, this is not meant as a protection
* against code running in the same VM, nor against an attacker who has local file system access.
* This is not meant as a protection against code running in the same VM, nor against an attacker
* who has local file system access on Jenkins master.
*
* <p>
* {@link Secret}s can correctly read-in plain text password, so this allows the existing
Expand Down Expand Up @@ -96,9 +99,13 @@ public int hashCode() {
}

/**
* Turns {@link jenkins.model.Jenkins#getSecretKey()} into an AES key.
* Turns {@link Jenkins#getSecretKey()} into an AES key.
*
* @deprecated
* This is no longer the key we use to encrypt new information, but we still need this
* to be able to decrypt what's already persisted.
*/
private static SecretKey getKey() throws UnsupportedEncodingException, GeneralSecurityException {
/*package*/ static SecretKey getLegacyKey() throws UnsupportedEncodingException, GeneralSecurityException {
String secret = SECRET;
if(secret==null) return Jenkins.getInstance().getSecretKeyAsAES128();
return Util.toAes128Key(secret);
Expand All @@ -111,8 +118,7 @@ private static SecretKey getKey() throws UnsupportedEncodingException, GeneralSe
*/
public String getEncryptedValue() {
try {
Cipher cipher = getCipher("AES");
cipher.init(Cipher.ENCRYPT_MODE, getKey());
Cipher cipher = KEY.encrypt();
// add the magic suffix which works like a check sum.
return new String(Base64.encode(cipher.doFinal((value+MAGIC).getBytes("UTF-8"))));
} catch (GeneralSecurityException e) {
Expand All @@ -129,12 +135,14 @@ public String getEncryptedValue() {
public static Secret decrypt(String data) {
if(data==null) return null;
try {
byte[] in = Base64.decode(data.toCharArray());
Secret s = tryDecrypt(KEY.decrypt(), in);
if (s!=null) return s;

// try our historical key for backward compatibility
Cipher cipher = getCipher("AES");
cipher.init(Cipher.DECRYPT_MODE, getKey());
String plainText = new String(cipher.doFinal(Base64.decode(data.toCharArray())), "UTF-8");
if(plainText.endsWith(MAGIC))
return new Secret(plainText.substring(0,plainText.length()-MAGIC.length()));
return null;
cipher.init(Cipher.DECRYPT_MODE, getLegacyKey());
return tryDecrypt(cipher, in);
} catch (GeneralSecurityException e) {
return null;
} catch (UnsupportedEncodingException e) {
Expand All @@ -144,6 +152,17 @@ public static Secret decrypt(String data) {
}
}

private static Secret tryDecrypt(Cipher cipher, byte[] in) throws UnsupportedEncodingException {
try {
String plainText = new String(cipher.doFinal(in), "UTF-8");
if(plainText.endsWith(MAGIC))
return new Secret(plainText.substring(0,plainText.length()-MAGIC.length()));
return null;
} catch (GeneralSecurityException e) {
return null;
}
}

/**
* Workaround for JENKINS-6459 / http://java.net/jira/browse/GLASSFISH-11862
* This method uses specific provider selected via hudson.util.Secret.provider system property
Expand Down Expand Up @@ -207,10 +226,15 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont
private static final String PROVIDER = System.getProperty(Secret.class.getName()+".provider");

/**
* For testing only. Override the secret key so that we can test this class without {@link jenkins.model.Jenkins}.
* For testing only. Override the secret key so that we can test this class without {@link Jenkins}.
*/
/*package*/ static String SECRET = null;

/**
* The key that encrypts the data on disk.
*/
private static final CryptoConfidentialKey KEY = new CryptoConfidentialKey(Secret.class.getName());

private static final long serialVersionUID = 1L;

static {
Expand Down

0 comments on commit a9aff08

Please sign in to comment.