SA-CORE-2009-003 - Local file inclusion on Windows

I repeat: "Actually, I'd prefer to just ignore any component that has any of these characters in them instead of trying to fix it up by stripping them. In general, I think "reject invalid input" is better than "correct invalid input." Any reason not to do that here?"

In this case, perhaps there is some crazy set of input characters that triggers an attack, and for some reason you need to submit the URL in a way that contains a /, \, or %00, but you don't actually need those characters in the arg() values to execute the attack, so stripping the characters will not prevent the attack but ignoring the whole arg() value if it contains them will.

I realize this is a bit far fetched, but the principal is sound and widely accepted, and there is no real cost to doing it that way (except that it requires a slight change to the patch which I'm not doing myself at this particular moment).

There's nothing wrong or invalid in the URL http://localhost/core6/?q=my%5cfoo (suppose we're discussing PHP namespaces). Use of \ in a filename is undesirable. If possible we'd escape it, next best is replace/remove it.

Throwing away the entire arg limits functionality. Compare:

Throw away: page.tpl.php
Remove/replace: page-myfoo.tpl.php or page-my-foo.tpl.php

D6 patch looks good but I did not test it.

Committed to HEAD. Thanks!

This patch is unnecessarily checking $extension, which never comes from user input (Well unless some theme is dumb but if you follow that logic there are no safe strings anywhere, ever), and it seems like it's actually checking *twice* on suggestions and doing so is adding a preg which is the kind of thing you don't want to have extras of if you don't need them.

Sorry, it is a str_replace, so that's not quite as big a deal.

Checking just prior to the file_exists is definitely the best place; the earlier check seems superfluous, though I guess not technically wrong.

Committed to Drupal 6, thanks.

Committed to 5.x.

