Discussion:
[Bug 73156] New: Security review of OOjs php
b***@wikimedia.org
2014-11-07 20:56:58 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

Bug ID: 73156
Summary: Security review of OOjs php
Product: Wikimedia
Version: wmf-deployment
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: Unprioritized
Component: Extension setup
Assignee: ***@wikimedia.org
Reporter: ***@wikimedia.org
CC: ***@espace-win.org, ***@wikimedia.org
Web browser: ---
Mobile Platform: ---

Nothing too concerning with what you're doing. Security is roughly the same as
using Html/Xml classes at this point.

The only thing I'd really like to see changed is in
php/widgets/InputWidget.php, the "sanitizeValue" function doesn't do any
(security) sanitization, which I think that could cause confusion later on. If
the name can't be changed, maybe make the comments explicit that it's not
security sanitization?

It would also be nice to have some extra sanitization built in from the start,
which we can't do in the Html/Xml classes since they're abused in odd ways, but
have bitten some developers (SemanticForms had bunch of issues because they
assumed these happened):
* Validate tag name will be parsed in html as a single tag name-- so doesn't
contain whitespace, /, >, or null.
* Validate attribute names don't contain whitespace, /, =, >
* Validate that form actions and button hrefs aren't javascript: urls

There are also a couple of places you're adding style attributes directly. Is
it possible to avoid that? My long-term plan is to have MediaWiki set a Content
Security Policy that doesn't allow inline css, so I'd prefer to not introduce
new uses of it, if possible.
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-07 21:07:07 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

Krinkle <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com
Summary|Security review of OOjs php |Security review of OOjs
| |UI's PHP implementation
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 21:25:32 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

Bartosz Dziewoński <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Assignee|***@wikimedia.org |***@gmail.com

--- Comment #1 from Bartosz Dziewoński <***@gmail.com> ---
Thanks for the review. Good points, let's do this.


(In reply to Chris Steipp from comment #0)
Post by b***@wikimedia.org
There are also a couple of places you're adding style attributes directly.
Is it possible to avoid that? My long-term plan is to have MediaWiki set a
Content Security Policy that doesn't allow inline css, so I'd prefer to not
introduce new uses of it, if possible.
It can't be avoided in all cases, for example GridLayout depends on them –
can't be fixed without a major refactoring or even just killing the things. We
also want to have the same behavior in PHP as in JS (or a subset), whenever
possible.

We could probably get rid of them in at least some places, such as
LabelElement. I'll look into them.
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 21:32:19 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

--- Comment #2 from Gerrit Notification Bot <***@wikimedia.org> ---
Change 174814 had a related patch set uploaded by Bartosz Dziewoński:
LinkTargetInputWidget: Update for #sanitizeValue → #cleanUpValue OOUI change

https://gerrit.wikimedia.org/r/174814
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 21:32:21 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

--- Comment #3 from Gerrit Notification Bot <***@wikimedia.org> ---
Change 174815 had a related patch set uploaded by Bartosz Dziewoński:
[BREAKING CHANGE] Rename InputWidget#sanitizeValue → #cleanUpValue

https://gerrit.wikimedia.org/r/174815
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 21:32:23 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

Gerrit Notification Bot <***@wikimedia.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |PATCH_TO_REVIEW
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 22:27:45 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

--- Comment #4 from Gerrit Notification Bot <***@wikimedia.org> ---
Change 174835 had a related patch set uploaded by Bartosz Dziewoński:
PHP: Reject malformed and potentially evil input when outputting HTML

https://gerrit.wikimedia.org/r/174835
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 22:37:58 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

--- Comment #5 from Bartosz Dziewoński <***@gmail.com> ---
(In reply to Chris Steipp from comment #0)
Post by b***@wikimedia.org
The only thing I'd really like to see changed is in
php/widgets/InputWidget.php, the "sanitizeValue" function doesn't do any
(security) sanitization, which I think that could cause confusion later on.
If the name can't be changed, maybe make the comments explicit that it's not
security sanitization?
https://gerrit.wikimedia.org/r/174815
https://gerrit.wikimedia.org/r/174814
Post by b***@wikimedia.org
It would also be nice to have some extra sanitization built in from the
start, which we can't do in the Html/Xml classes since they're abused in odd
ways, but have bitten some developers (SemanticForms had bunch of issues
* Validate tag name will be parsed in html as a single tag name-- so doesn't
contain whitespace, /, >, or null.
* Validate attribute names don't contain whitespace, /, =, >
* Validate that form actions and button hrefs aren't javascript: urls
https://gerrit.wikimedia.org/r/174835
https://gerrit.wikimedia.org/r/174833 (dependency)
https://gerrit.wikimedia.org/r/174834 (dependency)
Post by b***@wikimedia.org
There are also a couple of places you're adding style attributes directly.
Is it possible to avoid that? My long-term plan is to have MediaWiki set a
Content Security Policy that doesn't allow inline css, so I'd prefer to not
introduce new uses of it, if possible.
So in the end, there are only three such places in PHP code:

* LabelElement. This one already came up earlier as a code quality
issue, actually. Should be fixable, but preferably not right now :)
Filed bug 73677 so it's not forgotten.
* GridLayout. Alas, inline styles are a core part of how it works and
the only way to fix it would be to remove the class entirely.
* widgets.php demo, where it's used on a GridLayout (and is also
necessary).
--
You are receiving this mail because:
You are on the CC list for the bug.
b***@wikimedia.org
2014-11-20 23:10:52 UTC
Permalink
https://bugzilla.wikimedia.org/show_bug.cgi?id=73156

--- Comment #6 from Gerrit Notification Bot <***@wikimedia.org> ---
Change 174844 had a related patch set uploaded by Bartosz Dziewoński:
LabelElement: Kill inline styles

https://gerrit.wikimedia.org/r/174844
--
You are receiving this mail because:
You are on the CC list for the bug.
Loading...