[wp-trac] [WordPress Trac] #43215: Allow wp_kses to pass allowed CSS properties

[WordPress Trac]

#43215: Allow wp_kses to pass allowed CSS properties
-----------------------------+-----------------------------
 Reporter:  mclaurent        |      Owner:
     Type:  feature request  |     Status:  new
 Priority:  normal           |  Milestone:  Awaiting Review
Component:  Security         |    Version:  4.9.2
 Severity:  normal           |   Keywords:
  Focuses:                   |
-----------------------------+-----------------------------
 `wp_kses` takes arguments for allowed HTML elements, HTML element
 attributes, and allowed protocols. It takes all of these directly via the
 function argument through a nicely formatted assoc array. On the side
 side, attribute values (i.e. "display: block; visibility: hidden;") are
 configured via a hook, which means they are changed globally for all
 future calls.

 Here a sample how the system is behaving currently:
 {{{#!php
 <?php
 $allowed_output_html = array(
         'script' => array(),
         'noscript' => array(),
         'iframe' => array(
                 'src' => array(),
                 'width' => array(),
                 'height' => array(),
                 'style' => array(),
         ),
 );

 $allowed_output_protocol = array(
         'https',
         'javascript',
 );

 $google_tag_manager_noscript <<<'ENDSTRING'
 <noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123"
         height="0" width="0"
 style="display:none;visibility:hidden"></iframe></noscript>
 ENDSTRING;

 echo wp_kses( $google_tag_manager_noscript, $allowed_output_html,
 $allowed_output_protocol);

 // Outputs as it should except for the
 style="display:none;visibility:hidden" portion
 // <noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123"
 height="0" width="0"></iframe></noscript>

 }}}

 It seems that currently the only way to allow the "style" attribute from
 showing is to write a filter for "safe_style_css" to add in the "display"
 and "visibility" attributes, however this means that we are globally
 altering this behavior, which in other scenarios would allow unexpected
 HTML to appear (ie unsafe element properties). Even when removing the
 items immediately after executing the code will not work because the
 attribute may in the future be consider secure. This then causes an
 inconsistent execution.
 For example, imagine that if we wanted to make sure the "float" attribute
 was allowed in the attribute value. Currently this value is allowed, so us
 adding it in won't change the array. However when we're then undoing our
 action (ie removing the "float" attribute from the list of allowed
 attributes, we are actually removing one that was set as "safe" by
 WordPress themselves. It is easy to imaging how this could escalate
 further, when plugins blanket whitelist element attributes, then remove
 them from the list, then meaning that no attributes are allowed.
 Alternatively, in this same example if we removed our filter (with
 remove_filter) right after our wp_kses call,  it would mean that our
 attribute whitelist would apply to all HTML attributes (display,
 visibility,...) and not only the one we would want to whitelist (style).

 {{{#!php
 <?php

 // Function to whitelist safe CSS attributes
 function my_safe_css_attributes( $array ){
    $array[] = 'display';
    $array[] = 'visibility';
    return $array;
 });

 // Function to remove our CSS attributes from the whitelist
 function my_safe_css_remove_attributes( $array ){
    unset $array['display'];
    unset $array['visibility'];
    return $array;
 }

 $allowed_output_html = array(
         'script' => array(),
         'noscript' => array(),
         'iframe' => array(
                 'src' => array(),
                 'width' => array(),
                 'height' => array(),
                 'style' => array(),
         ),
 );

 $allowed_output_protocol = array(
         'https',
         'javascript',
 );

 $google_tag_manager_noscript <<<'ENDSTRING'
 <noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123"
         height="0" width="0"
 style="display:none;visibility:hidden"></iframe></noscript>
 ENDSTRING;

 // Telling WordPress about safe CSS attributes...
 add_filter( 'safe_style_css', 'my_safe_css_attributes' );

 echo wp_kses( $google_tag_manager_noscript, $allowed_output_html,
 $allowed_output_protocol);

 add_filter( 'safe_style_css', 'my_safe_css_remove_attributes' );

 // Outputs as it should.
 // <noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123"
 height="0" width="0"
 style="display:none;visibility:hidden"></iframe></noscript>

 }}}


 I initially expected the allowed_output_html to allow overriding allowed
 attribute values from the "style" element in the array, which I thought
 would have been a really neat way of overwriting the execution on a one-
 time basis.

 {{{#!php
 <?php
 $allowed_output_html = array(
         'script' => array(),
         'noscript' => array(),
         'iframe' => array(
                 'src' => array(),
                 'width' => array(),
                 'height' => array(),
                 'style' => array(
                         'display', 'visibility'
                 ),
         ),
 );

 // ...
 }}}


 I think it would make sense to make the function accept this added level
 of depth as it allows very flexible configuration of what attributes are
 allowed without compromising attribute

--
Ticket URL: <https://core.trac.wordpress.org/ticket/43215>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform