diff --git a/src/dist-static/CHANGELOG b/src/dist-static/CHANGELOG index 06311638..980bfe0a 100644 --- a/src/dist-static/CHANGELOG +++ b/src/dist-static/CHANGELOG @@ -1,3 +1,10 @@ + -- Disabling entity expansions, as we did in v.0.9.5.3 turns out not to be sufficient to prevent all + XML-config parsing related attacks (if an attacker can control the XML config file that will be + parsed). We now make XML parsing much more restrictove by default, but allow users to revert to the + old, permissive pre-0.9.5.3 behavior by setting config property 'com.mchange.v2.c3p0.cfg.xml.usePermissiveParser' + to true. That property replaces and leaves deprecated the 'com.mchange.v2.c3p0.cfg.xml.expandEntityReferences' + property introduced on 0.9.5.3. Many thanks to Aaron Massey (amassey) at HackerOne for calling attention + to the continued vulnerability of XML parsing to these kinds of attacks. -- Address situation where a throwable during forceKillAcquires() left the force_kill_acquires flag set to true, making it impossible for the pool to restart acquisition attempts on recovery. We now unset the flag under any circumstance, but log interrupts or unexpected throwables, and make diff --git a/src/dist-static/RELEASE_NOTES-c3p0-0.9.5.4 b/src/dist-static/RELEASE_NOTES-c3p0-0.9.5.4 new file mode 100644 index 00000000..eb76fe70 --- /dev/null +++ b/src/dist-static/RELEASE_NOTES-c3p0-0.9.5.4 @@ -0,0 +1,56 @@ +RELEASE NOTES, c3p0-0.9.5.4 +=========================== + ++ This minor bugfix release continues to address security issues associated + with parsing configuration in XML format. The solution provided in 0.9.5.3 + was not sufficiently general to address all "XXE" attacks. For more on + those in general, see https://vsecurity.com//download/papers/XMLDTDEntityAttacks.pdf + + The current release includes implementing the suggestions offered here: + https://github.com/OWASP/CheatSheetSeries/blob/31c94f233c40af4237432008106f42a9c4bff05e/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.md + + Because more than entity reference expansion is implicated, the configuration + key introduced in 0.9.5.3... + + com.mchange.v2.c3p0.cfg.xml.expandEntityReferences + + is now deprecated. Instead, if you wish to restore prior versions' permissive + parsing of XML -- and if you strictly control your XML configuration and + understand the security issues enabled by permissive parsing, you can set + + com.mchange.v2.c3p0.cfg.xml.usePermissiveParser=true + + (The deprecated key introduced in 0.9.5.3 will continue to work, but will + provoke warnings.) + + Many thanks to Aaron Massey (amassey) at HackerOne and zhutougg on GitHub + for calling attention to these issues. + ++ The release also includes a change in how c3p0 handles the rare condition + in which + + 1. A round of attempts to acquire Connections fails + 2. c3p0 attempts to wake and throw Exceptions to client threads waiting for + Connections to arrive, but a Thread interrupt or some unexpected Throwable + occurs before that process has completed. + + Previously, a failure within "forceKillAcquires()" would leave the pool in a broken + state, so that after database recovery clients might still fail to acquire Connections. + Now c3p0 uses Thread.interrupt() to make a best-effort attempt to free the clients, + logs a warning, then resets the pool to a recoverable state. + + This should be an extraordinarily rare occurrence, and c3p0 will emit warnings when + it does occur. + + If by some hard-to-understand cause, waiting client Threads become somehow impossible + to wake, despite use of notifyAll(), even after Thread.interrupt(), and even as new + Connections become available to satisfy them, then recovery after the incomplete wake + might lead to more aggressive tha intended (faster) pool growth (as Threads waiting + for new Connections are taken into account when c3p0 calculates its target_pool_size. + But it is very difficult to imagine any condition under which the semantics of the + JVM threading model are respected and yet wait()ing Threads remain stuck despite + notifyAll() and interrupt() calls. + + Many thanks to Stefan Cordes (rscadrde on github), Vipin Nair (swvist on github), and + Łukasz Jąder (ljader on github) for their work on this issue. + diff --git a/src/doc/index.html b/src/doc/index.html index 9a41cf4d..0c89433c 100644 --- a/src/doc/index.html +++ b/src/doc/index.html @@ -2757,17 +2757,21 @@

