tracker issue : CF-3710326

POSSIBLY only switch it on if the server is installed with the "secure profile"? -- Adam

Comment by External U.

Definitely would prefer to see this handled via Admin UI. I do not see how this is a security issue. Of all the attacks on CF servers we have seen, cfinclude has never been the avenue.

Vote by External U.

Passing bug to management team for review. (Comment added from ex-user id:vnigam)

Comment by Adobe D.

Thanks Viny. Pls keep us posted. -- Adam

Comment by External U.

I'm not opposed to having the capability to limit what you can cfinclude - there is no harm in having that capability. However, having it default to 'none of the above' and not be something that can be modified in the admin (and/or in code, say... in the Application.cfc, etc.) is just plain improper execution in my book.

Vote by External U.

Denard, they have to have a valid and reasonable use case before implementing any functionality. The "harm" here is I'm not sure Adobe know what they're doing, which means they need to spend more time thinking things through (and if necessary consulting with people who are actually familiar and expert in CFML development) before adding functionality. -- Adam

Comment by External U.

The reason why we stopped supporting other files in cfinclude was of course because of security. And the root cause was - we compile the included file as a cfm. The file type does not matter at all. So even if the included file is htm, js, css, txt file or a log file, everything gets compiled assuming it to be a cfml content. The current fix blocks everything, which I agree is not correct. The correct fix should be - apart from cfm, all other files should be statically included (not compiled). The allowedextforinclude will take the list of extensions which one would like to get compiled as cfm.

Comment by Rupesh K.

> The reason why we stopped supporting other files in cfinclude was of course because of security. And yet nowhere have I seen any actual or theoretical security risks that make this limitation valid. The only claim of any potential risk put forward is IF an ignorant programmer uses unchecked user input for the template attribute - but the file extension is irrelevant to that risk (i.e. it's at least an equal risk doing #userinput#.cfm vs #userinput#.js - or rather, a bigger risk, because it's far more likely that a CFML file will contain security-relevant logic, as opposed to simply cfoutput blocks) - in any case that's no more a risk than having <cfquery>#form.sql#</cfquery> and the solution to both is GIVING NEW DEVELOPERS THE KNOWLEDGE THEY LACK. > The correct fix should be - apart from cfm, all other files should be statically included (not compiled). No, the correct fix would be: 1) Educate developers to not do stupid things. Validating and encoding user input should be one of the first things that programmers are taught to do (for any language), yet many of the examples on the CF documentation either ignore it, or - even worse - do it wrong! 2) Provide developers with the tools to make their own decision. Which means leaving existing cfinclude functionality alone. If a developer doesn't want a file compiled they can use FileRead() instead - if a developer is specifically including a file over reading it, it should be assumed they know what they are doing and want it compiled. If developers don't know that cfinclude compiles the file then, again, this is a failure in the documentation, not a reason to add unnecessary roadblocks.

Comment by External U.

Remove the limitation - this breaks existing applications and has not been proven a necessary or valid change.

Vote by External U.

Thanks Peter, you pretty much summed up what I was going to say. You still need to demonstrate the ACTUAL security risk, Rupesh. You can't simply say "of course because of security". Unless you can demonstrate an actual, non-theoretical vector here, AND that this approach is the best way to deal with it: you should remove this functionality. -- Adam

Comment by External U.

Sorry for double-up: am on a bad connection and reloaded the form submission to get the page back. You really should protect against duplicate form submissions! Feel free to delete this and the one comment immediately before it. -- Adam

Comment by External U.

From what we have seen, cfinclude is used in two cases - static include of files such as JS, CSS, HTM, txt etc - include of cfm content which need to be compiled. The new change which we are going to make is merely giving you a control so that you decide in which case you want the content to be compiled and in which case you don't. The setting would be there in the application as well as in the administrator. Are you suggesting that the first use case is an invalid use case and we should *always* compile the content irrespective of the file type being included. Is there any reason why you want HTML, JS, CSS, TXT, LOG etc to be compiled, converted to a class file and then execute?

Comment by Rupesh K.

Yes. As it's a code-time thing, I think whether or not there's a perceived security issue (which I still can't see, TBH), should be left in the hands of the developer. I still cannot see the vector whether it's a CFM, JS or CSS (etc) file. I think you're addressing a non-issue. If we agree it's a non-issue, the functionality should be removed. Given it's a "change" it itself is a vector for regressions. Do we agree? Thanks for the follow-up, Rupesh. -- Adam

Comment by External U.

Added the new change. The new behavior will be 1) By default only cfm files gets compiled in cfinclude tag and all other file types gets included statically. 2) The key allowedextforinclude now can be specified at the application as well. 3) Added the setting to Server settings -> settings page 4) If wildcard(*) is specified in the value all files will be compiled. Wildcard support is added at both server and application level.

