6721651: Security problem with out-of-the-box management
Reviewed-by: emcmanus, lmalvent
--- a/jdk/src/share/classes/com/sun/jmx/remote/security/MBeanServerAccessController.java Fri Mar 06 12:40:38 2009 +0300
+++ b/jdk/src/share/classes/com/sun/jmx/remote/security/MBeanServerAccessController.java Mon Mar 09 23:50:11 2009 +0100
@@ -111,6 +111,22 @@
*/
protected abstract void checkWrite();
+ /**
+ * Check if the caller can create the named class. The default
+ * implementation of this method calls {@link #checkWrite()}.
+ */
+ protected void checkCreate(String className) {
+ checkWrite();
+ }
+
+ /**
+ * Check if the caller can unregister the named MBean. The default
+ * implementation of this method calls {@link #checkWrite()}.
+ */
+ protected void checkUnregister(ObjectName name) {
+ checkWrite();
+ }
+
//--------------------------------------------
//--------------------------------------------
//
@@ -148,7 +164,7 @@
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public ObjectInstance createMBean(String className, ObjectName name)
@@ -158,7 +174,7 @@
MBeanRegistrationException,
MBeanException,
NotCompliantMBeanException {
- checkWrite();
+ checkCreate(className);
SecurityManager sm = System.getSecurityManager();
if (sm == null) {
Object object = getMBeanServer().instantiate(className);
@@ -170,7 +186,7 @@
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public ObjectInstance createMBean(String className, ObjectName name,
@@ -181,7 +197,7 @@
MBeanRegistrationException,
MBeanException,
NotCompliantMBeanException {
- checkWrite();
+ checkCreate(className);
SecurityManager sm = System.getSecurityManager();
if (sm == null) {
Object object = getMBeanServer().instantiate(className,
@@ -196,7 +212,7 @@
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public ObjectInstance createMBean(String className,
@@ -209,7 +225,7 @@
MBeanException,
NotCompliantMBeanException,
InstanceNotFoundException {
- checkWrite();
+ checkCreate(className);
SecurityManager sm = System.getSecurityManager();
if (sm == null) {
Object object = getMBeanServer().instantiate(className,
@@ -222,7 +238,7 @@
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public ObjectInstance createMBean(String className,
@@ -237,7 +253,7 @@
MBeanException,
NotCompliantMBeanException,
InstanceNotFoundException {
- checkWrite();
+ checkCreate(className);
SecurityManager sm = System.getSecurityManager();
if (sm == null) {
Object object = getMBeanServer().instantiate(className,
@@ -394,45 +410,45 @@
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public Object instantiate(String className)
throws ReflectionException, MBeanException {
- checkWrite();
+ checkCreate(className);
return getMBeanServer().instantiate(className);
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public Object instantiate(String className,
Object params[],
String signature[])
throws ReflectionException, MBeanException {
- checkWrite();
+ checkCreate(className);
return getMBeanServer().instantiate(className, params, signature);
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public Object instantiate(String className, ObjectName loaderName)
throws ReflectionException, MBeanException, InstanceNotFoundException {
- checkWrite();
+ checkCreate(className);
return getMBeanServer().instantiate(className, loaderName);
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkCreate(className)</code>, then forward this method to the
* wrapped object.
*/
public Object instantiate(String className, ObjectName loaderName,
Object params[], String signature[])
throws ReflectionException, MBeanException, InstanceNotFoundException {
- checkWrite();
+ checkCreate(className);
return getMBeanServer().instantiate(className, loaderName,
params, signature);
}
@@ -579,12 +595,12 @@
}
/**
- * Call <code>checkWrite()</code>, then forward this method to the
+ * Call <code>checkUnregister()</code>, then forward this method to the
* wrapped object.
*/
public void unregisterMBean(ObjectName name)
throws InstanceNotFoundException, MBeanRegistrationException {
- checkWrite();
+ checkUnregister(name);
getMBeanServer().unregisterMBean(name);
}
--- a/jdk/src/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java Fri Mar 06 12:40:38 2009 +0300
+++ b/jdk/src/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java Mon Mar 09 23:50:11 2009 +0100
@@ -31,11 +31,17 @@
import java.security.AccessController;
import java.security.Principal;
import java.security.PrivilegedAction;
-import java.util.Collection;
+import java.util.ArrayList;
+import java.util.HashMap;
import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
import java.util.Properties;
import java.util.Set;
+import java.util.StringTokenizer;
+import java.util.regex.Pattern;
import javax.management.MBeanServer;
+import javax.management.ObjectName;
import javax.security.auth.Subject;
/**
@@ -46,7 +52,8 @@
* not allowed; in this case the request is not forwarded to the
* wrapped object.</p>
*
- * <p>This class implements the {@link #checkRead()} and {@link #checkWrite()}
+ * <p>This class implements the {@link #checkRead()}, {@link #checkWrite()},
+ * {@link #checkCreate(String)}, and {@link #checkUnregister(ObjectName)}
* methods based on an access level properties file containing username/access
* level pairs. The set of username/access level pairs is passed either as a
* filename which denotes a properties file on disk, or directly as an instance
@@ -56,14 +63,50 @@
* has exactly one access level. The same access level can be shared by several
* usernames.</p>
*
- * <p>The supported access level values are <i>readonly</i> and
- * <i>readwrite</i>.</p>
+ * <p>The supported access level values are {@code readonly} and
+ * {@code readwrite}. The {@code readwrite} access level can be
+ * qualified by one or more <i>clauses</i>, where each clause looks
+ * like <code>create <i>classNamePattern</i></code> or {@code
+ * unregister}. For example:</p>
+ *
+ * <pre>
+ * monitorRole readonly
+ * controlRole readwrite \
+ * create javax.management.timer.*,javax.management.monitor.* \
+ * unregister
+ * </pre>
+ *
+ * <p>(The continuation lines with {@code \} come from the parser for
+ * Properties files.)</p>
*/
public class MBeanServerFileAccessController
extends MBeanServerAccessController {
- public static final String READONLY = "readonly";
- public static final String READWRITE = "readwrite";
+ static final String READONLY = "readonly";
+ static final String READWRITE = "readwrite";
+
+ static final String CREATE = "create";
+ static final String UNREGISTER = "unregister";
+
+ private enum AccessType {READ, WRITE, CREATE, UNREGISTER};
+
+ private static class Access {
+ final boolean write;
+ final String[] createPatterns;
+ private boolean unregister;
+
+ Access(boolean write, boolean unregister, List<String> createPatternList) {
+ this.write = write;
+ int npats = (createPatternList == null) ? 0 : createPatternList.size();
+ if (npats == 0)
+ this.createPatterns = NO_STRINGS;
+ else
+ this.createPatterns = createPatternList.toArray(new String[npats]);
+ this.unregister = unregister;
+ }
+
+ private final String[] NO_STRINGS = new String[0];
+ }
/**
* <p>Create a new MBeanServerAccessController that forwards all the
@@ -87,8 +130,8 @@
throws IOException {
super();
this.accessFileName = accessFileName;
- props = propertiesFromFile(accessFileName);
- checkValues(props);
+ Properties props = propertiesFromFile(accessFileName);
+ parseProperties(props);
}
/**
@@ -123,14 +166,14 @@
* #setMBeanServer} method after doing access checks based on read and
* write permissions.</p>
*
- * <p>This instance is initialized from the specified properties instance.
- * This constructor makes a copy of the properties instance using its
- * <code>clone</code> method and it is the copy that is consulted to check
- * the username and access level of an incoming connection. The original
- * properties object can be modified without affecting the copy. If the
- * {@link #refresh} method is then called, the
- * <code>MBeanServerFileAccessController</code> will make a new copy of the
- * properties object at that time.</p>
+ * <p>This instance is initialized from the specified properties
+ * instance. This constructor makes a copy of the properties
+ * instance and it is the copy that is consulted to check the
+ * username and access level of an incoming connection. The
+ * original properties object can be modified without affecting
+ * the copy. If the {@link #refresh} method is then called, the
+ * <code>MBeanServerFileAccessController</code> will make a new
+ * copy of the properties object at that time.</p>
*
* @param accessFileProps properties list containing the username/access
* level entries.
@@ -145,8 +188,7 @@
if (accessFileProps == null)
throw new IllegalArgumentException("Null properties");
originalProps = accessFileProps;
- props = (Properties) accessFileProps.clone();
- checkValues(props);
+ parseProperties(accessFileProps);
}
/**
@@ -155,14 +197,14 @@
* #setMBeanServer} method after doing access checks based on read and
* write permissions.</p>
*
- * <p>This instance is initialized from the specified properties instance.
- * This constructor makes a copy of the properties instance using its
- * <code>clone</code> method and it is the copy that is consulted to check
- * the username and access level of an incoming connection. The original
- * properties object can be modified without affecting the copy. If the
- * {@link #refresh} method is then called, the
- * <code>MBeanServerFileAccessController</code> will make a new copy of the
- * properties object at that time.</p>
+ * <p>This instance is initialized from the specified properties
+ * instance. This constructor makes a copy of the properties
+ * instance and it is the copy that is consulted to check the
+ * username and access level of an incoming connection. The
+ * original properties object can be modified without affecting
+ * the copy. If the {@link #refresh} method is then called, the
+ * <code>MBeanServerFileAccessController</code> will make a new
+ * copy of the properties object at that time.</p>
*
* @param accessFileProps properties list containing the username/access
* level entries.
@@ -184,16 +226,36 @@
* Check if the caller can do read operations. This method does
* nothing if so, otherwise throws SecurityException.
*/
+ @Override
public void checkRead() {
- checkAccessLevel(READONLY);
+ checkAccess(AccessType.READ, null);
}
/**
* Check if the caller can do write operations. This method does
* nothing if so, otherwise throws SecurityException.
*/
+ @Override
public void checkWrite() {
- checkAccessLevel(READWRITE);
+ checkAccess(AccessType.WRITE, null);
+ }
+
+ /**
+ * Check if the caller can create MBeans or instances of the given class.
+ * This method does nothing if so, otherwise throws SecurityException.
+ */
+ @Override
+ public void checkCreate(String className) {
+ checkAccess(AccessType.CREATE, className);
+ }
+
+ /**
+ * Check if the caller can do unregister operations. This method does
+ * nothing if so, otherwise throws SecurityException.
+ */
+ @Override
+ public void checkUnregister(ObjectName name) {
+ checkAccess(AccessType.UNREGISTER, null);
}
/**
@@ -218,14 +280,13 @@
* @exception IllegalArgumentException if any of the supplied access
* level values differs from "readonly" or "readwrite".
*/
- public void refresh() throws IOException {
- synchronized (props) {
- if (accessFileName == null)
- props = (Properties) originalProps.clone();
- else
- props = propertiesFromFile(accessFileName);
- checkValues(props);
- }
+ public synchronized void refresh() throws IOException {
+ Properties props;
+ if (accessFileName == null)
+ props = (Properties) originalProps;
+ else
+ props = propertiesFromFile(accessFileName);
+ parseProperties(props);
}
private static Properties propertiesFromFile(String fname)
@@ -240,7 +301,7 @@
}
}
- private void checkAccessLevel(String accessLevel) {
+ private synchronized void checkAccess(AccessType requiredAccess, String arg) {
final AccessControlContext acc = AccessController.getContext();
final Subject s =
AccessController.doPrivileged(new PrivilegedAction<Subject>() {
@@ -250,39 +311,234 @@
});
if (s == null) return; /* security has not been enabled */
final Set principals = s.getPrincipals();
+ String newPropertyValue = null;
for (Iterator i = principals.iterator(); i.hasNext(); ) {
final Principal p = (Principal) i.next();
- String grantedAccessLevel;
- synchronized (props) {
- grantedAccessLevel = props.getProperty(p.getName());
- }
- if (grantedAccessLevel != null) {
- if (accessLevel.equals(READONLY) &&
- (grantedAccessLevel.equals(READONLY) ||
- grantedAccessLevel.equals(READWRITE)))
- return;
- if (accessLevel.equals(READWRITE) &&
- grantedAccessLevel.equals(READWRITE))
+ Access access = accessMap.get(p.getName());
+ if (access != null) {
+ boolean ok;
+ switch (requiredAccess) {
+ case READ:
+ ok = true; // all access entries imply read
+ break;
+ case WRITE:
+ ok = access.write;
+ break;
+ case UNREGISTER:
+ ok = access.unregister;
+ if (!ok && access.write)
+ newPropertyValue = "unregister";
+ break;
+ case CREATE:
+ ok = checkCreateAccess(access, arg);
+ if (!ok && access.write)
+ newPropertyValue = "create " + arg;
+ break;
+ default:
+ throw new AssertionError();
+ }
+ if (ok)
return;
}
}
- throw new SecurityException("Access denied! Invalid access level for " +
- "requested MBeanServer operation.");
+ SecurityException se = new SecurityException("Access denied! Invalid " +
+ "access level for requested MBeanServer operation.");
+ // Add some more information to help people with deployments that
+ // worked before we required explicit create clauses. We're not giving
+ // any information to the bad guys, other than that the access control
+ // is based on a file, which they could have worked out from the stack
+ // trace anyway.
+ if (newPropertyValue != null) {
+ SecurityException se2 = new SecurityException("Access property " +
+ "for this identity should be similar to: " + READWRITE +
+ " " + newPropertyValue);
+ se.initCause(se2);
+ }
+ throw se;
+ }
+
+ private static boolean checkCreateAccess(Access access, String className) {
+ for (String classNamePattern : access.createPatterns) {
+ if (classNameMatch(classNamePattern, className))
+ return true;
+ }
+ return false;
}
- private void checkValues(Properties props) {
- Collection c = props.values();
- for (Iterator i = c.iterator(); i.hasNext(); ) {
- final String accessLevel = (String) i.next();
- if (!accessLevel.equals(READONLY) &&
- !accessLevel.equals(READWRITE)) {
- throw new IllegalArgumentException(
- "Syntax error in access level entry [" + accessLevel + "]");
- }
+ private static boolean classNameMatch(String pattern, String className) {
+ // We studiously avoided regexes when parsing the properties file,
+ // because that is done whenever the VM is started with the
+ // appropriate -Dcom.sun.management options, even if nobody ever
+ // creates an MBean. We don't want to incur the overhead of loading
+ // all the regex code whenever those options are specified, but if we
+ // get as far as here then the VM is already running and somebody is
+ // doing the very unusual operation of remotely creating an MBean.
+ // Because that operation is so unusual, we don't try to optimize
+ // by hand-matching or by caching compiled Pattern objects.
+ StringBuilder sb = new StringBuilder();
+ StringTokenizer stok = new StringTokenizer(pattern, "*", true);
+ while (stok.hasMoreTokens()) {
+ String tok = stok.nextToken();
+ if (tok.equals("*"))
+ sb.append("[^.]*");
+ else
+ sb.append(Pattern.quote(tok));
+ }
+ return className.matches(sb.toString());
+ }
+
+ private void parseProperties(Properties props) {
+ this.accessMap = new HashMap<String, Access>();
+ for (Map.Entry<Object, Object> entry : props.entrySet()) {
+ String identity = (String) entry.getKey();
+ String accessString = (String) entry.getValue();
+ Access access = Parser.parseAccess(identity, accessString);
+ accessMap.put(identity, access);
}
}
- private Properties props;
+ private static class Parser {
+ private final static int EOS = -1; // pseudo-codepoint "end of string"
+ static {
+ assert !Character.isWhitespace(EOS);
+ }
+
+ private final String identity; // just for better error messages
+ private final String s; // the string we're parsing
+ private final int len; // s.length()
+ private int i;
+ private int c;
+ // At any point, either c is s.codePointAt(i), or i == len and
+ // c is EOS. We use int rather than char because it is conceivable
+ // (if unlikely) that a classname in a create clause might contain
+ // "supplementary characters", the ones that don't fit in the original
+ // 16 bits for Unicode.
+
+ private Parser(String identity, String s) {
+ this.identity = identity;
+ this.s = s;
+ this.len = s.length();
+ this.i = 0;
+ if (i < len)
+ this.c = s.codePointAt(i);
+ else
+ this.c = EOS;
+ }
+
+ static Access parseAccess(String identity, String s) {
+ return new Parser(identity, s).parseAccess();
+ }
+
+ private Access parseAccess() {
+ skipSpace();
+ String type = parseWord();
+ Access access;
+ if (type.equals(READONLY))
+ access = new Access(false, false, null);
+ else if (type.equals(READWRITE))
+ access = parseReadWrite();
+ else {
+ throw syntax("Expected " + READONLY + " or " + READWRITE +
+ ": " + type);
+ }
+ if (c != EOS)
+ throw syntax("Extra text at end of line");
+ return access;
+ }
+
+ private Access parseReadWrite() {
+ List<String> createClasses = new ArrayList<String>();
+ boolean unregister = false;
+ while (true) {
+ skipSpace();
+ if (c == EOS)
+ break;
+ String type = parseWord();
+ if (type.equals(UNREGISTER))
+ unregister = true;
+ else if (type.equals(CREATE))
+ parseCreate(createClasses);
+ else
+ throw syntax("Unrecognized keyword " + type);
+ }
+ return new Access(true, unregister, createClasses);
+ }
+
+ private void parseCreate(List<String> createClasses) {
+ while (true) {
+ skipSpace();
+ createClasses.add(parseClassName());
+ skipSpace();
+ if (c == ',')
+ next();
+ else
+ break;
+ }
+ }
+
+ private String parseClassName() {
+ // We don't check that classname components begin with suitable
+ // characters (so we accept 1.2.3 for example). This means that
+ // there are only two states, which we can call dotOK and !dotOK
+ // according as a dot (.) is legal or not. Initially we're in
+ // !dotOK since a classname can't start with a dot; after a dot
+ // we're in !dotOK again; and after any other characters we're in
+ // dotOK. The classname is only accepted if we end in dotOK,
+ // so we reject an empty name or a name that ends with a dot.
+ final int start = i;
+ boolean dotOK = false;
+ while (true) {
+ if (c == '.') {
+ if (!dotOK)
+ throw syntax("Bad . in class name");
+ dotOK = false;
+ } else if (c == '*' || Character.isJavaIdentifierPart(c))
+ dotOK = true;
+ else
+ break;
+ next();
+ }
+ String className = s.substring(start, i);
+ if (!dotOK)
+ throw syntax("Bad class name " + className);
+ return className;
+ }
+
+ // Advance c and i to the next character, unless already at EOS.
+ private void next() {
+ if (c != EOS) {
+ i += Character.charCount(c);
+ if (i < len)
+ c = s.codePointAt(i);
+ else
+ c = EOS;
+ }
+ }
+
+ private void skipSpace() {
+ while (Character.isWhitespace(c))
+ next();
+ }
+
+ private String parseWord() {
+ skipSpace();
+ if (c == EOS)
+ throw syntax("Expected word at end of line");
+ final int start = i;
+ while (c != EOS && !Character.isWhitespace(c))
+ next();
+ String word = s.substring(start, i);
+ skipSpace();
+ return word;
+ }
+
+ private IllegalArgumentException syntax(String msg) {
+ return new IllegalArgumentException(
+ msg + " [" + identity + " " + s + "]");
+ }
+ }
+
+ private Map<String, Access> accessMap;
private Properties originalProps;
private String accessFileName;
}
--- a/jdk/src/share/lib/management/jmxremote.access Fri Mar 06 12:40:38 2009 +0300
+++ b/jdk/src/share/lib/management/jmxremote.access Mon Mar 09 23:50:11 2009 +0100
@@ -8,7 +8,7 @@
# passwords. To be functional, a role must have an entry in
# both the password and the access files.
#
-# Default location of this file is $JRE/lib/management/jmxremote.access
+# The default location of this file is $JRE/lib/management/jmxremote.access
# You can specify an alternate location by specifying a property in
# the management config file $JRE/lib/management/management.properties
# (See that file for details)
@@ -16,7 +16,7 @@
# The file format for password and access files is syntactically the same
# as the Properties file format. The syntax is described in the Javadoc
# for java.util.Properties.load.
-# Typical access file has multiple lines, where each line is blank,
+# A typical access file has multiple lines, where each line is blank,
# a comment (like this one), or an access control entry.
#
# An access control entry consists of a role name, and an
@@ -29,10 +29,38 @@
# role can read measurements but cannot perform any action
# that changes the environment of the running program.
# "readwrite" grants access to read and write attributes of MBeans,
-# to invoke operations on them, and to create or remove them.
-# This access should be granted to only trusted clients,
-# since they can potentially interfere with the smooth
-# operation of a running program
+# to invoke operations on them, and optionally
+# to create or remove them. This access should be granted
+# only to trusted clients, since they can potentially
+# interfere with the smooth operation of a running program.
+#
+# The "readwrite" access level can optionally be followed by the "create" and/or
+# "unregister" keywords. The "unregister" keyword grants access to unregister
+# (delete) MBeans. The "create" keyword grants access to create MBeans of a
+# particular class or of any class matching a particular pattern. Access
+# should only be granted to create MBeans of known and trusted classes.
+#
+# For example, the following entry would grant readwrite access
+# to "controlRole", as well as access to create MBeans of the class
+# javax.management.monitor.CounterMonitor and to unregister any MBean:
+# controlRole readwrite \
+# create javax.management.monitor.CounterMonitorMBean \
+# unregister
+# or equivalently:
+# controlRole readwrite unregister create javax.management.monitor.CounterMBean
+#
+# The following entry would grant readwrite access as well as access to create
+# MBeans of any class in the packages javax.management.monitor and
+# javax.management.timer:
+# controlRole readwrite \
+# create javax.management.monitor.*,javax.management.timer.* \
+# unregister
+#
+# The \ character is defined in the Properties file syntax to allow continuation
+# lines as shown here. A * in a class pattern matches a sequence of characters
+# other than dot (.), so javax.management.monitor.* matches
+# javax.management.monitor.CounterMonitor but not
+# javax.management.monitor.foo.Bar.
#
# A given role should have at most one entry in this file. If a role
# has no entry, it has no access.
@@ -42,7 +70,10 @@
#
# Default access control entries:
# o The "monitorRole" role has readonly access.
-# o The "controlRole" role has readwrite access.
+# o The "controlRole" role has readwrite access and can create the standard
+# Timer and Monitor MBeans defined by the JMX API.
monitorRole readonly
-controlRole readwrite
+controlRole readwrite \
+ create javax.management.monitor.*,javax.management.timer.* \
+ unregister