|Summary:||Disable creation of command line parameters from GET parameters in the URL for CGIServlet|
|Product:||Tomcat 9||Reporter:||jm009 <jan0michael>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
Make creation of command line parameters from GET parameters optional
Make evaluation of enableCmdLineArguments in outer if.
Description jm009 2017-09-05 07:10:35 UTC
Created attachment 35290 [details] Make creation of command line parameters from GET parameters optional The CGI RFC says, that the server SHOULD create command line arguments from certain GET parameters. https://tools.ietf.org/html/rfc3875#section-4.4 4.4. The Script Command Line I don't like this, because I think, this can be a security risk in certain cases. I suggest to disable this feature by default, or at least allow to disable it by configuration. The proposed patch makes this feature configurable. The line private boolean enableCmdLineArguments = false; makes the feature disabled by default. Putting "= true" would make it enabled by default.
Comment 1 jm009 2017-09-05 07:35:48 UTC
Why I suggest to disable this feature by default: 1) I never saw a servlet that uses this feature 2) I suppose, this feature comes from the beginnings of the internet, when people wanted to run some system command by clicking on a link, and the term "security" was not yet used in computer science :-)
Comment 2 jm009 2017-09-05 09:14:15 UTC
Created attachment 35293 [details] Make evaluation of enableCmdLineArguments in outer if. In fact the evaluation of "boolean enableCmdLineArguments" can happen in the outer if block.
Comment 3 Christopher Schultz 2017-09-05 15:13:38 UTC
I like the idea of locking this down, but requiring a rebuild-from-source to change the setting isn't acceptable. Can you add a new <init-param> to control this behavior? +1 for making it DISABLED by default (i.e. DO NOT CREATE COMMAND-LINE ARGUMENTS by default).
Comment 4 jm009 2017-09-05 16:40:19 UTC
I don't understand... My patch already uses <init-param>. The patch as it is makes the feature disabled by default.
Comment 5 Christopher Schultz 2017-09-05 20:21:45 UTC
(In reply to jm009 from comment #4) > I don't understand... > My patch already uses <init-param>. Sorry, you are correct. I did not read the patch closely enough.
Comment 6 Mark Thomas 2017-09-08 09:43:15 UTC
I've applied the patch but I do have some feedback for future patches that will make applying your patches quicker and simpler for committers: - Enable checkstyle and ensure the build is clean after your changes - You'll want to configure your IDE to insert 4 spaces rather than a tab - Patches that add configuration options should also include appropriate additions to the documentation To avoid breakage for existing users that may depend on this functionality, it is only disabled by default in 9.0.x onwards.
Comment 7 jm009 2017-09-15 07:56:14 UTC
Thank you for the feedback and for applying the patch.
Comment 8 Mark Thomas 2017-09-15 08:12:44 UTC
np and thanks for the patch - just realised I forgot to say that before.