Searching for ../../admin takes me to the admin page

I cannot reproduce this behaviour.

All I get in the url when searching for 'admin' or '../admin' are paths like ?q=search/node/admin

Care to further elaborate how you can traverse to admin pages through the search field?

ok, now I get it :-)

Not sure if this is an issue or even a problem though. After all, this just seems like a more devious way to enter something like /admin to your path manually. The permissions system isn't overridden.

OMG.

I can confirm this behavior: If you have clean URLs enabled, and you go to the search page (path=search) and type in the box:
../../admin
it takes you to path=admin. Presumably this is because the URL of the search results page would be
search/node/../../admin

We need to URL encode the search keywords better! It seems like it could open up all kinds of doors to exploits.

This seems like a critical issue to me.

try node/1/../../node/2 before you panic. If there is a real problem, reopen the ticket. If URLs behave like directory paths but there is no sechole then don't.

No but this is a real problem. Bug but not sec hole. [edit: for example, search for ../../user/157412 on drupal.org. It does not search for that URL, but takes you to a user profile page. This is most definitely not a security hole but is a real bug.]

I'm not concerned so much that *urls* behave that way. I'm concerned that if you type something into the search box like "../../admin" that it turns it directly into a bad URL instead of doing the search. That's a bug.

I completely agree, it's just not a security bug, and his been this way since at least Drupal 6, so noncritical. Working on a patch for this.

This is a nightmare, searching for things with a "/" in them are currently broken in Drupal core, as far as I can tell. This patch fixes that, and also forces the system to use the old ?keys= syntax when a path has a .. in it. Kind of ugly, plus it won't work with tabs, but searching for things with .. in them has to be an edge case anyway so I wouldn't worry too much about this.

Sorry, patch in #9 was bad, please ignore, this one should be good.

Searching for slashes has a long history in Drupal. At least #68886: Handle ampersands in search queries and other URLs when clean URLs are on and #284899: Drupal url problem with clean urls are relevant. I will look more later.

Ugh. The proposed patch in #10 undoes most of the nice cleanup that has recently been done on the search keys. Maybe we can just do something simpler, like urlencode the keys when forming the redirect?

See #93854: Allow autocompletion requests to include slashes

I just don't think that all these URL things are all that relevant to this issue.

The problem here is that the search module, when composing a URL to redirect to that has the search keys in it, ought to URL-encode the search keys so that they are not interpreted as URL parts, but as one big suffix. Right?

Just as a note: If you search for ../../admin in D6 with clean URLs, the same thing happens (you end up not searching).

Sounds like it needs work for the approach suggested in #14

I just tried a few things:

a) urlencode() before the redirects in search.pages.inc -- this resulted in the infamous Apache permissions problem - Apache doesn't like URLs with %2F in them (%2F = /), as it thinks they could be attacks, so if you have a URL like (whatever)/search/node/(something with %2F in it), the default Apache config gives you a 403 error. Not good.

b) Using drupal_encode_url -- this avoids the %2F problem by leaving the slashes as slashes, but the problem with that is you end up with the reported problem here (i.e. you end up at admin when you search for ../../admin).

c) I tried doing urlencode(urlencode()), but that also gave me an Apache 403 error.

So I think the only solution is going to be to do our own encoding/decoding when we add the search keywords to the URL. I took the result of (c) and it appears that once you get rid of the %2F problem, the . characters are still a problem. When I replaced those with x, I got to the search result with xx/xx/admin as the search string. So I think we need to do some encoding on both . and / in search keys, as well as probably generally encoding everything for good measure. It sounds like a pair of custom encode/decode functions will be necessary.

Here's a patch with a custom encode/decode.

I really wasn't sure what to do with the . characters, so I turned them into -dot- in URLs. If you did search for -dot- (which seems unlikely), it would turn back into . in your search box. Which is a bit odd, but... Better solution ideas welcome!

Anyway, this patch seems to work in my informal testing -- it solves the reported problem here, and seems to work fine for searching in general. But it might have some test failures if there are tests that were checking for particular un-encoded URLs after searching. Let's see.

This -dot- business is silly, let's just use an escape character that will work the same way as a backslash works in PHP.

Better yet, we could go back to the patch in #10, not sure what was wrong with it. I don't see how it "undoes most of the nice cleanup that has recently been done on the search keys" - it leaves the current behavior untouched for the vast majority of cases, only reverting to the ?keys= behavior when faced with a ... Which is cleaner, a URL like search/node/-dot--dot-%2F-dot--dot-%2Fadmin<code> or <code>search/node?keys=../../admin?

In any case, even if you don't agree with my other points, the patch in #18 needs work because it uses arrays in str_replace unnecessarily.

What escape character would you suggest that will work well for URLs?

Agreed that I don't need arrays for str_replace. I thougt I was going to need to do something with the slashes, but it turned out I didn't need to.

Regarding the patch in #10, I think we should either change to using ?keys= in all cases, or we should stick with the idea of the keywords being in the URL, and URL-encode them, just as Google and other search engines do on their search pages.

I think if they're put into the query like the patch in #10, then they should be url-encoded anyway actually, shouldn't they? I think the url() function will do that, so really the URL will not look clean after #10 either, will it?