Locating and Resolving Configur

Due to security concerns surrounding liberal parsing of XML references, - as of c3p0-0.9.5.3, c3p0 by default no longer expands entity references in XML config files. - Entity references may be silently ignored! - However, in the very rare cases where configurations intentionally rely upon entity reference expansion, you can turn it back on. - Installations that understand the full transitive closure of all entity references in their XML config may enable entity reference expansion by setting the following property + c3p0 now XML files extremely restrictively by default. Among other things, it no longer expands entity references in XML config files. + Entity references may be silently ignored! It also no longer support xml includes, and tries to disable any use of inline document type definitions if the JVM's underlying + XML library supports that. + In the very rare cases where configurations intentionally rely upon entity reference expansion, DTDs, XML include, or other dangerous features, you can turn + permissive parsing back on, restoring c3p0's traditional behavior. Be sure you understand the security concerns, + and trust your control over and the integrity of your XML config file, before restoring the old, permissive behavior. Then, if you wish, you may set the following property to true:

- This will restore the traditional, liberal parsing of XML config files that c3p0 utilized by default in versions prior to c3p0-0.9.5.3. + This will restore the traditional, liberal parsing of XML config files that c3p0 utilized by default in versions prior to c3p0-0.9.5.3. (As of version 0.9.5.4, + com.mchange.v2.c3p0.cfg.xml.usePermissiveParser replaces the now-deprecated com.mchange.v2.c3p0.cfg.xml.expandEntityReferences introduced in 0.9.5.3, + because much more than entity reference expansion is now restricted.)

Logging-related properties