This fix breaks Advanced Forum. ( #449254: D5.17 removes ability to put templates in subdirectories and breaks styles ) I'm no security expert. Is there anything else that can be done here besides making it impossible to put templates in subdirectories?

Michelle

I just tested and this issue doesn't affect 6.x. Looks like the only problem is having subdirectories in your theme, not in modules. That doesn't help the 633 people using D5, though. :(

I put a request on the issue to do the security fix in a way that doesn't break AF but I'm not holding out much hope.

Michelle

Why does "/" need to be stripped out? Why not just "\\.." instead? Placing templates in sub-directories is a big deal. We need that back.

Oh, sorry. So it's 5 and down. I didn't actually test this. 6 or 7 but I certainly will. Moving back to 5 for now.

I agree with dvessel. Being able to organize templates into subdirectories is an important thing.

Uh.. What happen to my comment. (It did it again. Had to edit and save.)

I was saying, if this is left as is, there would be a lot of disappointment. On almost every site I've touched, I managed templates through sub-directories and not all of them are up to date. This isn't uncommon either.

And the stripping done outside of _phptemplate_default and drupal_discover_template seems redundant. The strings always lead to the same place.

Okay, looks like the patch is pointing nowhere. Trying again.

Thanks dvessel, you're a lifesaver! I don't understand this issue well enough to attempt a patch and was just hoping someone would be able to fix it. :) Now crossing fingers we can get it in. How would I go about helping test? I don't have a web setup on Windows and don't know how to try to hack it even if I did. Is there something else I can do?

Michelle

What Michelle was saying in #28 is that Advanced Forum is not affected on 6.x. Let's discuss this first in HEAD.

About the patch:

-    $arg = str_replace(array("/", "\\", "\0"), '', $arg);
+    $arg = str_replace(array("../", "..\\", "\0"), '', $arg);

Why allowing "\\" at all into a path? That's Windows specific and will or will not work on other platforms. I suggest we remove those altogether.

I don't understand. Why discuss this in HEAD when the change doesn't negatively impact HEAD?

@merlinofchaos: this affects D7 the same way it affects D6 and D5. What Michelle was saying is that Advanced Forum on D6 uses a different technique and so it is not affected. The original issue (not being able to organize templates in subfolders) remains.

Not at all. It uses a different technique because D5 didn't have the nice theming system we have in D6. In this situation, they are apples and oranges, because templates are handled completely differently from 5 to 6. In 5 you had to specify everything. In 6, you get it registered and the registration handles paths for you.

Looks fine to me. Does testbot work with 5.x? Changing status to see.

@Damien Tournoud Re:"Why allowing "\\" at all into a path?". I'd guess a back-slash is equally capable of allowing arbitrary code execution and all we are doing is filtering for both possibilities here in one shot. An easy way out.

I just tested on my site. Upgraded to 5.17, forums borked. Applied #41, forums unborked. Thanks!

Michelle

The patch in #34 doesn't do what it is intended to do; try nonexisting\\....\\\\filename.

I'm absolutely fine with having the code doing the suggestions escape accordingly; at least those bits have the info they need. A utility function would be required in that case.

To me, this seems to boil down to the difference between API and implementation detail; is the fact that you can add paths to suggestions an implementation detail, or something that is supported? If the latter, we may want to update PHPDoc and documentation accordingly.

I vote for documenting it. This is important functionality and shouldn't be dismissed for lack of documentation. If documenting something makes it "real" then let's do it. I'll write it if someone points me in the right direction. I've never touched the API docs before.

Michelle

Heine, that's why I'm not a security expert. Nailing a patch on the first try would be a first for me.

This is the exact same patch from pwolanin with updated docs. So no worries Michelle.

What more needs to be done here? I'd really like to get this into a dev at least so people don't have to be patching core to use Advanced Forum.

Thanks,

Michelle

This also breaks Taxonomy Filter in 6.x. When I roll back to the previous patch, all works fine.

solotandem, it should have no effect in 6. Is there a post that details the problem?

Why do you say it should have no effect in 6.x? If a module has template files in subdirectories of the module root, then the patch prevents those template files from being found and used. Rolling back makes them work again. The patch is aimed at Windows boxes but it messes up other boxes.

Taxonomy Filter in 6.x has template files in subdirectories, and it is affected by the patch.

There is not a separate post about this. This is the post.

Do you mean in subdirectories of the theme? They were never found in subdirectories of modules... If they were ever discovered automatically in subdirectories of modules, Advanced Forum would be in trouble. That's actually a fear of mine that someone will decide to add that feature to D7. LOL!

Michelle

It doesn't affect 6.x because in the theme registry, the path and the template file are different parameters.

Thanks for all the comments.

The module sets:

$variables['template_files'][] = 'path/template';

in a preprocess function to override the default theme defined in hook_theme. The patch removes the '/' and thus fails to find the template file.

Is this an invalid use of D6 theme API?

solotandem, a module assuming a specific sub-path is not a good idea since a theme overriding the template is not guaranteed to follow what's assumed from the module. So yes, it is an invalid use of the api. The module defining the theming hook through HOOK_theme is the only place where specific paths can be set. That way, a theme overriding the template can set it's own independent path that works for the theme.

It might have made sense for something that's custom and specific to an install but if Taxonomy Filter is doing this, please create an issue and it can be discussed there.

While I agree that a contrib module should not be doing this, what about if someone uses a theme preprocess on their site to move files into a subdirectory? Do we want to disallow that? Is there any harm in applying this fix to 6.x/7.x as well in that case?

Michelle

Michelle, I think it's less of an issue in 6/7 since the discovery of the override is done automatically no matter where it's located within the theme. As long as that base template is found, all the alternate templates will follow it.

If the theme needs finer control over where the suggested templates are located that's independent of the base template, it becomes difficult but it is possible by hacking the theme registry. I've done this in the latest theme I've been working on and it takes a lot of effort for what could be considered a minor gain.

dvessel: That's true. I guess most people wouldn't need this because of the auto discovery.

So can we get this fixed in D5, pretty please? :)

Michelle

Michelle, dvessel: I created #459380: Template discovery fails after core patch to discuss the issue on the module. Would you continue this discussion there to help me determine a fix?

dvessel: I added the template to a new hook_theme in the submodule, but, of course, it does not pick it up with the $variables code above.

Michelle, dvessel, pwolanin: I created #459380: Template discovery fails after core patch to discuss the issue on the module. Would you continue this discussion there to help me determine a fix?

dvessel: I added the template to a new hook_theme in the submodule, but, of course, it does not pick it up with the $variables code above.

Committed to 5.x.

Thank you!!!

Michelle