[beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain()

looks great.

If you insist :)

Just a trivial test addition: straight to RTBC.

Hm. Looking at the list of functions that invoke http://api.drupal.org/api/function/check_url/7, I see a couple of other candidates, for example:

http://api.drupal.org/api/function/template_preprocess_html/7 does 'href' => check_url($favicon),

but the subsequently invoked http://api.drupal.org/api/function/drupal_add_html_head_link/7 does check_plain($attributes['href']) once again.

I'm starting to question why http://api.drupal.org/api/function/filter_xss_bad_protocol/7 invokes check_plain() at all.

.

I'm not sure either. At the very least, I'd say that the check_plain() actually belongs into check_url(), and not into filter_xss_bad_protocol(). But that doesn't solve the unpredictable chaining problem.

The problem is not really new, but gets more apparent due to D7's multi-layer rendering system. Previously, we simply said: "Everything that gets output in a theme function has to be sanitized." Since there was only one theme function, no problem. In D7, we have an entire stack of #pre_render, #theme, #theme_wrappers functions, from which every single may - or may not - be the last instance in the rendering process. Yuck.

Therefore, bumping to critical + adding security tags.

@Pasqualle: so what's the right way to add a 'src' or 'href' to a $attributes array, as in #555830-19: Clean up theme_image() to use drupal_attributes() for all attributes and revisit defaults for "alt" and "title"? drupal_attributes() calls check_plain() on all attribute values.

Thanks for your input, Damien! We definitely need some more security team members on this issue.

While I agree that it's wonky that check_url() check_plain()s, I'm not sure whether it's a good idea to remove the check_plain() from it. Looking at the list of functions that call http://api.drupal.org/api/function/check_url/7, at least a couple of them want to use the returned value directly in XML/HTML output, so those would actually have to call:

And we have the issue that all check_* functions are - as of now - supposed to return values that are safe for outputting.

To resolve the issue(s) at hand, how about moving the check_plain() into check_url(), and updating many of the current calls to check_url() to invoke filter_xss_bad_protocol() instead? (This partially means to add a check_plain() further down in the rendering chain, as in one of the given examples above.)

You can ignore this follow-up. Just mentioning, similar problem, weird solution: http://api.drupal.org/api/function/drupal_set_title/7

I'm very worried about themers, and would highly prefer to not remove check_url(), but move the check_plain() into it. The other changes of this patch look good.

However, didn't look up whether this patch actually solves all issues identified in this thread.

CNR for bot, but still "needs work".

Like this?

Please review both the patch, and apply it and grep for remaining places where calls to check_url() were left in tact.

Perhaps we don't need to change the default of 2nd param from TRUE to FALSE? What do you think? Senseless kitten killing?

114 critical left. Go review some!

I dislike a function like this needing to call filter_xss_bad_protocol() directly. This patch fixes that, enabling theme functions that add to a $attributes array to still use check_url().

This patch also adds a unit test.

As per #27, here's a patch that offers l() function usage in appropriate places.

Please review.

1) Missing spaces for 'html' key.

2) The 'class' attribute is always an array.

Missing t() for 'More'. Not sure whether this change is good for translatability (probably not).

Powered by Dreditor.

Looks good to me.

Do we have this documented somewhere?

Adding a docblock to subsequent patch. Should this go in a separate issue?

Rerolling.

Looks ready to fly for me.

This patch is a prerequisite for #690980: Disabled form elements not properly rendered

I don't like the fact that we added the $check_plain parameter. It is better not to clobber those IMO and to call things as follows:

check_plain(check_url($foo))

That is actually more transparent. Thoughts on that? I might be the only one that thinks that.

eff nailed it.

#38: drupal.check-url.38.patch queued for re-testing.

.