If it comes to that, maybe a dash or a carat? I'm hoping we can find some sort of compromise between the patches in #10 and #18 so that nothing so ugly is necessary.

It seems that Apache is giving me a 403 on any URL that has a . in it. I don't think you can just escape it.

Sorry, cross posted.

For #21 - I'm actually beginning to think we should change to using ?keys= in all cases. There are too many edge cases in which inserting things into the URL is problematic, and urlencoding it isn't very nice. The advantages of the ?keys approach is that things like / and . and $ and ! do not need to be escaped. I'd also like to note that google seems to do its searches in the same way (e.g. see http://google.com/search?q=drupal).

And cross-posted again.

With different apache configurations possible, it's probably best to use the safer ?keys= [urlencodedstuff] approach?

Actually, if the . character is in a query, it's not complaining.

So...

I think we should either:

a) Abandon the idea of having the keywords be part of the base URL, and always make the URL be ?keys=whatever. This would simplify search_menu() and probably fix all kinds of other issues, and I think we wouldn't have to do any URL-encoding beyond what Drupal already does for query strings.

b) Figure out some way to encode a "." in keywords that is less silly than -dot-, and do that. And URL-encode all keyword strings (which can include all sorts of things, like they could be "foo bar ../../admin category:abc"), so that they make reasonable URLs.

OK, we're cross posting. :)

It looks like you are advocating for (a). I'm not necessarily opposed to it, but it's a rather big philosophical change. pwolanin: do you have an opinion?

+100 to using an actual query string and not a magical menu path that starts from an arbitrary segment.

Maybe +0 in Drupal 8, -ℵ₀ in Drupal 7. Compared to the weight of the change the benefit is negligible.

Yes we discussed in IRC that moving search string to the query string just wouldn't be something we can let fly so late in D7. See #894486: Use the query string for search keys rather than appending them to the URL filed for D8.

OK, so we're back to suggestion in #26 (b).... Is urlencoding the search keys acceptable for D7 (and possibly porting to D6)?

If so, we also need to figure out a way to encode a "." in a search key, because generating a URL with "." in it (outside of a query string) causes 403 errors in Apache, at least on my test box.

The patch in #18 chose one method. I'd love to have a concrete, working suggestion for a better method. And the patch in #18 also needs to be simplified so that it doesn't use arrays as inputs to str_replace, as noted in #19.

Could we just escape the . to %2E? Haven't tested it, not sure.

Also just a note, we have already supported query strings like ?keys=, but we just weren't generating them ourselves upon search, not sure that's too big a change compared to the benefits of being able to avoid this. URL paths just weren't designed to hold arbitrary user input, whereas query strings were. It makes a lot more sense to stick the input in the query string rather than in the URL. Note that this issue isn't applicable to sites with clean URLs disabled for that very reason.

I would rather not switch to ?keys if we have a reasonable alternative. Does urlencode not work?

RE #34: urlencode doesn't remove ., and I get 403 when doing searches after just urlencode.
RE: #33: You are right, we are already supporting keys= urls. I think the main idea is not to break older URLs that people may have linked to.

In any case, here's a patch that uses %2E for ., which in my casual testing, appears to work. Additionally, if you type in a URL directly, such as "search/node/search done before", it works fine. I don't think it will break any existing actual working search bookmarks either to urldecode the search keys. As a bonus, the custom decode function isn't needed.

Thoughts?

Thanks for the patch :-)

Yep, this solves the problem when we use relative paths on the search input.

After this, also if you try to go to example.com/search/node/../.. and paths like that still have the same behaviour(go to the relative path), but like chx mentioned that's the expected behaviour for paths, so this seems to be ready enough :-)

I thought that the web server is doing urldecoding also?

I'd agree that extra decoding generally won't hurt, unless you are trying to find something with "%" in it.

I agree that this could use another round of reviews. We don't do that anywhere else in Drupal, so this patch looks highly suspicious.

I have already given my review - I don't think there is no way to do this that will consistently work on all environments, plus it is not such a pretty solution to put encoded characters straight into the URL like that anyway. The patch above, for instance, works for me on my windows machine but gives me a server (apache) error when I try it on my linux setup. The only way to do this (the "proper" way) is to put the user input in the query string where it belongs. We can change this (go to search/node?keys=foobar and leave support for the "old"-style paths of search/node/foobar. We can also selectively redirect to search/node?keys= whenever the input does not work nicely directly in the URL. This is the approach that the patch in #10 had.

The switch to a query string is the "real" solution, however we decided that this wouldn't fly until Drupal 8, so we have the patch in #35 now, which is a weird pattern to have, but something like it is necessary due to the constraints of fixing this in the Drupal 7 release cycle. At the very least, however, this definitely needs some complete tests to make sure the problem is fixed for paths containing special characters such as a forward slash (/) or a period (.).

OK. This has been the same way since at least Drupal 6, and it doesn't look like we can fix it without some major API/behavior changes, and it's too late for that for Drupal 7.

So I'm bumping this to Drupal 8.

Marking this as a duplicate of
#894486: Use the query string for search keys rather than appending them to the URL
which I am updating to major/task.