[Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Simon Urli
Hi everyone,

I'm currently working on improving security on XWiki comments. We
already use a restricted mode in our comments but that does not cover
every possible case. In order to improve it we should also filter out
some part of the html when using the html  macro.

I propose:

   (a) that we use a configurable whitelist of HTML attributes that
would be allowed in the output HTML: all the other attributes would be
filtered out.

   (b) that the HTML macro is put in restricted mode for users who do
not have scripting rights.

For (a) I'm hesitating between a whitelist or a blacklist: I assume a
blacklist would be shorter but there's also more risk of missing
something. On the contrary a configurable whitelist doesn't prevent
administrator to accept more than what we give in standard.

A first whitelist could be (taken from:
https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146)
alt, class, height, id, name, rel, scope, style, target, title, width

Note that href is not included in this list for example.

WDYT?

Simon

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Simon Urli


On 25/07/2019 10:39, Simon Urli wrote:

> Hi everyone,
>
> I'm currently working on improving security on XWiki comments. We
> already use a restricted mode in our comments but that does not cover
> every possible case. In order to improve it we should also filter out
> some part of the html when using the html  macro.
>
> I propose:
>
>    (a) that we use a configurable whitelist of HTML attributes that
> would be allowed in the output HTML: all the other attributes would be
> filtered out.
>
>    (b) that the HTML macro is put in restricted mode for users who do
> not have scripting rights.
>
> For (a) I'm hesitating between a whitelist or a blacklist: I assume a
> blacklist would be shorter but there's also more risk of missing
> something. On the contrary a configurable whitelist doesn't prevent
> administrator to accept more than what we give in standard.
>
> A first whitelist could be (taken from:
> https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146)
>
> alt, class, height, id, name, rel, scope, style, target, title, width
>
> Note that href is not included in this list for example.

IMO the href not being included in the list is related to the
possibility to write something like:
<a href="javascript:myfunction()">

Now I guess we could also detect the "javascript:" prefix in the href
attribute in restricted mode and discard only those, I don't see other
usecase where it could be a problem.

>
> WDYT?
>
> Simon
>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Thomas Mortagne
Administrator
Note that this mail is only for the html cleaner. But this white list
will also be important to fix
https://jira.xwiki.org/browse/XWIKI-9151.

On Thu, Jul 25, 2019 at 10:49 AM Simon Urli <[hidden email]> wrote:

>
>
>
> On 25/07/2019 10:39, Simon Urli wrote:
> > Hi everyone,
> >
> > I'm currently working on improving security on XWiki comments. We
> > already use a restricted mode in our comments but that does not cover
> > every possible case. In order to improve it we should also filter out
> > some part of the html when using the html  macro.
> >
> > I propose:
> >
> >    (a) that we use a configurable whitelist of HTML attributes that
> > would be allowed in the output HTML: all the other attributes would be
> > filtered out.
> >
> >    (b) that the HTML macro is put in restricted mode for users who do
> > not have scripting rights.
> >
> > For (a) I'm hesitating between a whitelist or a blacklist: I assume a
> > blacklist would be shorter but there's also more risk of missing
> > something. On the contrary a configurable whitelist doesn't prevent
> > administrator to accept more than what we give in standard.
> >
> > A first whitelist could be (taken from:
> > https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146)
> >
> > alt, class, height, id, name, rel, scope, style, target, title, width
> >
> > Note that href is not included in this list for example.
>
> IMO the href not being included in the list is related to the
> possibility to write something like:
> <a href="javascript:myfunction()">
>
> Now I guess we could also detect the "javascript:" prefix in the href
> attribute in restricted mode and discard only those, I don't see other
> usecase where it could be a problem.
>
> >
> > WDYT?
> >
> > Simon
> >
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com



--
Thomas Mortagne
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Marius Dumitru Florea
In reply to this post by Simon Urli
On Thu, Jul 25, 2019 at 11:39 AM Simon Urli <[hidden email]> wrote:

> Hi everyone,
>
> I'm currently working on improving security on XWiki comments. We
> already use a restricted mode in our comments but that does not cover
> every possible case. In order to improve it we should also filter out
> some part of the html when using the html  macro.
>

Is the HTML macro needed in comments? Do you have a real use case for this?
Wouldn't it be more simple to completly forbid HTML and script macros in
comments?

Thanks,
Marius


>
> I propose:
>
>    (a) that we use a configurable whitelist of HTML attributes that
> would be allowed in the output HTML: all the other attributes would be
> filtered out.
>
>    (b) that the HTML macro is put in restricted mode for users who do
> not have scripting rights.
>
> For (a) I'm hesitating between a whitelist or a blacklist: I assume a
> blacklist would be shorter but there's also more risk of missing
> something. On the contrary a configurable whitelist doesn't prevent
> administrator to accept more than what we give in standard.
>
> A first whitelist could be (taken from:
>
> https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146
> )
> alt, class, height, id, name, rel, scope, style, target, title, width
>
> Note that href is not included in this list for example.
>
> WDYT?
>
> Simon
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com
>
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Simon Urli
Hi Marius,

On 25/07/2019 16:22, Marius Dumitru Florea wrote:

> On Thu, Jul 25, 2019 at 11:39 AM Simon Urli <[hidden email]> wrote:
>
>> Hi everyone,
>>
>> I'm currently working on improving security on XWiki comments. We
>> already use a restricted mode in our comments but that does not cover
>> every possible case. In order to improve it we should also filter out
>> some part of the html when using the html  macro.
>>
>
> Is the HTML macro needed in comments? Do you have a real use case for this?
> Wouldn't it be more simple to completly forbid HTML and script macros in
> comments?

That's also an option, but potentially more breaking.

IMO the decision was already taken a while ago with
https://github.com/xwiki-contrib/syntax-markdown/commit/907dc0630a1b7cad4292c1472deb7ae4d68c66ae 
which forces to use html macro with clean option when used in restricted
mode, but does not prevent to use html macro at all. Contrary to the
script macro which have all been disabled on the following commit:
https://github.com/xwiki/xwiki-platform/commit/544d290a37cf86dff27aa7bc12be3f6a219f1508

Simon

>
> Thanks,
> Marius
>
>
>>
>> I propose:
>>
>>     (a) that we use a configurable whitelist of HTML attributes that
>> would be allowed in the output HTML: all the other attributes would be
>> filtered out.
>>
>>     (b) that the HTML macro is put in restricted mode for users who do
>> not have scripting rights.
>>
>> For (a) I'm hesitating between a whitelist or a blacklist: I assume a
>> blacklist would be shorter but there's also more risk of missing
>> something. On the contrary a configurable whitelist doesn't prevent
>> administrator to accept more than what we give in standard.
>>
>> A first whitelist could be (taken from:
>>
>> https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146
>> )
>> alt, class, height, id, name, rel, scope, style, target, title, width
>>
>> Note that href is not included in this list for example.
>>
>> WDYT?
>>
>> Simon
>>
>> --
>> Simon Urli
>> Software Engineer at XWiki SAS
>> [hidden email]
>> More about us at http://www.xwiki.com
>>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Marius Dumitru Florea
On Thu, Jul 25, 2019 at 5:33 PM Simon Urli <[hidden email]> wrote:

> Hi Marius,
>
> On 25/07/2019 16:22, Marius Dumitru Florea wrote:
> > On Thu, Jul 25, 2019 at 11:39 AM Simon Urli <[hidden email]>
> wrote:
> >
> >> Hi everyone,
> >>
> >> I'm currently working on improving security on XWiki comments. We
> >> already use a restricted mode in our comments but that does not cover
> >> every possible case. In order to improve it we should also filter out
> >> some part of the html when using the html  macro.
> >>
> >
> > Is the HTML macro needed in comments? Do you have a real use case for
> this?
> > Wouldn't it be more simple to completly forbid HTML and script macros in
> > comments?
>
> That's also an option, but potentially more breaking.
>
> IMO the decision was already taken a while ago with
>
> https://github.com/xwiki-contrib/syntax-markdown/commit/907dc0630a1b7cad4292c1472deb7ae4d68c66ae


I don't understand why a change in markdown syntax (contrib project) can be
considered as decision for the XWiki Project. But anyway, any decision can
be revisited.

Again, do you have a real use case where you need HTML macro in comments?


>
> which forces to use html macro with clean option when used in restricted
> mode, but does not prevent to use html macro at all. Contrary to the
> script macro which have all been disabled on the following commit:
>
> https://github.com/xwiki/xwiki-platform/commit/544d290a37cf86dff27aa7bc12be3f6a219f1508
>
> Simon
> >
> > Thanks,
> > Marius
> >
> >
> >>
> >> I propose:
> >>
> >>     (a) that we use a configurable whitelist of HTML attributes that
> >> would be allowed in the output HTML: all the other attributes would be
> >> filtered out.
> >>
> >>     (b) that the HTML macro is put in restricted mode for users who do
> >> not have scripting rights.
> >>
> >> For (a) I'm hesitating between a whitelist or a blacklist: I assume a
> >> blacklist would be shorter but there's also more risk of missing
> >> something. On the contrary a configurable whitelist doesn't prevent
> >> administrator to accept more than what we give in standard.
> >>
> >> A first whitelist could be (taken from:
> >>
> >>
> https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146
> >> )
> >> alt, class, height, id, name, rel, scope, style, target, title, width
> >>
> >> Note that href is not included in this list for example.
> >>
> >> WDYT?
> >>
> >> Simon
> >>
> >> --
> >> Simon Urli
> >> Software Engineer at XWiki SAS
> >> [hidden email]
> >> More about us at http://www.xwiki.com
> >>
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com
>
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Simon Urli


On 25/07/2019 16:49, Marius Dumitru Florea wrote:

> On Thu, Jul 25, 2019 at 5:33 PM Simon Urli <[hidden email]> wrote:
>
>> Hi Marius,
>>
>> On 25/07/2019 16:22, Marius Dumitru Florea wrote:
>>> On Thu, Jul 25, 2019 at 11:39 AM Simon Urli <[hidden email]>
>> wrote:
>>>
>>>> Hi everyone,
>>>>
>>>> I'm currently working on improving security on XWiki comments. We
>>>> already use a restricted mode in our comments but that does not cover
>>>> every possible case. In order to improve it we should also filter out
>>>> some part of the html when using the html  macro.
>>>>
>>>
>>> Is the HTML macro needed in comments? Do you have a real use case for
>> this?
>>> Wouldn't it be more simple to completly forbid HTML and script macros in
>>> comments?
>>
>> That's also an option, but potentially more breaking.
>>
>> IMO the decision was already taken a while ago with
>>
>> https://github.com/xwiki-contrib/syntax-markdown/commit/907dc0630a1b7cad4292c1472deb7ae4d68c66ae
>
>
> I don't understand why a change in markdown syntax (contrib project) can be
> considered as decision for the XWiki Project. But anyway, any decision can
> be revisited.
>

Woops, I messed up with my links sorry, it was this one:
https://github.com/xwiki/xwiki-rendering/commit/907dc0630a1b7cad4292c1472deb7ae4d68c66ae

> Again, do you have a real use case where you need HTML macro in comments?

I don't need HTML Macro in comments, but since it has never been
forbidden I assume that lot of users already used comments with html
macro, in which case it would be a breaking change for them to forbid it
now.

Now, if the restricted mode is only used internally for comments AFAIK,
we recently made it available in the API too for the whole document
content (cf https://jira.xwiki.org/browse/XWIKI-16459), and I don't
think in would be a good idea for those to forbid the usage of HTML macro.

Simon

>
>
>>
>> which forces to use html macro with clean option when used in restricted
>> mode, but does not prevent to use html macro at all. Contrary to the
>> script macro which have all been disabled on the following commit:
>>
>> https://github.com/xwiki/xwiki-platform/commit/544d290a37cf86dff27aa7bc12be3f6a219f1508
>>
>> Simon
>>>
>>> Thanks,
>>> Marius
>>>
>>>
>>>>
>>>> I propose:
>>>>
>>>>      (a) that we use a configurable whitelist of HTML attributes that
>>>> would be allowed in the output HTML: all the other attributes would be
>>>> filtered out.
>>>>
>>>>      (b) that the HTML macro is put in restricted mode for users who do
>>>> not have scripting rights.
>>>>
>>>> For (a) I'm hesitating between a whitelist or a blacklist: I assume a
>>>> blacklist would be shorter but there's also more risk of missing
>>>> something. On the contrary a configurable whitelist doesn't prevent
>>>> administrator to accept more than what we give in standard.
>>>>
>>>> A first whitelist could be (taken from:
>>>>
>>>>
>> https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146
>>>> )
>>>> alt, class, height, id, name, rel, scope, style, target, title, width
>>>>
>>>> Note that href is not included in this list for example.
>>>>
>>>> WDYT?
>>>>
>>>> Simon
>>>>
>>>> --
>>>> Simon Urli
>>>> Software Engineer at XWiki SAS
>>>> [hidden email]
>>>> More about us at http://www.xwiki.com
>>>>
>>
>> --
>> Simon Urli
>> Software Engineer at XWiki SAS
>> [hidden email]
>> More about us at http://www.xwiki.com
>>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

Clemens Robbenhaar-3
In reply to this post by Thomas Mortagne
Personally I favor a whitelist for the allowed attributes.

It seems one needs them for wiki syntax like (%
onmouseover="alert('gotcha');" %) , too.
This means we do not only need it for the HTML-Macro, but also in
(nearly all?) other places.
(Then for me it is ok to discuss this for the HTML-Macro first, where it
might be more obvious that one can do ... interesting things with it.)

For the "href"  I'd prefer allowing it and check for the 'javascript:'
prefix, but I have to admit that I do not know how to implement this, so
my option counts little when it is about to choose what to implement.

But then, on second thoughts one might do interesting things with e.g.:

{{html clean="false" wiki="false"}}
<a
href="data:application/html;base64,PHNjcmlwdD5hbGVydCgnZ290Y2hhJyk8L3NjcmlwdD4=">click</a>
{{/html}}

where the base64 encoded part decodes to:
<script>alert('gotcha')</script>, so it seems safer to disallow it by
default. (This does not work out of the box, but requires the user to
choose an application to open the link, but still ...)

For the "style" attribute there are some ways to create XSS style
attacks as CSS became more powerful over time; e.g.
https://stackoverflow.com/q/4546591
Thus I am not sure if we should include it in the whitelist by default.
I think the creation of StyleSheetExtensions in a wiki page needs Admin
rights for the same reason already.

Clemens

On 25.07.19 10:54, Thomas Mortagne wrote:

> Note that this mail is only for the html cleaner. But this white list
> will also be important to fix
> https://jira.xwiki.org/browse/XWIKI-9151.
>
> On Thu, Jul 25, 2019 at 10:49 AM Simon Urli <[hidden email]> wrote:
>>
>>
>> On 25/07/2019 10:39, Simon Urli wrote:
>>> Hi everyone,
>>>
>>> I'm currently working on improving security on XWiki comments. We
>>> already use a restricted mode in our comments but that does not cover
>>> every possible case. In order to improve it we should also filter out
>>> some part of the html when using the html  macro.
>>>
>>> I propose:
>>>
>>>     (a) that we use a configurable whitelist of HTML attributes that
>>> would be allowed in the output HTML: all the other attributes would be
>>> filtered out.
>>>
>>>     (b) that the HTML macro is put in restricted mode for users who do
>>> not have scripting rights.
>>>
>>> For (a) I'm hesitating between a whitelist or a blacklist: I assume a
>>> blacklist would be shorter but there's also more risk of missing
>>> something. On the contrary a configurable whitelist doesn't prevent
>>> administrator to accept more than what we give in standard.
>>>
>>> A first whitelist could be (taken from:
>>> https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146)
>>>
>>> alt, class, height, id, name, rel, scope, style, target, title, width
>>>
>>> Note that href is not included in this list for example.
>> IMO the href not being included in the list is related to the
>> possibility to write something like:
>> <a href="javascript:myfunction()">
>>
>> Now I guess we could also detect the "javascript:" prefix in the href
>> attribute in restricted mode and discard only those, I don't see other
>> usecase where it could be a problem.
>>
>>> WDYT?
>>>
>>> Simon
>>>
>> --
>> Simon Urli
>> Software Engineer at XWiki SAS
>> [hidden email]
>> More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Use an attribute whitelist for HtmlCleaner restricted mode

vmassol
Administrator
In reply to this post by Simon Urli
Hi Simon,

I haven’t read this thread yet but just wanted to chime in to mention that Thomas Delafosse had worked on this in the past and had an implementation for it. AFAIR he even pushed a PR for it (which is probably still existing). Could be interesting to see what he did. We also probably discussed it on the devs list at the time but that might be harder to find so it’s good you’re asking again (and time has passed so what we said back then could be different today!).

Thanks
-Vincent

> On 25 Jul 2019, at 10:39, Simon Urli <[hidden email]> wrote:
>
> Hi everyone,
>
> I'm currently working on improving security on XWiki comments. We already use a restricted mode in our comments but that does not cover every possible case. In order to improve it we should also filter out some part of the html when using the html  macro.
>
> I propose:
>
>  (a) that we use a configurable whitelist of HTML attributes that would be allowed in the output HTML: all the other attributes would be filtered out.
>
>  (b) that the HTML macro is put in restricted mode for users who do not have scripting rights.
>
> For (a) I'm hesitating between a whitelist or a blacklist: I assume a blacklist would be shorter but there's also more risk of missing something. On the contrary a configurable whitelist doesn't prevent administrator to accept more than what we give in standard.
>
> A first whitelist could be (taken from: https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146)
> alt, class, height, id, name, rel, scope, style, target, title, width
>
> Note that href is not included in this list for example.
>
> WDYT?
>
> Simon
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com