Hi there, For my helpers module I am trying to be ACAP (as consistent as possible). And some things I have (not yet) decided on, but which IMO could be very usefull for the codyng guidelines and core: * I filter ONLY in the theme function. That way you can be assured that theme functions get the raw data. Having ONE place, and one way where/how we filter makes it easier to look for sec. issues. 'My rule' is: As soon as we make an HTML string from something, we filter it. Anything that gets HTML programatically is therefore filtered. * t(): on the same level. Only in the theme level do we output t()ed strings. This makes it a lot simpler, because you know that functions and methods pass the original strings along, and that they are only translated in the VERY END. This should also make testing against strings a lot easier. I even found a critical sec. issue that opened the "access control open to the world" because I translated two string similar. * whenever a (theme) function can receive HTML as optional string, that string is NOT translated. eg theme_emtpy_placeholder($message = NULL) will return t('not available'), if no param is passed. But the doxygen states: @param $message: Optional. Provide a translated message string. * Theme functions can call other theme functions. They should do so, at all times if there is a function that formats what they want to output (ie call theme_item_list() instead of internally building up LI strings!). These are merely ideas, and I think we can and should make something simpler or maybe even this as guideline. We can then make sure that all changes to core at least follow these guidelines.
Bèr Kessels wrote:
Hi there,
For my helpers module I am trying to be ACAP (as consistent as possible). And some things I have (not yet) decided on, but which IMO could be very usefull for the codyng guidelines and core:
* I filter ONLY in the theme function. That way you can be assured that theme functions get the raw data. Having ONE place, and one way where/how we filter makes it easier to look for sec. issues. 'My rule' is: As soon as we make an HTML string from something, we filter it. Anything that gets HTML programatically is therefore filtered.
I think this is a pretty bad idea. This way every themer has a chance to remove our XSS checks.
* t(): on the same level. Only in the theme level do we output t()ed strings. This makes it a lot simpler, because you know that functions and methods pass the original strings along, and that they are only translated in the VERY END. This should also make testing against strings a lot easier. I even found a critical sec. issue that opened the "access control open to the world" because I translated two string similar.
I am not too thrilled about that either. Themers might decide to change strings and then we would need theme dependend translations. Cheers, Gerhard
Op woensdag 10 mei 2006 12:30, schreef Gerhard Killesreiter:
I think this is a pretty bad idea. This way every themer has a chance to remove our XSS checks.
Sounds fair. However, we now do *not* have a central place. Quite some of our checks/filters DO appear in theme functions! So I agree that the theme layer might not be the best place, but fact remains that we need a *central* place, not? IMO having it "all over the place" is worse then having it in a theme layer where we say "themes are the ones to filter/sanitize all output". Too often do I now find that theme_a calls theme_b and that theme_b filters out stuff that theme_a wants to put in. MOre often even do I find that function_x filters code, and makes it a chunk of HTML, while theme_a wants to do something usefull to that chunk and therefore wishes to receive the original data. I tried to address these three issue (security, theme-power and overhead) in one "guideline".
* t(): on the same level. Only in the theme level do we output t()ed strings. This makes it a lot simpler, because you know that functions and methods pass the original strings along, and that they are only translated in the VERY END. This should also make testing against strings a lot easier. I even found a critical sec. issue that opened the "access control open to the world" because I translated two string similar.
I am not too thrilled about that either. Themers might decide to change strings and then we would need theme dependend translations.
Why would that be? You will not receive any new strings. Its only the place where strings are translated that will change. IMO the "place where the string is injected into the HTML" is by far the best place to translate that string. Bèr
Bèr Kessels wrote:
Op woensdag 10 mei 2006 12:30, schreef Gerhard Killesreiter:
I think this is a pretty bad idea. This way every themer has a chance to remove our XSS checks.
Sounds fair.
However, we now do *not* have a central place. Quite some of our checks/filters DO appear in theme functions!
Guess we should fix this, then.
So I agree that the theme layer might not be the best place, but fact remains that we need a *central* place, not?
It is hard to do that. Actually, there is a way to get rid of XSS once and for all: Filter your complete output through filter_admin_xss in your theme. :p
IMO having it "all over the place" is worse then having it in a theme layer where we say "themes are the ones to filter/sanitize all output".
I am very uncomfortable with this. Also, this will add complexity to the themes while many people try to remove complexity.
Too often do I now find that theme_a calls theme_b and that theme_b filters out stuff that theme_a wants to put in. MOre often even do I find that function_x filters code, and makes it a chunk of HTML, while theme_a wants to do something usefull to that chunk and therefore wishes to receive the original data. I tried to address these three issue (security, theme-power and overhead) in one "guideline".
Well, it is certainly high on the power site, but a bit low on security. Not sure about the overhead.
* t(): on the same level. Only in the theme level do we output t()ed strings. This makes it a lot simpler, because you know that functions and methods pass the original strings along, and that they are only translated in the VERY END. This should also make testing against strings a lot easier. I even found a critical sec. issue that opened the "access control open to the world" because I translated two string similar.
I am not too thrilled about that either. Themers might decide to change strings and then we would need theme dependend translations.
Why would that be? You will not receive any new strings. Its only the place where strings are translated that will change. IMO the "place where the string is injected into the HTML" is by far the best place to translate that string.
I can replace any themed function by anything else so I sure can just change the strings. Cheers, Gerhard
Op woensdag 10 mei 2006 13:30, schreef Gerhard Killesreiter:
So I agree that the theme layer might not be the best place, but fact remains that we need a *central* place, not?
It is hard to do that. Actually, there is a way to get rid of XSS once and for all: Filter your complete output through filter_admin_xss in your theme. :p
Yes. If that function is smart enough filtering for ONLY XSS should definately be done in the very end. It now also tries to fiddle with HTML so it cannot be used like this, yet. However, lets forget about XSS, but focus on HTML itself. It is not at all hard to filter this in one place. In fact, Ruby in Rails (yes, there we go again) has a very clean, clear, and thus *secure* concept. input ->| RoR app |->| view |-> output RoR filters input on its model/controller level. This has very little to do with XSS/HTML filtering, but all with cleaning out stuff to avoid SQL injection etc. Result: You need not bother about all that, becuase on the very front-end there was taken care of it (obviously this can potentially be overriden if you really want to). then RoR "does stuff" and pipes the data to the views level. That is the place where one filters the output. Not *inside* the model/controllers, but in the view, the output. In drupal you can see the theme as the view layer. So yes, it puts responsibility on the ones printing stuff to the screen, but the fact that it is 100% clear who is responsible and where the stuff takes place, makes it very secure. We might need some better helper methods though. because "filtering" in RoR is nothing more then <%=h @foo %> (in PHP <?php print h($foo) ?> ) But wheter or not its the views layer where output sanitizing takes place, we still need a central layer to do that sanitizing. It could even be automated and placed in the theme engines. Or we could agree that it happens /just/ before calling theme functions. Having sometimes the theme layer sanitizing, sometimes the controller level is asking for problems. "no, your theme forgets to call the filter" "hmm, I thought the module did the filtering". Or "woa, that is a big security hole, strange, I though the theme function did the check_clean()", "nope, it was there, but we moved that into the modules".
IMO having it "all over the place" is worse then having it in a theme layer where we say "themes are the ones to filter/sanitize all output".
I am very uncomfortable with this. Also, this will add complexity to the themes while many people try to remove complexity.
I can replace any themed function by anything else so I sure can just change the strings.
What I propose is: theme('pimp', 'bling bling'); theme_pimp($string) { return '<span class="bling">'. t($string) .'</span>'; } vs: $bling = t('bling bling') theme('pimp', $bling); theme_pimp($string) { return '<span class="bling">'. $string .'</span>'; } We mix up both right now. Though the fist one is the least used, it does happen a lot. I am proposing to choose ONE and work towards that. Bèr
I think this is a pretty bad idea. This way every themer has a chance to remove our XSS checks.
Sounds fair.
However, we now do *not* have a central place. Quite some of our checks/filters DO appear in theme functions!
Having a central place sounds like a particularly good idea, IMO. I usually don't use contributed module because they are prone to security issues. If all the escaping was (forced to be) done in a central place, it would be ten times easier to audit the code (before installing it). Whether this is feasible in the theme layer, I don't know. I do know, however, that I like the idea. -- Dries Buytaert :: http://buytaert.net/
On 11 May 2006, at 2:28 PM, Dries Buytaert wrote:
Having a central place sounds like a particularly good idea, IMO. I usually don't use contributed module because they are prone to security issues. If all the escaping was (forced to be) done in a central place, it would be ten times easier to audit the code (before installing it). Whether this is feasible in the theme layer, I don't know. I do know, however, that I like the idea.
One of the things about fapi 2.0 , and cck, would be that everything would have a model. IE: every form would have it's fields defined. Same goes for node types, etc. Doing the checking automatically on the model object sounds like the best central place. That would mean that all contrib modules would get save html at all times. -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com
I think we should aks the people working on the new output system what they thnk, or plan. Op donderdag 11 mei 2006 14:28, schreef Dries Buytaert:
I think this is a pretty bad idea. This way every themer has a chance to remove our XSS checks.
Sounds fair.
However, we now do *not* have a central place. Quite some of our checks/filters DO appear in theme functions!
Having a central place sounds like a particularly good idea, IMO. I usually don't use contributed module because they are prone to security issues. If all the escaping was (forced to be) done in a central place, it would be ten times easier to audit the code (before installing it). Whether this is feasible in the theme layer, I don't know. I do know, however, that I like the idea.
Bottomline: We could do better, security-wise, if we have either agreements on where sanitzing should happen, or if we have such a layer built into Drupal itself. I recall some people working on a new concept for outputting "stuff". Building on top of Fapi. Using concepts from fapi. Is this part of your plans? Or should we look for a solution that is not in that layer? Bèr
On 11 May 2006, at 7:07 PM, Bèr Kessels wrote:
Is this part of your plans? Or should we look for a solution that is not in that layer?
This is definitely part of those plans. But this solution _will_not_be_ready_for_4.8_ I am going to be doing the pre-requisite changes to the menu system to make fapi 2.0 possible, and then I want to have fapi 2.0 stay in contrib for a while. Those changes will just make it into 4.8. I am aiming to get FAPI 2.0 up and running in contrib, with some (non trivial) contrib modules already ported. At that point, it's going to be much simpler to break up and get into core. Trying to hit a moving target for this API was one of the reasons that the FAPI port took so long, and was so difficult. Meanwhile, perhaps we can look at implementing this in theme() since we will be working on that anyway? A lot of the variable passing mechanisms from phptemplate will be moving to core, so we could perhaps implement it as a _variables() ? -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com
Op donderdag 11 mei 2006 19:33, schreef Adrian Rossouw:
Meanwhile, perhaps we can look at implementing this in theme() since we will be working on that anyway?
sounds like a plan. However, my initial plan / proposal was nothing technical. We are developers, and therefore tend to look at technical solutions for all our problems :) What I tried to propose is just a guideline. Something that says: "if you print raw data in your HTML in a theme function, you should always sanitize it in that theme function". or even "All your raw data needs to be cleaned out before sending it to the theme level". This is not an immediate solution, but at least it allows us to work towards one. @Khalid: I was not referring to the filtering system itself. The part that filters nodes and comments works pretty well IMO. I was referring to things like where a module collects Foo (outside the node/comment system) as input and e-g prints them in a list. If that module developer forgets to call the proper filters for the Foos we have a security hole. Views module, e.g. does this very well, but imagine a module like that with all the input not sanitizing. Yes, those places are only accessible by admins. But no: having something in the admin area is not a reason for not sanitizing HTML. Now if theme_item_list filters out the abovementioned lists already, then the module developer can lean back and know security is taken care of, while we can lean back and know that the amount of critical holes in contribs has grown smaller (has anyone ever done an audit on contribs?)
On 12 May 2006, at 9:37 AM, Bèr Kessels wrote:
I was not referring to the filtering system itself. The part that filters nodes and comments works pretty well IMO. I was referring to things like where a module collects Foo (outside the node/comment system) as input and e-g prints them in a list. If that module developer forgets to call the proper filters for the Foos we have a security hole. Views module, e.g. does this very well, but imagine a module like that with all the input not sanitizing. Yes, those places are only accessible by admins. But no: having something in the admin area is not a reason for not sanitizing HTML.
Yup, then i believe the 'model' part of fapi 2.0 is the best place to do it. We can't even start working on that until the menu / callback system is refactored/fixed. I'll write up a spec about what I'd like to see happen to the menu system. -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com
On 5/11/06, Bèr Kessels <ber@webschuur.com> wrote:
I think we should aks the people working on the new output system what they thnk, or plan.
Op donderdag 11 mei 2006 14:28, schreef Dries Buytaert:
I think this is a pretty bad idea. This way every themer has a chance to remove our XSS checks.
Sounds fair.
However, we now do *not* have a central place. Quite some of our checks/filters DO appear in theme functions!
Having a central place sounds like a particularly good idea, IMO. I usually don't use contributed module because they are prone to security issues. If all the escaping was (forced to be) done in a central place, it would be ten times easier to audit the code (before installing it). Whether this is feasible in the theme layer, I don't know. I do know, however, that I like the idea.
Bottomline: We could do better, security-wise, if we have either agreements on where sanitzing should happen, or if we have such a layer built into Drupal itself.
I recall some people working on a new concept for outputting "stuff". Building on top of Fapi. Using concepts from fapi.
Is this part of your plans? Or should we look for a solution that is not in that layer?
Is this related to the classic debate of whether filtering should happen by default for everything or not? (Karoly and Steven: any comments?)
participants (5)
-
Adrian Rossouw -
Bèr Kessels -
Dries Buytaert -
Gerhard Killesreiter -
Khalid B