Commit 05e8cf52331785435d1154ffb0b93888ed529d77

Authored by Alan Davis
1 parent 87380006c9
Exists in master

REPO-480 Platform XXE protection implements OWASP recommendations

- Add additional features (xIncludeAware and expandEntityReferences) to the configurable values.
   - Remove all the warnings shown in Itellij
src/main/java/org/alfresco/xmlfactory/FactoryHelper.java
... ... @@ -37,31 +37,42 @@ import org.xml.sax.SAXNotSupportedException;
37 37 /**
38 38 * The configuration is taken from https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#Java
39 39 */
40   -public class FactoryHelper
  40 +class FactoryHelper
41 41 {
42 42 private static final Log logger = LogFactory.getLog(FactoryHelper.class);
43 43  
44   - public final static String FEATURE_EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";
45   - public final static String FEATURE_EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";
46   - public final static String FEATURE_USE_ENTITY_RESOLVER2 = "http://xml.org/sax/features/use-entity-resolver2";
47   - public final static String FEATURE_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
48   - public final static String FEATURE_DISALLOW_DOCTYPE = "http://apache.org/xml/features/disallow-doctype-decl";
  44 + final static String FEATURE_EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";
  45 + final static String FEATURE_EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";
  46 + final static String FEATURE_USE_ENTITY_RESOLVER2 = "http://xml.org/sax/features/use-entity-resolver2";
  47 + final static String FEATURE_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
  48 +
  49 + // There is no standards for these values. Note they don't start with http:
  50 + private final static String ADDITIONAL_FEATURE_X_INCLUDE_AWARE = "xIncludeAware";
  51 + private final static String ADDITIONAL_FEATURE_EXPAND_ENTITY_REFERENCES = "expandEntityReferences";
  52 +
  53 + final static String FEATURE_DISALLOW_DOCTYPE = "http://apache.org/xml/features/disallow-doctype-decl";
49 54  
50   - public final static List<String> DEFAULT_FEATURES_TO_DISABLE = Collections.unmodifiableList(new ArrayList<String>(
51   - Arrays.asList(FEATURE_EXTERNAL_GENERAL_ENTITIES,
  55 + final static List<String> DEFAULT_FEATURES_TO_DISABLE = Collections.unmodifiableList(new ArrayList<>(
  56 + Arrays.asList(
  57 + FEATURE_EXTERNAL_GENERAL_ENTITIES,
52 58 FEATURE_EXTERNAL_PARAMETER_ENTITIES,
53 59 FEATURE_USE_ENTITY_RESOLVER2,
54   - FEATURE_LOAD_EXTERNAL_DTD)));
  60 + FEATURE_LOAD_EXTERNAL_DTD,
  61 +
  62 + ADDITIONAL_FEATURE_X_INCLUDE_AWARE,
  63 + ADDITIONAL_FEATURE_EXPAND_ENTITY_REFERENCES)));
55 64  
56   - public final static List<String> DEFAULT_FEATURES_TO_ENABLE = Collections.unmodifiableList(new ArrayList<String>(
57   - Arrays.asList(XMLConstants.FEATURE_SECURE_PROCESSING,
  65 + final static List<String> DEFAULT_FEATURES_TO_ENABLE = Collections.unmodifiableList(new ArrayList<>(
  66 + Arrays.asList(
  67 + XMLConstants.FEATURE_SECURE_PROCESSING,
58 68 FEATURE_DISALLOW_DOCTYPE)));
59 69  
60 70 /* white list of classes that can use the parsers with no security restrictions */
61   - public final static List<String> DEFAULT_WHITE_LIST_CALLERS = Collections.unmodifiableList(new ArrayList<String>(
  71 + final static List<String> DEFAULT_WHITE_LIST_CALLERS = Collections.unmodifiableList(new ArrayList<>(
62 72 Arrays.asList(
63 73 "com.sun.xml.ws.transport.http.servlet.WSServletContextListener",
64 74 "org.springframework.beans.factory.xml.XmlBeanDefinitionReader",
  75 + "org.springframework.beans.factory.support.AbstractBeanFactory",
65 76 "org.apache.myfaces.config.FacesConfigurator",
66 77 "org.hibernate.cfg.Configuration",
67 78 "org.alfresco.ibatis.HierarchicalXMLConfigBuilder",
... ... @@ -70,11 +81,11 @@ public class FactoryHelper
70 81 )));
71 82  
72 83 // Property names used to configure the factories
73   - public static final String FEATURES_TO_ENABLE = "features.to.enable";
74   - public static final String FEATURES_TO_DISABLE = "features.to.disable";
75   - public static final String WHITE_LIST_CALLERS = "white.list.callers";
  84 + static final String FEATURES_TO_ENABLE = "features.to.enable";
  85 + static final String FEATURES_TO_DISABLE = "features.to.disable";
  86 + static final String WHITE_LIST_CALLERS = "white.list.callers";
76 87  
77   - public void configureFactory(DocumentBuilderFactory factory, List<String> featuresToEnable,
  88 + void configureFactory(DocumentBuilderFactory factory, List<String> featuresToEnable,
78 89 List<String> featuresToDisable, List<String> whiteListCallers)
79 90 {
80 91 if (!isCallInWhiteList(whiteListCallers))
... ... @@ -93,11 +104,10 @@ public class FactoryHelper
93 104 setFeature(factory, featureToDisable, false);
94 105 }
95 106 }
96   - applyAdditionalFeatures(factory);
97 107 }
98 108 }
99 109  
100   - public void configureFactory(SAXParserFactory factory, List<String> featuresToEnable,
  110 + void configureFactory(SAXParserFactory factory, List<String> featuresToEnable,
101 111 List<String> featuresToDisable, List<String> whiteListCallers)
102 112 {
103 113 if (!isCallInWhiteList(whiteListCallers))
... ... @@ -116,16 +126,15 @@ public class FactoryHelper
116 126 setFeature(factory, featureToDisable, false);
117 127 }
118 128 }
119   - applyAdditionalFeatures(factory);
120 129 }
121 130 }
122 131  
123 132 private boolean isCallInWhiteList(List<String> whiteListCallers)
124 133 {
125 134 StackTraceElement[] currentStackTrace = (new Exception()).getStackTrace();
126   - for (int i = 0; i < currentStackTrace.length; i++)
  135 + for (StackTraceElement clazz: currentStackTrace)
127 136 {
128   - String currentClassName = currentStackTrace[i].getClassName();
  137 + String currentClassName = clazz.getClassName();
129 138 for (String className : whiteListCallers)
130 139 {
131 140 if (currentClassName.equals(className))
... ... @@ -138,36 +147,22 @@ public class FactoryHelper
138 147 return false;
139 148 }
140 149  
141   - private void applyAdditionalFeatures(DocumentBuilderFactory factory)
142   - {
143   - try
144   - {
145   - factory.setXIncludeAware(false);
146   - factory.setExpandEntityReferences(false);
147   - }
148   - catch (Exception e)
149   - {
150   - logConfigurationFailure(factory.getClass().getName(), e);
151   - }
152   - }
153   -
154   - private void applyAdditionalFeatures(SAXParserFactory factory)
155   - {
156   - try
157   - {
158   - factory.setXIncludeAware(false);
159   - }
160   - catch (Exception e)
161   - {
162   - logConfigurationFailure(factory.getClass().getName(), e);
163   - }
164   - }
165   -
166 150 private void setFeature(DocumentBuilderFactory factory, String feature, boolean enable)
167 151 {
168 152 try
169 153 {
170   - factory.setFeature(feature, enable);
  154 + if (ADDITIONAL_FEATURE_X_INCLUDE_AWARE.equals(feature))
  155 + {
  156 + factory.setXIncludeAware(enable);
  157 + }
  158 + else if (ADDITIONAL_FEATURE_EXPAND_ENTITY_REFERENCES.equals(feature))
  159 + {
  160 + factory.setExpandEntityReferences(enable);
  161 + }
  162 + else
  163 + {
  164 + factory.setFeature(feature, enable);
  165 + }
171 166 }
172 167 catch (ParserConfigurationException pce)
173 168 {
... ... @@ -179,19 +174,18 @@ public class FactoryHelper
179 174 {
180 175 try
181 176 {
182   - factory.setFeature(feature, enable);
183   - }
184   - catch (ParserConfigurationException pce)
185   - {
186   - logConfigurationFailure(factory.getClass().getName(), feature, pce);
187   - }
188   - catch (SAXNotRecognizedException nre)
189   - {
190   - logConfigurationFailure(factory.getClass().getName(), feature, nre);
  177 + if (ADDITIONAL_FEATURE_X_INCLUDE_AWARE.equals(feature))
  178 + {
  179 + factory.setXIncludeAware(enable);
  180 + }
  181 + else if (!ADDITIONAL_FEATURE_EXPAND_ENTITY_REFERENCES.equals(feature)) // Does not exist on SAXParserFactory
  182 + {
  183 + factory.setFeature(feature, enable);
  184 + }
191 185 }
192   - catch (SAXNotSupportedException nse)
  186 + catch (ParserConfigurationException | SAXNotSupportedException | SAXNotRecognizedException e)
193 187 {
194   - logConfigurationFailure(factory.getClass().getName(), feature, nse);
  188 + logConfigurationFailure(factory.getClass().getName(), feature, e);
195 189 }
196 190 }
197 191  
... ... @@ -203,14 +197,6 @@ public class FactoryHelper
203 197 }
204 198 }
205 199  
206   - private void logConfigurationFailure(String factoryName, Exception e)
207   - {
208   - if (logger.isWarnEnabled())
209   - {
210   - logger.warn("Failed to configure " + factoryName, e);
211   - }
212   - }
213   -
214 200 /**
215 201 * Returns a List of features (to be enabled or disabled) or class names (to be included in a caller white list) for
216 202 * a factory. A similar approach to the one used to select the JAXP factories in the first place is used to find a
... ... @@ -227,10 +213,9 @@ public class FactoryHelper
227 213 * @param defaultFeatures to be returned if other values are not found.
228 214 * @return the list of features or class names.
229 215 */
230   - public List<String> getConfiguration(Class<?> factoryClass, String propertyName, List<String> defaultFeatures)
  216 + List<String> getConfiguration(Class<?> factoryClass, String propertyName, List<String> defaultFeatures)
231 217 {
232 218 List<String> features = defaultFeatures;
233   - ClassLoader loader = null;
234 219  
235 220 String factoryName = factoryClass.getName();
236 221 String extendedPropertyName = factoryName+'.'+propertyName;
... ... @@ -249,14 +234,13 @@ public class FactoryHelper
249 234 // Look for values in $JAVA_HOME/jre/lib/<factoryName>.properties.
250 235 if (value == null)
251 236 {
252   - URL url = null;
253 237 String javaHome = getJavaHome();
254 238 if (javaHome != null)
255 239 {
256 240 File file = new File(new File(new File(javaHome), "lib"), factoryName+".properties");
257 241 try
258 242 {
259   - url = file.toURI().toURL();
  243 + URL url = file.toURI().toURL();
260 244 value = getProperty(url, propertyName);
261 245 }
262 246 catch (MalformedURLException e)
... ... @@ -270,7 +254,7 @@ public class FactoryHelper
270 254 if (value == null)
271 255 {
272 256 String resourceName = "META-INF/services/" + factoryName+".properties";
273   - URL url = getResource(loader, resourceName);
  257 + URL url = getResource(null, resourceName);
274 258 value = getProperty(url, propertyName);
275 259 }
276 260  
... ... @@ -293,7 +277,7 @@ public class FactoryHelper
293 277  
294 278 String getJavaHome()
295 279 {
296   - return System.getenv("JAVA_HOME");
  280 + return System.getProperty("java.home");
297 281 }
298 282  
299 283 String getSystemProperty(String propertyName)
... ...
src/test/java/org/alfresco/xmlfactory/AppTest.java
... ... @@ -100,6 +100,55 @@ public class AppTest
100 100 assertFalse(spf.isXIncludeAware());
101 101 }
102 102  
  103 + /**
  104 + * Test we have set features the way we expect as defaults.
  105 + */
  106 + public void testDocumentBuilderFactoryInWhiteList() throws Throwable
  107 + {
  108 + // Using constructor rather than the service locator and then using the helper to configure it.
  109 + DocumentBuilderFactory dbf = new DocumentBuilderFactoryImpl();
  110 + FactoryHelper factoryHelper = new FactoryHelper();
  111 + List<String> whiteListClasses = Collections.singletonList(getClass().getName());
  112 + factoryHelper.configureFactory(dbf, FactoryHelper.DEFAULT_FEATURES_TO_ENABLE,
  113 + FactoryHelper.DEFAULT_FEATURES_TO_DISABLE,
  114 + whiteListClasses);
  115 +
  116 + assertFalse(dbf.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING));
  117 + assertFalse(dbf.getFeature(FactoryHelper.FEATURE_DISALLOW_DOCTYPE));
  118 +
  119 + assertTrue(dbf.getFeature(FactoryHelper.FEATURE_EXTERNAL_GENERAL_ENTITIES));
  120 + assertTrue(dbf.getFeature(FactoryHelper.FEATURE_EXTERNAL_PARAMETER_ENTITIES));
  121 + assertTrue(dbf.getFeature(FactoryHelper.FEATURE_USE_ENTITY_RESOLVER2));
  122 + assertTrue(dbf.getFeature(FactoryHelper.FEATURE_LOAD_EXTERNAL_DTD));
  123 +
  124 + assertTrue(dbf.isExpandEntityReferences());
  125 + assertFalse(dbf.isXIncludeAware()); // false is the default so is same as the non whitelist test
  126 + }
  127 +
  128 + /**
  129 + * Test we have set features the way we expect as defaults.
  130 + */
  131 + public void testSAXParserFactoryInWhiteList() throws Throwable
  132 + {
  133 + // Using constructor rather than the service locator and then using the helper to configure it.
  134 + SAXParserFactory spf = new SAXParserFactoryImpl();
  135 + FactoryHelper factoryHelper = new FactoryHelper();
  136 + List<String> whiteListClasses = Collections.singletonList(getClass().getName());
  137 + factoryHelper.configureFactory(spf, FactoryHelper.DEFAULT_FEATURES_TO_ENABLE,
  138 + FactoryHelper.DEFAULT_FEATURES_TO_DISABLE,
  139 + whiteListClasses);
  140 +
  141 + assertFalse(spf.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING));
  142 + assertFalse(spf.getFeature(FactoryHelper.FEATURE_DISALLOW_DOCTYPE));
  143 +
  144 + assertTrue(spf.getFeature(FactoryHelper.FEATURE_EXTERNAL_GENERAL_ENTITIES));
  145 + assertTrue(spf.getFeature(FactoryHelper.FEATURE_EXTERNAL_PARAMETER_ENTITIES));
  146 + assertTrue(spf.getFeature(FactoryHelper.FEATURE_USE_ENTITY_RESOLVER2));
  147 + assertTrue(spf.getFeature(FactoryHelper.FEATURE_LOAD_EXTERNAL_DTD));
  148 +
  149 + assertFalse(spf.isXIncludeAware()); // false is the default so is same as the non whitelist test
  150 + }
  151 +
103 152 private class TestFactoryHelper extends FactoryHelper
104 153 {
105 154 final Map<String, Properties> testValues = new HashMap<>();
... ... @@ -169,55 +218,6 @@ public class AppTest
169 218 }
170 219  
171 220 /**
172   - * Test we have set features the way we expect as defaults.
173   - */
174   - public void testDocumentBuilderFactoryInWhiteList() throws Throwable
175   - {
176   - // Using constructor rather than the service locator and then using the helper to configure it.
177   - DocumentBuilderFactory dbf = new DocumentBuilderFactoryImpl();
178   - FactoryHelper factoryHelper = new FactoryHelper();
179   - List<String> whiteListClasses = Collections.singletonList(getClass().getName());
180   - factoryHelper.configureFactory(dbf, FactoryHelper.DEFAULT_FEATURES_TO_ENABLE,
181   - FactoryHelper.DEFAULT_FEATURES_TO_DISABLE,
182   - whiteListClasses);
183   -
184   - assertFalse(dbf.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING));
185   - assertFalse(dbf.getFeature(FactoryHelper.FEATURE_DISALLOW_DOCTYPE));
186   -
187   - assertTrue(dbf.getFeature(FactoryHelper.FEATURE_EXTERNAL_GENERAL_ENTITIES));
188   - assertTrue(dbf.getFeature(FactoryHelper.FEATURE_EXTERNAL_PARAMETER_ENTITIES));
189   - assertTrue(dbf.getFeature(FactoryHelper.FEATURE_USE_ENTITY_RESOLVER2));
190   - assertTrue(dbf.getFeature(FactoryHelper.FEATURE_LOAD_EXTERNAL_DTD));
191   -
192   - assertTrue(dbf.isExpandEntityReferences());
193   - assertFalse(dbf.isXIncludeAware()); // false is the default so is same as the non whitelist test
194   - }
195   -
196   - /**
197   - * Test we have set features the way we expect as defaults.
198   - */
199   - public void testSAXParserFactoryInWhiteList() throws Throwable
200   - {
201   - // Using constructor rather than the service locator and then using the helper to configure it.
202   - SAXParserFactory spf = new SAXParserFactoryImpl();
203   - FactoryHelper factoryHelper = new FactoryHelper();
204   - List<String> whiteListClasses = Collections.singletonList(getClass().getName());
205   - factoryHelper.configureFactory(spf, FactoryHelper.DEFAULT_FEATURES_TO_ENABLE,
206   - FactoryHelper.DEFAULT_FEATURES_TO_DISABLE,
207   - whiteListClasses);
208   -
209   - assertFalse(spf.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING));
210   - assertFalse(spf.getFeature(FactoryHelper.FEATURE_DISALLOW_DOCTYPE));
211   -
212   - assertTrue(spf.getFeature(FactoryHelper.FEATURE_EXTERNAL_GENERAL_ENTITIES));
213   - assertTrue(spf.getFeature(FactoryHelper.FEATURE_EXTERNAL_PARAMETER_ENTITIES));
214   - assertTrue(spf.getFeature(FactoryHelper.FEATURE_USE_ENTITY_RESOLVER2));
215   - assertTrue(spf.getFeature(FactoryHelper.FEATURE_LOAD_EXTERNAL_DTD));
216   -
217   - assertFalse(spf.isXIncludeAware()); // false is the default so is same as the non whitelist test
218   - }
219   -
220   - /**
221 221 * Test the pick up of configuration properties. We just overload the methods in the helper as putting files in
222 222 * the jre structure or META_INF is not a good idea in an automated test. Has been manually tested. This tests
223 223 * the code around the actual reading of property files and system properties.
... ...