diff --git a/src/java/com/mchange/v2/c3p0/cfg/C3P0ConfigXmlUtils.java b/src/java/com/mchange/v2/c3p0/cfg/C3P0ConfigXmlUtils.java index c87bf230..2ad90795 100644 --- a/src/java/com/mchange/v2/c3p0/cfg/C3P0ConfigXmlUtils.java +++ b/src/java/com/mchange/v2/c3p0/cfg/C3P0ConfigXmlUtils.java @@ -115,13 +115,7 @@ private final static void warnCommonXmlConfigResourceMisspellings() } - // thanks to zhutougg on GitHub https://github.com/zhutougg/c3p0/commit/2eb0ea97f745740b18dd45e4a909112d4685f87b - // let's address hazards associated with overliberal parsing of XML, CVE-2018-20433 - // - // by default entity references will not be expanded, but callers can specify expansion if they wish (important - // to retain backwards compatibility with existing config files where users understand the risks) - - public static C3P0Config extractXmlConfigFromDefaultResource( boolean expandEntityReferences ) throws Exception + public static C3P0Config extractXmlConfigFromDefaultResource( boolean usePermissiveParser ) throws Exception { InputStream is = null; @@ -134,7 +128,7 @@ public static C3P0Config extractXmlConfigFromDefaultResource( boolean expandEnti return null; } else - return extractXmlConfigFromInputStream( is, expandEntityReferences ); + return extractXmlConfigFromInputStream( is, usePermissiveParser ); } finally { @@ -147,11 +141,66 @@ public static C3P0Config extractXmlConfigFromDefaultResource( boolean expandEnti } } - public static C3P0Config extractXmlConfigFromInputStream(InputStream is, boolean expandEntityReferences) throws Exception + private static void attemptSetFeature( DocumentBuilderFactory dbf, String featureUri, boolean setting ) + { + try { dbf.setFeature( featureUri, setting ); } + catch (ParserConfigurationException e) + { + if ( logger.isLoggable( MLevel.FINE ) ) + logger.log(MLevel.FINE, "Attempted but failed to set presumably unsupported feature '" + featureUri + "' to " + setting + "."); + } + } + + // thanks to zhutougg on GitHub https://github.com/zhutougg/c3p0/commit/2eb0ea97f745740b18dd45e4a909112d4685f87b + // let's address hazards associated with overliberal parsing of XML, CVE-2018-20433 + // + // by default entity references will not be expanded, but callers can specify expansion if they wish (important + // to retain backwards compatibility with existing config files where users understand the risks) + // + // -=-=-=- + // + // disabling entity expansions turns out not to be sufficient to prevent attacks (if an attacker can control the + // XML config file that will be parsed). we now enable a wide variety of restrictions by default, but allow users + // to revert to the old behavior by setting usePermissiveParser to 'true' + // + // Many thanks to Aaron Massey (amassey) at HackerOne for calling attention to the continued vulnerability, + // and to Dominique Righetto (righettod on GitHub) for + // + // https://github.com/OWASP/CheatSheetSeries/blob/31c94f233c40af4237432008106f42a9c4bff05e/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.md + // (via Aaron Massey) + // + // for instructions on how to overkill the fix + + private static void cautionDocumentBuilderFactory( DocumentBuilderFactory dbf ) + { + // the big one, if possible disable doctype declarations entirely + attemptSetFeature(dbf, "http://apache.org/xml/features/disallow-doctype-decl", true); + + // for a varety of libraries, disable external general entities + attemptSetFeature(dbf, "http://xerces.apache.org/xerces-j/features.html#external-general-entities", false); + attemptSetFeature(dbf, "http://xerces.apache.org/xerces2-j/features.html#external-general-entities", false); + attemptSetFeature(dbf, "http://xml.org/sax/features/external-general-entities", false); + + // for a variety of libraries, disable external parameter entities + attemptSetFeature(dbf, "http://xerces.apache.org/xerces-j/features.html#external-parameter-entities", false); + attemptSetFeature(dbf, "http://xerces.apache.org/xerces2-j/features.html#external-parameter-entities", false); + attemptSetFeature(dbf, "http://xml.org/sax/features/external-parameter-entities", false); + + // if possible, disable external DTDs + attemptSetFeature(dbf, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + + // disallow xinclude resolution + dbf.setXIncludeAware(false); + + // disallow entity reference expansion in general + dbf.setExpandEntityReferences( false ); + } + + public static C3P0Config extractXmlConfigFromInputStream(InputStream is, boolean usePermissiveParser) throws Exception { DocumentBuilderFactory fact = DocumentBuilderFactory.newInstance(); - fact.setExpandEntityReferences( expandEntityReferences ); + if (! usePermissiveParser ) cautionDocumentBuilderFactory( fact ); DocumentBuilder db = fact.newDocumentBuilder(); Document doc = db.parse( is ); diff --git a/src/java/com/mchange/v2/c3p0/cfg/DefaultC3P0ConfigFinder.java b/src/java/com/mchange/v2/c3p0/cfg/DefaultC3P0ConfigFinder.java index afca338c..537b0b7d 100644 --- a/src/java/com/mchange/v2/c3p0/cfg/DefaultC3P0ConfigFinder.java +++ b/src/java/com/mchange/v2/c3p0/cfg/DefaultC3P0ConfigFinder.java @@ -43,9 +43,10 @@ public class DefaultC3P0ConfigFinder implements C3P0ConfigFinder { - final static String XML_CFG_FILE_KEY = "com.mchange.v2.c3p0.cfg.xml"; - final static String XML_CFG_EXPAND_ENTITY_REFS_KEY = "com.mchange.v2.c3p0.cfg.xml.expandEntityReferences"; - final static String CLASSLOADER_RESOURCE_PREFIX = "classloader:"; + final static String XML_CFG_FILE_KEY = "com.mchange.v2.c3p0.cfg.xml"; + final static String XML_CFG_EXPAND_ENTITY_REFS_KEY = "com.mchange.v2.c3p0.cfg.xml.expandEntityReferences"; // deprecated, defaults to false + final static String XML_CFG_USE_PERMISSIVE_PARSER_KEY = "com.mchange.v2.c3p0.cfg.xml.usePermissiveParser"; // defaults to false + final static String CLASSLOADER_RESOURCE_PREFIX = "classloader:"; final static MLogger logger = MLog.getLogger( DefaultC3P0ConfigFinder.class ); @@ -69,11 +70,11 @@ public C3P0Config findConfig() throws Exception flatDefaults.putAll( C3P0ConfigUtils.extractC3P0PropertiesResources() ); String cfgFile = C3P0Config.getPropsFileConfigProperty( XML_CFG_FILE_KEY ); - boolean expandEntityReferences = findExpandEntityReferences(); + boolean usePermissiveParser = findUsePermissiveParser(); if (cfgFile == null) { - C3P0Config xmlConfig = C3P0ConfigXmlUtils.extractXmlConfigFromDefaultResource( expandEntityReferences ); + C3P0Config xmlConfig = C3P0ConfigXmlUtils.extractXmlConfigFromDefaultResource( usePermissiveParser ); if (xmlConfig != null) { insertDefaultsUnderNascentConfig( flatDefaults, xmlConfig ); @@ -114,7 +115,7 @@ public C3P0Config findConfig() throws Exception mbOverrideWarning( "file", cfgFile ); } - C3P0Config xmlConfig = C3P0ConfigXmlUtils.extractXmlConfigFromInputStream( is, expandEntityReferences ); + C3P0Config xmlConfig = C3P0ConfigXmlUtils.extractXmlConfigFromInputStream( is, usePermissiveParser ); insertDefaultsUnderNascentConfig( flatDefaults, xmlConfig ); out = xmlConfig; } @@ -146,16 +147,43 @@ private void mbOverrideWarning( String srcType, String srcName ) logger.log( MLevel.WARNING, "Configuation defined in " + srcType + "'" + srcName + "' overrides all other c3p0 config." ); } - private boolean findExpandEntityReferences() + private static boolean affirmativelyTrue( String propStr ) + { return (propStr != null && propStr.trim().equalsIgnoreCase("true")); } + + private boolean findUsePermissiveParser() { - String check = C3P0Config.getPropsFileConfigProperty( XML_CFG_EXPAND_ENTITY_REFS_KEY ); - boolean out = (check != null && check.trim().equalsIgnoreCase("true")); + boolean deprecatedExpandEntityRefs = affirmativelyTrue( C3P0Config.getPropsFileConfigProperty( XML_CFG_EXPAND_ENTITY_REFS_KEY ) ); + boolean usePermissiveParser = affirmativelyTrue( C3P0Config.getPropsFileConfigProperty( XML_CFG_USE_PERMISSIVE_PARSER_KEY ) ); + + boolean out = usePermissiveParser || deprecatedExpandEntityRefs; + if ( out && logger.isLoggable( MLevel.WARNING ) ) + { + String warningKey; + if ( deprecatedExpandEntityRefs ) + { + logger.log( MLevel.WARNING, + "You have set the configuration property '" + XML_CFG_EXPAND_ENTITY_REFS_KEY + "', which has been deprecated, to true. " + + "Please use '" + XML_CFG_USE_PERMISSIVE_PARSER_KEY + "' instead. " + + "Please be aware that permissive parsing enables inline document type definitions, XML inclusions, and other fetures!"); + if ( usePermissiveParser ) + warningKey = "Configuration property '" + XML_CFG_USE_PERMISSIVE_PARSER_KEY + "'"; + else + warningKey = "Configuration property '" + XML_CFG_EXPAND_ENTITY_REFS_KEY + "' (deprecated)"; + } + else + warningKey = "Configuration property '" + XML_CFG_USE_PERMISSIVE_PARSER_KEY + "'"; + logger.log( MLevel.WARNING, - "Configuration property '" + XML_CFG_EXPAND_ENTITY_REFS_KEY + "' is set to 'true'. " + - "Entity references will be resolved in XML c3p0 configuration files. This may be a security hazard. " + - "Be sure you understand your XML config files, including the full transitive closure of entity references. " + - "See CVE-2018-20433, https://nvd.nist.gov/vuln/detail/CVE-2018-20433"); + warningKey + " is set to 'true'. " + + "Entity references will be resolved in XML c3p0 configuration files, doctypes and xml includes will be permitted, the file will in general be parsed very permissively. " + + "This may be a security hazard. " + + "Be sure you understand your XML config files, including the full transitive closure of entity references and incusions. " + + "See CVE-2018-20433, https://nvd.nist.gov/vuln/detail/CVE-2018-20433 / " + + "See also https://github.com/OWASP/CheatSheetSeries/blob/31c94f233c40af4237432008106f42a9c4bff05e/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.md / " + + "See also https://vsecurity.com//download/papers/XMLDTDEntityAttacks.pdf" ); + } + return out; } }