321010 - Need better stripping of illegal css chars from category names

Status: RESOLVED FIXED

(Calendar :: Security, defect)

Description

[Joey Minta]

dmose identified this in https://bugzilla.mozilla.org/show_bug.cgi?id=297934#c31

I'm not really sure if there's any way to actually do more than cause a CSS parsing error, but regardless we should just update our regexps to strip out any problematic characters from category names.

As far as I know, this has never been exploited in any way.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I can't actually think of an exploit for this other than causing the appropriate stylesheet not to apply to this element, which would simply make it not be colored the way one might expect.  As such, removing from the 0.1 blocker list.

Comment 2

[Joey Minta]

We also need to a better job of detecting duplicates when this happens.  What we really need is a set of global category<->safeCategory functions, so that we can share them in prefs, the event dialog, and the color-management.

Comment 3

[gekacheka]

Attachment: patch part1 v1: replace nonascii and nonalphanum chars with hexadecimal unicode char code

(patch -l -p 1 -i file.patch)

Replaces whitespace with '_'.
Replaces nonascii and non-alpha-num chars with hexadecimal unicode char code.
(Result must be lowercase for category color style rule to work.
 Was not able to get backslash escapes to work.
 Was not able to get encodeURIComponent to work [assumes 8bit chars?].)

Tested on w2k SB by creating categories with non-alphanum and/or nonascii characters, and giving them colors.  Events in those categories successfully picked up the appropriate category color bar.  (Exception: comma char ',')

Comment 4

[gekacheka]

Attachment: patch part2 v1: escape literal commas in category names

(patch -l -p 1 -i file.patch)

Escape commas within a category name when stored, so can distinguish between a escaped comma within a category name and the unescaped commas between category names.  Unescape them when retrieved.  Adds utility functions categoriesStringToArray and categoriesArrayToString, and uses them in categories.js (preference pane), item dialogs, and grid views.

Tested as follows: 
a. open options | categories, then creating a category with commas such as 
    "Alpha, Beta, and Associates" and giving it a color, such as red.
b. the category persists after closing the options dialog and opening it 
   again.  (It does not get split it multiple categories.) 
c. create an event with this category
d. verify that it is displayed with the color stripe in each grid view
e. verify that category is preserved after opening event again in
   edit dialog. (It does not get split into multiple categories.)

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 298197 [details] [diff] [review]
patch part1 v1: replace nonascii and nonalphanum chars with hexadecimal unicode char code