needs a security review (others have said it's ok to rtbc after security review).

I stick with #17, we are mixing things that are way too different to be mixed:

• Filtering "bad protocols": this is done by filter_xss_bad_protocol(). It's a security feature: we don't allow potentially harmful protocols such as javascript: to get through
• Escaping the URL when it is output in an HTML (argument) context: this is done by check_plain() or drupal_attributes() depending on the way the argument is output. This is *not* a security feature, but just something mandatory to output any text in an HTML context.

Currently, check_url() is basically a wrapper around check_plain(filter_xss_bad_protocol($url)). This is bad, because nobody (including the colloquial "themer") understand what this means.

Most of the time, when you are properly using t() or drupal_attributes(), you don't need to take care about the check_plain() yourself:

should be written as:

patch in #17 was suggested in #53, seeing what the testing bot thinks of it.

also, sounds like there is no consensus on what approach to take... does that make this issue needs work? or more reviews?

Fair analysis. I'm happy with solution #4 above, if there are buy-ins from others.

This is a re-roll of #38, adjusted for #56.4.

I would add a new placeholder to t() handling this, too. * perhaps?

I didn't do this in this patch, because it seems like it would introduce too many permutations needing special symbols (i.e., a symbol for drupal_strip_dangerous_protocols(), a separate symbol for check_url(), and maybe others). Since protocol stripping only needs to happen for user-entered text, I'm not sure there's enough use-cases to warrant adding more complexity within t().

The reason I'm leaving [needs security review] in this issue title is that in addition to the decision reached in #56.4, this patch continues what #38 did in additionally changing some places that used to call check_url() to just calling check_plain() (as discussed in earlier comments). The idea being that protocol stripping is something to be done for user-entered text only, not generically whenever we have a URL. This makes sense to me, but I'd feel better knowing someone from the security team had a chance to approve this (@chx, @Damien: I don't know if you're officially on the security team, but if you feel comfortable RTBC'ing, please do so).

Coltrane doesn't see any security problems. How many security reviews do we need?

For anyone joining this issue late, see last paragraph of #59 for the security review that's needed.

Thanks coltrane! Therefore, removing the [needs security review] issue title prefix.

But, this issue is a pre-req for #690980: Disabled form elements not properly rendered, which itself is a beta blocker, which makes this issue a "beta blocker blocker", so let's get it in. Thanks!

Some of this patch is a necessary fix to avoid double HTML escaping. That's why we need a new function. Aside from adding a new function, I can't see another API change. Correct me if I am wrong.

However, quite some hunks (like the more link itself which sparked this issue which can easily be fixed by changing a @ into a !) look like a performance optimization to me and makes this patch into the proverbial kitten killer monster. As far as I can see, changing check_url(url()) to check_plain(url()) saves an expensive protocol-stripping call but there is no change in the output whatsoever. These do not belong in this patch and make it very hard to read and evaluate.

Also:

Why exactly we do not need a protocol stripping off actions??

I'm... honestly scratching my head how we got from the initial bug report (a module accidentally double-check-plained a URL in a string which could be solved in 8 seconds by switching @ to !) to a sweeping API change with security implications, several months and several alphas into our release cycle. I'm even more scratching my head why the behaviour of check_url(), filter_xss_protocol(), etc. that's been part of Drupal since time immemorial is suddenly a critical, beta blocking issue.

Yes, the initial bug report was just about theme_more_link() where a change from @ to ! was all that was needed. Then we started fixing problems in theme_image() in #555830: Clean up theme_image() to use drupal_attributes() for all attributes and revisit defaults for "alt" and "title", and discovered that it was impossible to set $attributes['src'] = check_url(...), because drupal_attributes() does a check_plain(), and unlike t(), there's no special symbol for turning that off: i.e., there's nothing like $attributes['!src']. #10 summarizes why this is critical in D7, even though it's a legacy problem from D6. So most of this thread debated how to decouple protocol stripping from check_plain(): this is summarized in #56, and #56.4 was chosen.

Could someone maybe write the documentation that will need to go out to the devel list to explain what the hell happened and how they need to change all their modules and themes? That might help me understand.
...
It would be great to hear what of those outputs is undesirable, and what effect this patch has specifically.

This patch deals with 2 questions:

1) When should protocol stripping be done?
2) What is the API of check_url() and filter_xss_bad_protocol() and the new function drupal_strip_dangerous_protocols()?

#1 will be answered later, in context of chx's review.

As for #2, there is no API change in this patch with respect to check_url() and filter_xss_bad_protocol(). check_url() takes a plain-text string, strips protocols, and returns an HTML-encouded string. That's true in HEAD and is unaffected by this patch. filter_xss_bad_protocol() without the 2nd argument passed takes an HTML-encoded string, strips protocols, and returns an HTML-encouded string. That's true in HEAD and is unaffected by this patch. filter_xss_bad_protocol() with a 2nd argument of FALSE takes a plain-text string, strips protocols, and returns an HTML-encouded string. That's true in HEAD and is unaffected by this patch in order to retain BC, though comments are added in the patch to makes it clear that such a parameter in filter_xss_bad_protocol() is useless and should be removed in D8. This patch adds a new function, drupal_strip_dangerous_protocols(), for taking a plain-text string, stripping protocols, and returning a plain-text string, addressing the drupal_attributes() use-case.

However, quite some hunks look like a performance optimization to me and makes this patch into the proverbial kitten killer monster. As far as I can see, changing check_url(url()) to check_plain(url()) saves an expensive protocol-stripping call but there is no change in the output whatsoever. These do not belong in this patch and make it very hard to read and evaluate.
...
Why exactly we do not need a protocol stripping off actions??

