url_is_external() is either broken or misnamed. It does not check that a URL is external to the calling website. It checks that the URL is absolute. Huge difference.
What is the desired behavior:
1) Check that a URL is absolute (and therefore not subject to Drupal access control and potentially malicious)?
2) Check that a URL goes off the current site?
If #2, we aren't doing that by verifying the URL against $base_url.
If #1, the function should be renamed url_is_absolute().
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 819844-external-doc.patch | 926 bytes | agentrickard |
| #11 | 819844-external.patch | 5.9 KB | agentrickard |
| #1 | 819844-external.patch | 599 bytes | agentrickard |
Comments
Comment #1
agentrickardRescinded. It means 'external' to Drupal. Patch for clarification to the docblock.
Comment #2
agentrickardComment #3
grendzy commentedI'm still confused, what does "external to drupal" mean? Would a non-drupal path like "phpbb2/viewforum.php" be considered "external to drupal"? What about an absolute URL with a domain that matches HTTP_HOST?
Comment #4
grendzy commentedhuh, this function seems bogus... if the scheme isn't in the list allowed by filter_xss_bad_protocol, it returns FALSE which is clearly wrong for many schemes, e.g.:
file:///etc/services
gopher://example.com/
xmpp:user@example.com/
Comment #5
cha0s commentedYes, it appears we may do better in filter_xss_bad_protocol() with a blacklist than a whitelist.... thoughts?
Comment #6
agentrickard@grendzy
You shouldn't be able to process a path like 'phpbb2/viewforum.php' through any of Drupal's URL handling functions. It would have to be absolute.
What about an absolute URL with a domain that matches HTTP_HOST?That was my question at first, too.
I think the intent of this function is actually
url_is_absolute().This might need to be bumped to critical, as it's clearly a big WTF.
Comment #7
grendzy commentedI'm struggling to understand why url_is_external uses filter_xss_bad_protocol at all; it doesn't seem relevant.
That said we need a whitelist for filter_xss_bad_protocol since new dangerous schemes can be introduced at any time.
BTW this was named
menu_path_is_externalin D6.Comment #8
grendzy commentedfound the issue that introduced this change: #607008: Fix bugs in https support and force using https for authorize.php if available
If the indented use was only within the context of the menu router system, perhaps the old name was better after all. The new name seems to imply the function can be used to test any URL, which it clearly cannot.
Comment #9
dwwIf by "old name" you mean "menu_path_is_external()", then no, the old name was worse. ;) The function didn't have anything to do with the menu system per se, but it was named as if it did, and lived in menu.inc, which sucked for authorize.php wanting to use it, since it would have meant loading nearly 3400 lines of code we don't want to have to rely on inside authorize.php just to reuse this small helper function.
I'll admit to not fully understanding what we're doing with filter_xss_bad_protocol() in here. To answer that, you'd have to read through #131009: Menu bugfixes: 40x pages, external items, parenting and/or get chx to comment here, since that's the issue where the original menu_path_is_external() function was added.
I *though* this was a better function name, and that it was fairly well documented. ;) Sorry, but I don't have the cycles right now to fully parse your objections and resulting confusion. I'm happy to try to help improve things, but I've got a ton of other stuff on my plate right now.
Thanks/sorry,
-Derek
Comment #10
agentrickardI read through the two issues that @dww posted, and I see the purpose behind the change.
I think this just needs clarifying documentation, (to make it clear that the function is designed to assert that a url is not govered by Drupal's menu access control system). It also means a refactor of #733028: SA-CORE-2010-001 - Open redirection to _not_ use this function or to change its inline documentation to note that the security check is only for whether Drupal's menu system can assert access control.
Comment #11
agentrickardPatch that improves the documentation (which is missing @param currently).
Comment #12
agentrickardComment #13
dwwCool, thanks for looking into all this! However, #11 includes hunks for modules/system/system.install and modules/system/system.test that are unrelated to this issue.
Comment #14
agentrickardOops! Actually, it is related (it's how we found this problem), it's just not wanted here.
Comment #15
agentrickardFixed.
Comment #16
dwwLooks good to me, thanks!
Comment #17
grendzy commented+1
looks good. Thanks for unraveling this mystery.
Comment #18
dries commentedCommitted to CVS HEAD. Thanks.