Comment by S V.

(1) is still a regression. The DEFAULT behaviour should be same as always. You still haven't put forward a case as to why you implemented this functionality, and accordingly haven't explained why you haven't simply taken it out completely. -- Adam

Comment by External U.

I have to concur with Adam... the default should be the wildcard * lest applications relying on compiling with other extensions will break. As for reasoning... while a use case would certainly go a long way towards helping your developers understand why this was implemented... I'm amenable to the compromise of having the capability to specify this in Admin and in code so long as it doesn't break existing applications. I see the value in enforcing a coding practice, but really just save developers a step and make it default to the wildcard and let the developers pick and choose how they want to implement this feature. Either way will involve teaching developers correctly - one just forces them to learn it, at the risk of alienating the uninformed.

Comment by External U.

S V Pavankumar, Hello? {quote} You still haven't put forward a case as to why you implemented this functionality, and accordingly haven't explained why you haven't simply taken it out completely. {quote} -- Adam

Comment by External U.

Is there any consideration to back port this change to ColdFusion 9 and 10? I can see two reasons why it should. 1) If it is a fix to improve the security position of ColdFusion, as stated, it absolutely needs to be. 2) Allows for testing of the behavior on 9 & 10 without having to do a full migration to 11 to see what might break.

Comment by External U.

As I said earlier, this basically provides a way to you to decide what you want to compile and what not, when the file is being included. This was added after we saw few cases where an application included a file and somehow the bad guys were able to write to this file being included. The file in one of the case was actually a log file where the application was logging invalid user input apart from other errors/log messages. As you would figure, the bad input was actually some CFM code which went into the log file and when this got included, the user given code got executed. The application had not expected this file to be compiled. As we understand, there are many applications which use cfinclude to include static content. This is a good change. You are getting a control to decide what to compile. You will also get some performance improvement because your JS, HTM, CSS files would not be compiled, loaded and processed by the server. If you don't need any of this and want to compile everything, you can always use "wildcard". To answer whether this should be given as an update to CF 9 & 10, we would not want to break applications while applying updates in their production servers. We can add this where by default we would allow all the extensions to be compiled which then does not fix anything immediately. So how do we go about it? Do you think it needs to be made available in CF 9 & CF 10?

Comment by Rupesh K.

If there is a perceived security issue you are dealing with, then - yes - you need to roll it out for the currently supported versions of ColdFusion, yes. As to how to roll it out with low impact? Release it with the wildcard allowing all file extensions, and also release a security bulletin describing what you've done, and how people might (or might not) want to leverage it. The publicise the bulletin as much as you can (your blog, Twitter, get your account managers to contact your clients, etc... the stuff you ought to be doing WHENEVER you fix something relating to security). This is not perfect and a lot of people will not leverage it. But what you *have* done is fulfilled your due diligence in addressing the issue. Make sense? -- Adam

Comment by External U.

Pavan, while I think the change you made is better for backwards compatibility we then loose the ability to block cfinclude by file extensions which prevents directory traversal. Which I thought was cool. Here's what I think would be best (from a security perspective): 1) By default only compiles cfm, cfml files - I can't imagine there are a lot of cases where developers are cfincluding a non cfm file and expecting it to execute cfml. 2) Setting compileExtForInclude at app and server level which allows you to wildcard this or add extensions. 3) Setting allowedExtForInclude which defaults to * but does not compile only statically includes these files. This allows you to set this to a more restrictive value to prevent file the attacker from reading a log file or config file when there is a user controlled variable in the input. Perhaps if secureProfile is enabled it would then set this to a list like htm,html,css,js by default on the server. It would be great if the settings were added to CF9&CF10 as well, if backwards compatibility is the issue I don't think many people expect a non cfm file to be compiled via cfinclude. At a minimum if the allowedextforinclude setting only controls the compilation, then I think the name of the setting needs to change, since all files are allowed but only some types would be compiled.

Comment by External U.

If someone is writing code to include a file (rightly or wrongly) it is to have CF process it. So changing behaviour to make it *not* compile is wrong. I am far better with the notion of disallowing it from working and accordingly erroring than I am with making it not actually do what it's supposed to. You can't have a tag that *sometimes* includes, compiles, executes a file; and sometimes doesn't. That's really shit language design. I really don't think this functionality has been thought through and designed to the point it should be part of the product (and I'm not just saying that because I was initially against it). -- Adam

Comment by External U.

Adam - I agree that my suggestion could become somewhat confusing to end users. Maybe the original way was best, it just needed some admin / server settings. Given that we now understand the security issues this feature was designed to mitigate I don't think it should be excluded from the product. I also don't think it should allow wildcard compilation by default. But I also do think that many people use cfinclude to include static files because it is easier than using cffile or FileRead.

Comment by External U.