>diff -r f4cf6d37ca1e mozilla/calendar/base/src/calUtils.js
>--- a/mozilla/calendar/base/src/calUtils.js	Sun Jan 20 12:19:41 2008 -0500
>+++ b/mozilla/calendar/base/src/calUtils.js	Sun Jan 20 18:07:00 2008 -0500
>@@ -171,7 +171,34 @@ function calendarDefaultTimezone() {
>  * @return              The formatted string
>  */
> function formatStringForCSSRule(aString) {
>-    return aString.replace(/ /g, "_").toLowerCase();
>+    // replace whitespace with '_'.
>+    // replace nonascii and ascii not in nmchar with hexadecimal unicode.
>+    //   nmchar		[_a-zA-Z0-9-]|{nonascii}|{escape}
>+    //   name		{nmchar}+
>+    //   http://www.w3.org/TR/CSS21/grammar.html#scanner
>+    var result = aString.replace(/[ \t]/g, "_");
>+    if (result.search(/[^_a-zA-Z0-9-]/) != -1) { // has illegal chars

Why do you need to go through each character separately? This might also do it, or did I miss something?

I tested with "\u2713" and "\u00A9", which with a normal text appended result in:
_uxa9__ux20_Foo_ux20_Bar
_ux2713__ux20_Foo_ux20_Bar

> function formatStringForCSSRule(aString) {
>     function toHex(nat) { // char code is natural number (positive integer)
>         var str = "";
>         while (nat != 0) {
>             str = "0123456789abcdef".charAt(nat & 0xf) + str;
>             nat >>= 4;
>         }
>         return str;
>     }
>     var result = aString.replace(/[ \t]/g, "_");
>     return result.replace(/[^_a-zA-Z0-9-]/g, function(match) {
>                           return "_ux" + toHex( match.charCodeAt(0)) + "_";
>                           })
>                  .toLowerCase();
> }


Aside from that, we probably also want to replace  _ with its _uxNN_ variant, so that category names like "_uxa9_" don't have equal color to the category "©". I know this is higly unprobable, but since its just a matter of removing the _ from the regex, I think its ok.

r+ if my solution doesn't work, otherwise it would be great if you can create a new patch with the update function.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 298202 [details] [diff] [review]
patch part2 v1: escape literal commas in category names

>+function categoriesStringToArray(aCategories) {
>+    function revertCommas(name) { return name.replace(/~cOmMa~/g, ","); }
>+    return aCategories.replace(/\\,/g, "~cOmMa~").split(",").map(revertCommas);
>+}
Instead of a cryptic string that the user could possibly input, you could use a null character such as \u0000. dumping the string will let the string temporarily appear blank, but since you replace backwards again everything should be fine.

Otherwise everything looks fine, r=philipp

Comment 7

[gekacheka]

Attachment: patch part1 v2: replace nonascii and nonalphanum chars with hexadecimal unicode char code invertibly

(patch -l -p 1 -i file.patch)

CHANGES

calUtils.js: formatStringForCSSRule
  Made formatStringForCSSRule a one-to-one function so there is no
  danger of two category names mapping to same formatted name: 
  (f(a) = f(b)) implies (a = b), so it is invertible.  Also made it run
  in two passes on string instead of three, for efficiency (thanks for
  pointing out that string replace can take a function parameter).

calendar-views.js: updateStyleSheetForObject
  A consequence of the above change is that formatStringForCSSRule
  should no longer be called on its result values: it is no longer
  idempotent, so if f(a) != a, then f(f(a)) != f(a).  Therefore,
  removed call to formatStringForCSSRule function in
  updateStyleSheetForObject, because the colored category names here
  come from preference keys, and the preference keys are already the
  result of applying formatStringForCSSRule to the category (because
  preference keys cannot be unicode).

calendar-management.js: calendarListInitCategoryColors
  A consequence of the second change above is that old color
  preference keys might still exist and produce illegal style rules,
  causing errors.  Therefore, during category color startup
  initialization it now calls calendarConvertObsoleteColorPrefs, which
  converts old color preference keys to the new format if the new
  format key does not already exist, and in any case filters out
  illegal category names so they cannot produce CSS style rule errors.

Tested as before (comment 3) using SB on w2k.  To test conversion,
with no SB running, added some old style category color preferences to
prefs.js by hand (such as "calendar.category.color.A / B"), then start
SB, exit SB, and examine file to verify that they were converted.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 298371 [details] [diff] [review]
patch part1 v2: replace nonascii and nonalphanum chars with hexadecimal unicode char code invertibly

> function formatStringForCSSRule(aString) {
>+    function toReplacement(ch) {
>+        // char code is natural number (positive integer)
>+        var nat = ch.charCodeAt(0);
>+        switch(nat) {
>+        case 0x20: // space
>+            return "_";
>+        default:
>+            var str = "";
>+            while (nat != 0) { 
>+                str = "0123456789abcdef".charAt(nat & 0xf) + str; // lowercase
>+                nat >>= 4;
>+            }
>+            return "-ux"+str+"-"; // lowercase
>+        }
>+    }
>+    // Result must be lowercase or style rule will not work.
>+    return aString.toLowerCase().replace(/[^a-zA-Z0-9]/g, toReplacement);
> }

I found yet another shortcut which you can also use in your other patch. I can go ahead and update both patch before checking in, if you like.

>    function toReplacement(ch) {
>        // char code is natural number (positive integer)
>        var nat = ch.charCodeAt(0);
>        switch(nat) {
>        case 0x20: // space
>            return "_";
>        default:
>            return "-ux" + nat.toString(16) + "-"; // lowercase
>        }
>    }
>    // Result must be lowercase or style rule will not work.
>    return aString.toLowerCase().replace(/[^a-zA-Z0-9]/g, toReplacement);
>}

Also I think its ok to not special case space, -ux20- is fine for me since the user won't see it. I don't have a strong opinion on that though.


Other than that just a few minor missing whitespaces (i.e around operators), r=philipp

Comment 9

[gekacheka]

> I found yet another shortcut which you can also use in your other patch. I can
> go ahead and update both patch before checking in, if you like.

The nat.toString(16) change looks good.

My thoughts regarding encoding of space:  Space is common.  The users (and developers and testers) do see the strings when debugging problems with prefs.js and when looking at options | advanced | config editor.  The '_' will be easier to read and faster to encode.

For ~cOmMa~, please substitute \uFFFF instead; it is the unicode non-char for purposes like this I think.

Thanks!

(p.s. One issue for a follow up bug: the encoding for comma currently stores comma in ICS as "\," and the escaped comma as "\\\\," but there currently is no interface to get multiple value properties like CATEGORIES from an item so there appears to be no way to use just "," for comma between categories.)

Comment 10

[Martin Schröder [:mschroeder]]

gekacheka, Philipp: Are both patches ready for checkin, or do they need an additional iteration with nits fixed?

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Patch as checked in

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Thanks for the patch!