Monday, January 16, 2017, 12:22:51 PM, Christoph Rüger wrote: [snip] >> We basically chose to forbid ExternalEntities completely by extending >> NodeModel and so set some properties to the DocumentBuilderFactory as >> described here: >> https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#JAXP_DocumentBuilderFactory.2C_SAXParserFactory_and_DOM4J > > Note sure what you can do with extending NodeModel, because > getDocumentBuilderFactory is static. As far as I see, all we (at > FreeMarker) can do is adding a boolean disableExternalEntityResolution > parameter to that static method and to the also static NodeModel.parse > methods, and JavaDoc the dangers clearly there. > > A parameter disableExternalEntityResolution would be helpful. [snip]
As of FM3, I think we should just remove NodeModel.parse. I'm sure that there are libraries that deal with streamlining (safe) DOM loading and such, if someone needs that. It was kind of nice that we also call simplify() there, but it's really not a big deal, as instead you can just load the DOM however you like, then pass it to NodeModel.simplify(Node) and you are done. For such a minor convenience, we shouldn't pull yet another thing on ourselves that has nothing to do with the duty of a template engine. I mean it's not our business to solve that bare SAX isn't very convenient, or work out the security subtleties of parsing XML with SAX. As of FM2, I have looked into this, and found that the workaround on OWASP is quite "hacky", like it assumes that I'm using Xalan, etc. Also I don't get why don't they just recommend using a custom entity resolved that bans everything (or better, everything that's not white-listed by the application), instead of trying to ban XML features that are mandatory per spec. I'm not sure they are doing the right thing. So, I have decided that I don't want to solve this on the FreeMarker level. It's just not in the scope of FreeMarker, it's not our expertise. If we integrate something like this, suddenly we have to guarantee that it will keep working forever (like testing if the Xalan features still work in environment X), we had to respond for why it works the way it does, etc. So, instead, I have JavaDoc-et it clearly when this convenience method is safe to use, and how to substitute it in other cases. And thanks for pointing this out! As of extending things in your case, there's nothing that you can change in this regard with extending, so I suspect you actually didn't do that to solve this. (I have also deprecated the static setter nonsense, where you can set the DOMBuilderFactory for example.) -- Thanks, Daniel Dekany
