From 82b3bda2aa8bc381e474394188d0435a8719c067 Mon Sep 17 00:00:00 2001 From: dfuchs Date: Mon, 9 Mar 2009 23:50:11 +0100 Subject: [PATCH] 6721651: Security problem with out-of-the-box management Reviewed-by: emcmanus, lmalvent --- .../security/MBeanServerAccessController.java | 52 ++- .../MBeanServerFileAccessController.java | 374 +++++++++++++++--- src/share/lib/management/jmxremote.access | 47 ++- 3 files changed, 388 insertions(+), 85 deletions(-) diff --git a/src/share/classes/com/sun/jmx/remote/security/MBeanServerAccessController.java b/src/share/classes/com/sun/jmx/remote/security/MBeanServerAccessController.java index d75f829c3..71e6f6ff0 100644 --- a/src/share/classes/com/sun/jmx/remote/security/MBeanServerAccessController.java +++ b/src/share/classes/com/sun/jmx/remote/security/MBeanServerAccessController.java @@ -111,6 +111,22 @@ public abstract class MBeanServerAccessController */ 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 @@ public abstract class MBeanServerAccessController } /** - * Call checkWrite(), then forward this method to the + * Call checkCreate(className), then forward this method to the * wrapped object. */ public ObjectInstance createMBean(String className, ObjectName name) @@ -158,7 +174,7 @@ public abstract class MBeanServerAccessController MBeanRegistrationException, MBeanException, NotCompliantMBeanException { - checkWrite(); + checkCreate(className); SecurityManager sm = System.getSecurityManager(); if (sm == null) { Object object = getMBeanServer().instantiate(className); @@ -170,7 +186,7 @@ public abstract class MBeanServerAccessController } /** - * Call checkWrite(), then forward this method to the + * Call checkCreate(className), then forward this method to the * wrapped object. */ public ObjectInstance createMBean(String className, ObjectName name, @@ -181,7 +197,7 @@ public abstract class MBeanServerAccessController MBeanRegistrationException, MBeanException, NotCompliantMBeanException { - checkWrite(); + checkCreate(className); SecurityManager sm = System.getSecurityManager(); if (sm == null) { Object object = getMBeanServer().instantiate(className, @@ -196,7 +212,7 @@ public abstract class MBeanServerAccessController } /** - * Call checkWrite(), then forward this method to the + * Call checkCreate(className), then forward this method to the * wrapped object. */ public ObjectInstance createMBean(String className, @@ -209,7 +225,7 @@ public abstract class MBeanServerAccessController MBeanException, NotCompliantMBeanException, InstanceNotFoundException { - checkWrite(); + checkCreate(className); SecurityManager sm = System.getSecurityManager(); if (sm == null) { Object object = getMBeanServer().instantiate(className, @@ -222,7 +238,7 @@ public abstract class MBeanServerAccessController } /** - * Call checkWrite(), then forward this method to the + * Call checkCreate(className), then forward this method to the * wrapped object. */ public ObjectInstance createMBean(String className, @@ -237,7 +253,7 @@ public abstract class MBeanServerAccessController MBeanException, NotCompliantMBeanException, InstanceNotFoundException { - checkWrite(); + checkCreate(className); SecurityManager sm = System.getSecurityManager(); if (sm == null) { Object object = getMBeanServer().instantiate(className, @@ -394,45 +410,45 @@ public abstract class MBeanServerAccessController } /** - * Call checkWrite(), then forward this method to the + * Call checkCreate(className), then forward this method to the * wrapped object. */ public Object instantiate(String className) throws ReflectionException, MBeanException { - checkWrite(); + checkCreate(className); return getMBeanServer().instantiate(className); } /** - * Call checkWrite(), then forward this method to the + * Call checkCreate(className), 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 checkWrite(), then forward this method to the + * Call checkCreate(className), 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 checkWrite(), then forward this method to the + * Call checkCreate(className), 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 @@ public abstract class MBeanServerAccessController } /** - * Call checkWrite(), then forward this method to the + * Call checkUnregister(), then forward this method to the * wrapped object. */ public void unregisterMBean(ObjectName name) throws InstanceNotFoundException, MBeanRegistrationException { - checkWrite(); + checkUnregister(name); getMBeanServer().unregisterMBean(name); } diff --git a/src/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java b/src/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java index 8bca71dbc..01bb6108e 100644 --- a/src/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java +++ b/src/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java @@ -31,11 +31,17 @@ import java.security.AccessControlContext; 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 @@ import javax.security.auth.Subject; * not allowed; in this case the request is not forwarded to the * wrapped object.

* - *

This class implements the {@link #checkRead()} and {@link #checkWrite()} + *

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 @@ import javax.security.auth.Subject; * has exactly one access level. The same access level can be shared by several * usernames.

* - *

The supported access level values are readonly and - * readwrite.

+ *

The supported access level values are {@code readonly} and + * {@code readwrite}. The {@code readwrite} access level can be + * qualified by one or more clauses, where each clause looks + * like create classNamePattern or {@code + * unregister}. For example:

+ * + * 
+ * monitorRole  readonly
+ * controlRole  readwrite \
+ *              create javax.management.timer.*,javax.management.monitor.* \
+ *              unregister
+ *
+ * + *

(The continuation lines with {@code \} come from the parser for + * Properties files.)

*/ 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 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]; + } /** *

Create a new MBeanServerAccessController that forwards all the @@ -87,8 +130,8 @@ public class MBeanServerFileAccessController throws IOException { super(); this.accessFileName = accessFileName; - props = propertiesFromFile(accessFileName); - checkValues(props); + Properties props = propertiesFromFile(accessFileName); + parseProperties(props); } /** @@ -123,14 +166,14 @@ public class MBeanServerFileAccessController * #setMBeanServer} method after doing access checks based on read and * write permissions.

* - *

This instance is initialized from the specified properties instance. - * This constructor makes a copy of the properties instance using its - * clone 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 - * MBeanServerFileAccessController will make a new copy of the - * properties object at that time.

+ *

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 + * MBeanServerFileAccessController will make a new + * copy of the properties object at that time.

* * @param accessFileProps properties list containing the username/access * level entries. @@ -145,8 +188,7 @@ public class MBeanServerFileAccessController if (accessFileProps == null) throw new IllegalArgumentException("Null properties"); originalProps = accessFileProps; - props = (Properties) accessFileProps.clone(); - checkValues(props); + parseProperties(accessFileProps); } /** @@ -155,14 +197,14 @@ public class MBeanServerFileAccessController * #setMBeanServer} method after doing access checks based on read and * write permissions.

* - *

This instance is initialized from the specified properties instance. - * This constructor makes a copy of the properties instance using its - * clone 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 - * MBeanServerFileAccessController will make a new copy of the - * properties object at that time.

+ *

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 + * MBeanServerFileAccessController will make a new + * copy of the properties object at that time.

* * @param accessFileProps properties list containing the username/access * level entries. @@ -184,16 +226,36 @@ public class MBeanServerFileAccessController * 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 @@ public class MBeanServerFileAccessController * @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 @@ public class MBeanServerFileAccessController } } - 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() { @@ -250,39 +311,234 @@ public class MBeanServerFileAccessController }); 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 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(); + for (Map.Entry entry : props.entrySet()) { + String identity = (String) entry.getKey(); + String accessString = (String) entry.getValue(); + Access access = Parser.parseAccess(identity, accessString); + accessMap.put(identity, access); + } } - 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 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 createClasses = new ArrayList(); + 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 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 Properties props; + private Map accessMap; private Properties originalProps; private String accessFileName; } diff --git a/src/share/lib/management/jmxremote.access b/src/share/lib/management/jmxremote.access index 765f118a3..ce80b47a1 100644 --- a/src/share/lib/management/jmxremote.access +++ b/src/share/lib/management/jmxremote.access @@ -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 -- GitLab