|Summary:||SSI Servlet should support safe configuration|
|Product:||Tomcat 6||Reporter:||Yair Lenga <yair.lenga>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
Replacement for SSIServlet.java SSIProcessor.java SSIFilter.java
Patch to disable exec by default, new allowExec tag
Description Yair Lenga 2010-03-22 16:37:48 UTC
The current configuration of the SSI module is "All" or "None". The "ALL" option will expose all the legacy Apache SSI directive (echo, printenv, if, exec, ...). As documented, allowing SSI will allow execution of arbitrary programs using the "exec". As a result, there is no safe way to expose sites/projects containing SSI directive, without taking a security risk, or reviewing of every file. The "exec" directive, with the cmd option is a major risk. Even for Apache, you have the option to allowing the "safe" include (includeNoExec). The includeNoExec allow pages to be served, even when the content is not reviewed, or when users are allowed to upload content to the site. I have a big site which need to be converted into JSP. I would like to use the SSI servlet to allow for transition over time. The extra risk from ( from exec cmd) make it impossible to deploy the SSI. My request: Modify the configuration of SSI as follow: By default, it will only allow "safe" directive (no exec cmd=...). This will eliminate the risk from arbitrary execution of commands ("del *.*"). It will also remove many potentail load problems. The cmd= should only be allowed using a directive like "allowUnsafeExec", which will default to false. I think that the change will make it easier to use the SSI feature, without exposing the server to big risk. The risk associated with the "safer" version of SSI is similar to the risk from running JSP pages.
Comment 1 Mark Thomas 2010-03-22 17:11:06 UTC
Patches for enhancements are always welcome
Comment 2 Yair Lenga 2010-03-22 18:06:18 UTC
Created attachment 25166 [details] Replacement for SSIServlet.java SSIProcessor.java SSIFilter.java Attached is a quick fix that adds 'allowExec' parameter to the SSI servlet and filter. I could not build the complete Tomcat tree - I'll be happy to test any patched version with this (or similar change). Overall < 50 lines of changes. Basic logic: remove the exec command from the SSIProcessor, unless the allow_exec is true.
Comment 3 Yair Lenga 2010-03-22 18:10:55 UTC
Will you take a patch for Tomcat 5.5 ? I'm using RedHat5, which has a Tomcat 5.5 bundled in. It's much easier to get a security upgrade installed, than to get a new version upgrade.
Comment 4 Mark Thomas 2010-06-29 12:45:54 UTC
*** Bug 49520 has been marked as a duplicate of this bug. ***
Comment 5 Yair Lenga 2010-07-09 10:01:32 UTC
(In reply to comment #4) > *** Bug 49520 has been marked as a duplicate of this bug. *** Mark, Is there anything I can do to speed up the inclusion of this change ? I've noticed it did not make it for 6.0.28, where few other CGI/SSI related changed were incorporated. I would love to use the SSI, but I can not use it because of the security reisk of the "unsafe" include/exec.
Comment 6 Mark Thomas 2010-07-09 10:17:16 UTC
Providing patches in diff -u format would help.
Comment 7 Yair Lenga 2010-07-13 15:52:28 UTC
Created attachment 25760 [details] Patch to disable exec by default, new allowExec tag Patch for three files, created against 6.0.26-src
Comment 8 Mark Thomas 2010-07-13 17:16:40 UTC
The diff is inverted and the patch is using tabs rather than spaces. I should eb able to work with that but you might need to fix it.
Comment 9 Mark Thomas 2010-07-13 17:38:55 UTC
In the end I used the patch a guide and write a new one. Some additional comments: - if you do an svn diff against a normal source tree patches usually apply cleanly - new features should be documented The patch has been applied to truck and proposed for 6.0.x
Comment 10 Yair Lenga 2010-07-13 17:54:53 UTC
Mark, Thanks for taking the change. I'll follow you suggestions regarding svn diff for the next time. Do I have to submit anything for the change to flow to 7.X ? Yair
Comment 11 Mark Thomas 2010-07-13 18:08:34 UTC
Sorry truck should have been trunk and trunk == 7.0.x so it is already there.
Comment 12 Mark Thomas 2010-07-16 06:23:49 UTC
Fixed in 6.0.x and will be included in 6.0.29 onwards.