Ok, back to #1 from above: it's not just a performance optimization: it's a question of when should protocols be stripped and when they should not be. The output is the same when the input string doesn't contain dangerous protocols, but the output is different when the input string does contain dangerous protocols. Why does l() in HEAD call check_plain(url()) and not check_url(url())? And why does theme_image() in D6 call check_url() and not check_plain()? (Note: theme_image() for D7 was changed in the previously mentioned issue). Which functions should be responsible for stripping protocols and which should not be? sun answers this question in #21: the only functions that should strip protocols are the ones that are dealing with strings that come from user input. l() doesn't strip protocols because it's a generic function: maybe a module wants to call it with $path = 'javascript:...' or $path = 'somecustomprotocol:...', and l() should not force that to be invalid. The same answer applies to theme_image(), which is why it's been changed already in that other issue. And the same answer applies to theme_form() with respect to #action. It's a mistake that HEAD calls check_url() in so many places, because protocols should only be stripped in the functions that deal with strings that come from user input.

Maybe this change to limiting where protocol stripping is done should have been a separate issue. But it seemed to be relevant to this one, since this is where we realized the need to decouple protocol stripping from html encoding. Anyway, it is this aspect that made it take so long to reach RTBC status, because someone with real, in-the-wild, Drupal XSS attack experience needed to review all these hunks that change check_url() to check_plain() and similar removals of protocol stripping. But look at the comments from #59-#64, and I think as long as we trust that coltrane performed an adequate security review of these hunks, then I feel good about them.

Re #68:
The patch does add this info to the PHPDoc of l(). The comment is also added in the test, because the test in HEAD is wrong (i.e., inconsistent with l()) and is being fixed.

CNR for chx to review this comment and decide whether to RTBC or give additional feedback.

We can go that way but massive amounts of doxygen is needed. It needs to be crystal clear which function are such 'generics' that can take anything and it's the caller who needs to the protocol stripping via the new function. Also as l runs through url() it will not recognize custom protocols (say, skype: or magnet:) as an external URL due to the external URL checking happens via filter_xss_bad_protocol. So the documentation for l should mention that if it is indeed your wish to allow for skype: then use the filter_allowed_protocols $conf override.

Edit: also, although it looks like we took a long time to reach RTBC status, the patch / issue was essentially restarted in #56.

I will open a new issue for #69.1, since in #71, chx says that will require more discussion and documentation.

#845000: Inconsistency and lack of clarity for which functions are responsible for stripping dangerous URL protocols

Nope. The new doxygen is incorrect. See #66 url() and #845000-3: Inconsistency and lack of clarity for which functions are responsible for stripping dangerous URL protocols for a detailed analysis.

Very interesting analysis in #75. Understanding and properly documenting url() and l() in that issue will be very important. In the meantime, simplifying this patch even further based on that feedback:

This patch removes this PHPDoc addition to url(). To be taken up in the other issue.

This patch removes this PHPDoc change to l(). To be taken up in the other issue.

This patch removes drupal_strip_dangerous_protocols() here. We're calling l(). l() itself is one of:
1) safe, as per chx's analysis in the other issue
2) not safe, and this is a problem to be solved within l() in the other issue
3) intentionally not safe to place the onus of protocol stripping on the caller, but this theme function is just an intermediary, so if that's the case, the onus should be on this function's caller, not on this function.
Regardless which of these 3 is the case, we should not be calling drupal_strip_dangerous_protocols() here, and we'll figure out and document the situation with l() in the other issue.

same as above

same as above

This patch removes this hunk. To be addressed in the other issue, once we can properly document l().

Powered by Dreditor.

Thanks, this patch is much easier to follow now.

A couple of small things:

1. As far as I can tell, if we remove the $decode param of filter_xss_bad_protocol(), then this function is exactly the same as check_url(), so we should probably deprecate the entire function, and remove it in D8?

2. Let's change www.google.com to www.example.com in the test.

3. We still need explicit instructions for module/theme developers on how their code needs to change. I can almost piece it together from the patch, but would prefer a sentence or two from someone who worked on this patch.

.

I can't imagine why contrib wouldn't want/need to make similar changes as well to keep step.

Good point. I guess I would say that if a contrib module/theme has broken code that results in double check_plain(), similar to the broken HEAD code that started this issue, then the contrib module/theme now has a way to fix it. So if contrib code is passing the result of check_url() to something that will end up calling check_plain() (for example, an @ placeholder in t(), or an attribute value passed to drupal_attributes()), it should replace the call to check_url() with a call to drupal_strip_dangerous_protocols() instead.

FYI: #80 created a regression: #858874: The more link in the node module's "Recent content" block always links to http://admin/content, which I marked as a duplicate of #845000: Inconsistency and lack of clarity for which functions are responsible for stripping dangerous URL protocols, since the latter issue will have to encompass the former.

Just today, I learned that we could have solved this perhaps less elegantly, but once for all:

#882438: Globally prevent double encoding in check_plain() by raising minimum PHP to 5.2.3