Last modified: 2014-07-18 17:05:13 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T13106, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 11106 - Enable customizing of CSS values filter
Enable customizing of CSS values filter
Status: NEW
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
unspecified
All All
: Low enhancement with 2 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: css
  Show dependency treegraph
 
Reported: 2007-08-29 01:41 UTC by Danny B.
Modified: 2014-07-18 17:05 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Danny B. 2007-08-29 01:41:44 UTC
Sanitizer currently wipes out entire url() CSS values.

This is unnecessarily too strict.

It should at least allow same domain (either via full URL or relative path) URLs. 

On WMF sites it should allow something like "^[a-z0-9]+\.(wik(i((m|p)edia|books|news|quote|source|versity)|tionary)\.org/.*"

Furthermore it would be even better to have configurable list (eg. [[MediaWiki:Allowedurls]]) of allowed URLs perhaps using regexps like eg. blacklists do.
Comment 1 Danny B. 2008-01-02 19:28:00 UTC
The current function works this way:

static function checkCss( $value ) {
  $stripped = Sanitizer::decodeCharReferences( $value );

  // Remove any comments; IE gets token splitting wrong
  $stripped = StringUtils::delimiterReplace( '/*', '*/', ' ', $stripped );

  $value = $stripped;

  // ... and continue checks
  $stripped = preg_replace( '!\\\\([0-9A-Fa-f]{1,6})[ \\n\\r\\t\\f]?!e',
    'codepointToUtf8(hexdec("$1"))', $stripped );
  $stripped = str_replace( '\\', '', $stripped );
  if( preg_match( '/(?:expression|tps*:\/\/|url\\s*\().*/is',
      $stripped ) ) {
    # haxx0r
    return false;
  }

  return $value;
}

I'd suggest perhaps to add optional variable such as $wgStrippedCss or so to LocalSettings and make the change from

if( preg_match( '/(?:expression|tps*:\/\/|url\\s*\().*/is', $stripped ) )
...

to something like (in kind of pseudocode just to show the sense)

$restrictCss = ( $wgStrippedCss == null ? '/(?:expression|tps*:\/\/|url\\s*\().*/is' : $wgStrippedCss )
if( preg_match( $restrictCss, $stripped ) )
...

where WMF sites would have

$wgStrippedCss = '/(?:expression|tps*:\/\/(?!upload\\.wikimedia\\.org)|url\\s*\(\\s*(?:'|")?\\s*(?!http:\/\/upload\\.wikimedia\\.org)).*/is'

in LocalSettings to allow linking of images uploaded on WMF sites.
Comment 2 Andre Klapper 2014-07-04 12:00:40 UTC
While the logic in checkCss() has changed in the meantime (see https://git.wikimedia.org/blob/mediawiki%2Fcore.git/1765227b43dea46e44094c7380d28e84e5139751/includes%2FSanitizer.php#L851 ), the underlying request here seems to be still valid.
Comment 3 Daniel Renfro 2014-07-05 03:01:34 UTC
Looks like the commit to Sanitizer.php that disallows the CSS url() function was meant to avoid running JavaScript in CSS with the url("javascript:..."); type of thing. (See the Mozilla bug report at https://bugzilla.mozilla.org/show_bug.cgi?id=230134 for more info.) 

While I agree with the security concerns (and bad bad bad coding practice) of executing JavaScript from within a CSS file, I don't agree that the url() function should be completely disallowed. 

The method above (comment1) seems like a good idea: if some global variable ($wgDisallowedCssRegex) is set, use that. If not, fallback to what is there now.

I might tackle this on the weekend if I've got time.
Comment 4 Daniel Renfro 2014-07-06 03:21:33 UTC
See https://gerrit.wikimedia.org/r/#/c/144249/
Comment 5 Gerrit Notification Bot 2014-07-07 20:50:41 UTC
Change 144249 had a related patch set uploaded by Nemo bis:
Breaking out disallowed CSS into a global variable 

https://gerrit.wikimedia.org/r/144249
Comment 6 Gerrit Notification Bot 2014-07-18 00:13:14 UTC
Change 144249 merged by jenkins-bot:
Breaking out disallowed CSS into a global variable

https://gerrit.wikimedia.org/r/144249
Comment 7 Krinkle 2014-07-18 02:29:03 UTC
I'm not sure I see how making this entire thing a configuration variable is a good thing. Security should not be configurable.

Another big reason why url() is forbidden is to avoid cross-domain requests being made from a wiki page (especially with regards to CSRF, DDOS, traffic sniffing, privacy policy etc.).

When additional security issues are found and added to MediaWiki, existing installs that customised this filter for some silly feature, will no longer be using adequate security measures.

I recommend this feature be reverted and we figure out a way to enable this other use of url() in a sane way. Whether we want that way to be allowed always or behind an opt-in flag is a separate question, but I don't think there is valid use case for making the entire thing configurable. That only complicates maintenance, security updates, and overall mobility of wikitext between sites.
Comment 8 Tim Starling 2014-07-18 03:02:10 UTC
I think any exceptions to CSS sanitization should be specific to the use case. I don't think we should expose a regex, since that is an implementation detail internal to Sanitizer, which may change in the future. I don't think it's secure to generally allow url() pointing to any page on a domain, since url() can be used for scary non-image things, like the behavior property.
Comment 9 Daniel Renfro 2014-07-18 03:08:24 UTC
The url() function in CSS is perfectly valid as long as there's not a "javascript:" psuedo-schema in it that executes code. There are plenty of legimate cases where the url() function might be useful/necessary that are currently forbidden by the Sanitizer because it is overly generic. 

Maybe a better fix to this problem, instead of pulling out the regex to be possibly overridden/reset/unset by an admin, is to make sure the string "javascript:" does not appear it in.
Comment 10 Daniel Renfro 2014-07-18 03:09:25 UTC
P.S. I'm totally cool with reverting all this (as Krinkle says above) and finding another way.
Comment 11 Chris Steipp 2014-07-18 17:02:19 UTC
(In reply to Krinkle from comment #7)
> I'm not sure I see how making this entire thing a configuration variable is
> a good thing. Security should not be configurable.

My reason for +2'ing that is that I'd prefer an admin, who really wants to allow some of that css through, put it in their configuration rather than doing their own hack in core, which would be less secure for them.

At the WMF, I would likely never be ok with an exception allowing any url()'s through, so if that really is Danny B.'s only motivation, someone should probably just update the title and wontfix this bug.

> I recommend this feature be reverted and we figure out a way to enable this
> other use of url() in a sane way. Whether we want that way to be allowed
> always or behind an opt-in flag is a separate question, but I don't think
> there is valid use case for making the entire thing configurable. That only
> complicates maintenance, security updates, and overall mobility of wikitext
> between sites.

I'm fine with reverting it this change, since Timo and Tim seem to feel strongly about it. Also it definitely hurts the mobility of wikitext between sites, I hadn't considered that aspect of it.

I'm honestly skeptical we can find a sane way to support urls in css, but I'm open to being surprised. And again, if that's the goal, let's change the bug reflect what's actually wanted.
Comment 12 Chris Steipp 2014-07-18 17:05:13 UTC
Reverted in I1dbb927997693d686b4677b9c2107be99dedd7b2

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links