On 2011/04/10 18:32:38, xtof wrote:
I think this is pretty much ready, except for one thing I just thought
of.
Sorry, I should have thought of that earlier :/
In ClippedImageImpl, we're using a SafeUri in the context of a url()
style
expression ("background: url({3})"). However, the contract of SafeUri
is
currently not tight enough for this to be safe I think: SafeUri would
allow
e.g., parentheses, colons and dashes in un-escaped form in the URL.
In a
template where the URL appears in a url() css expression as above, a
URL like,
http://harmless.com/foo);background:url(javascript:evil());
would pass UriUtils#sanitizeUri (without getting URI-escaped or
anything), and
would result in script execution via CSS injection.
I hadn't looked closely at the SafeStyles API and thought it would be
covered there (and ClippedImageImpl would of course be updated to use
SafeStyles).
But note that, similarly, http://harmless.com"onclick="evil()" would
pass sanitizeUri and could result in script execution via HTML injection
(not through SafeHtml/SafeHtmlTemplates though, as the quotes would be
escaped).
The CSS spec says whitespace, quotes (single and double) and parentheses
can be escaped using a backslash:
http://www.w3.org/TR/CSS2/syndata.html#uri
So maybe we could special-case SafeUri in CSS contexts in the generator?
(we already special-case it for it's .asString(), and special-case URL
contexts for non-SafeUri values to sanitize them, so...)
So I think we need to,
- tighten the SafeUri contract so it explicitly specifies which
character set
can occur in a SafeUri
- add a sanitizeAndEscapeUri method to UriUtils that both checks for
an allowed
scheme and URI/%-escapes characters not in the allowed charset (or
maybe rejects
URLs with certain funny characters in them).
We can quite simply %-escape those chars that are special to CSS (see
above), or use the more radical approach of re-%-encode the whole URI:
i.e. %-encode any "funny char" (except % chars that are not followed by
a pair of hex digits; similar to SafeHtmlUtils#htmlEscapeAllowEntities,
but for %-escaping).
Or we can reverse-engineer the (proposed) Web Address algorithm to only
%-encode those chars that would cause issues:
http://www.w3.org/html/wg/href/draft continued at
http://tools.ietf.org/html/draft-ietf-iri-3987bis-05#section-7.2
But I wonder if this shouldn't be the rule for sanitizeUri (instead of
introducing a new method and leaving a "hole" in sanitizeUri under
certain circumstances).
There should probably be an encodeAllowEscapes too, similar to
htmlEscapeAllowEntities, when e.g. you know your URL is safe, but you're
unsure if it's "encoded enough" for every context (including CSS), i.e.
the URL could contain unescaped parentheses or single quotes.
Should we defer this patch? or tackle this in a following one? (maybe
adding a TODO in
fromTrustedString/fromString/fromUntrustedString/sanitizeUri?)
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
