ist-dresden / composum-nodes

Set of Apache Sling / AEM development tools: JCR browser, user and package management and more
https://www.composum.com/home/nodes.html
MIT License
55 stars 21 forks source link

Query does not work if the XSS Filter is configured with specific policies #295

Closed royteeuwen closed 1 year ago

royteeuwen commented 1 year ago

The NodeServlet QueryOperation fetches the "query" parameter by using the RequestUtil, which uses the XSSFilter to filter the incoming parameter. This causes issues if specific policies are set for the XSS Filter (https://github.com/ist-dresden/composum-nodes/blob/develop/console/src/main/java/com/composum/sling/nodes/servlet/NodeServlet.java#L423)

Question, is it logical to use the XSS filter for a query in the Composum nodes interface? Would it be ok to disable this one way or another?

stoerr commented 1 year ago

Interesting question. The XSSFilter is used in many places, and as a rule is used to filter all input. Hopefully the JCR is safe against malicious input there, though I can't be sure. The query is output again in the result, so some XSS filtering is certainly needed, though that possibly could be done on the output, instead, though filtering the input IMHO feels like the more secure way in the long term. Especially since the browser will probably often be used by admin users. Can you give an example what the XSS Filter does there in your case? Perhaps even a config file, to try it out ourselves?

royteeuwen commented 1 year ago

@stoerr What happens is the following:

Input: /jcr:root/home/users//element(*,rep:User)[@rep:authorizable='roy@teeuwen.be'] Result:

java.text.ParseException: Query: /jcr:root/home/users//element(*,rep:User)[@(*)rep:authorizable='roy@teeuwen.be']; 
expected: ]query: '/jcr:root/home/users//element(*,rep:User)[@rep:authorizable='roy@teeuwen.be']'

I don't see a valid fix for this, I don't want to disable XSS Filtering for the rest of AEM.

The weird thing is, I have the issue on 1 AEMaaCS customer but not on another, while we didn't set any custom XSS settings (that I could find), the only difference being that they are a different AEMaaCS version..

Anything you can think of about settings that could give you more info?

stoerr commented 1 year ago

@ist-rw : Well, if XSS.filter encodes a @ then this is obviously deadly for a query. https://benhoyt.com/writings/dont-sanitize-do-escape/ makes a point to take care to escape things on output properly, which definitely has a point here. Since there are also other scenarios where XSS.filter can harm things (conceiveably in jcr:like or jcr:contains) I removed the XSS.filter for the query and checked again that query is properly escaped in the output variants.

royteeuwen commented 1 year ago

@stoerr any luck on getting that sling-12 release automated / released ;)?

stoerr commented 1 year ago

Sorry, I'm on it. :-) There were quite a lot of surprising detail problems, but I already managed to make a release of Nodes 3.4.4 . For 4.x I'd like to do some more restructuring, including a release of the parent project, but I'm pretty close now.

stoerr commented 1 year ago

Good news - there is a new release https://github.com/ist-dresden/composum-nodes/releases/tag/composum-nodes-4.2.1 now. I've also branched off the old develop to release/sling-11 and merged release/sling-12 into develop. The develop is now the main branch for improvements, in sling-11 we probably won't do anything unless somebody explicitly asks. Sorry that that took a while, but creating releases is just the push of a button in the future, which will likely speed up things.

royteeuwen commented 1 year ago

Awesome!