Thursday, April 29, 2021
1 change · master
Security fixes and vulnerability patches
This change reduces the risk of unsafe content being inserted into pages and emails by replacing direct raw rendering with safer, automatically escaped output. It updates several Odoo areas so existing website, mail, reporting, digest, live chat, payment, and portal content continues to display correctly while improving protection against injection issues.
Original PR description
`t-raw` is a relatively regular source of issues, not just because it gets misused (though that happens) but because it's a form of action at a distance: a commit adds a `t-raw` with a checked input,…
`t-raw` is a relatively regular source of issues, not just because it
gets misused (though that happens) but because it's a form of action
at a distance: a commit adds a `t-raw` with a checked input, this gets
validated and merged, then the *input* gets modified or overridden or
... and the devs & reviewers don't realize it will end up in a
`t-raw`... and we've got an injection vector.
The markupsafe package (on which we already have a hard dependency)
turns out to have a solution for that: if the parameter to
`markupsafe.escape` has a `__html__` method, it will just call that.
It also provides `markupsafe.Markup` which defines exactly one method,
and furthermore will automatically escape the parameters to its
methods (which can be a bit disconcerting for some) as well as
formatting parameters e.g.
Markup("foo %s %s") % (a, b)
will automatically escape `a` and `b` before they're merged into the
format string, and will return a markup. Obviously that doesn't work
the other way around (because `str.__mod__` and `str.format` always
return a string). It does work for concatenation though.
This means if we can use `markupsafe.escape` as the standard escape
function (which is a good idea since werkzeug [is deprecating
`werkzeug.utils.escape`
anyway](https://github.com/pallets/werkzeug/issues/1758)) we can
leverage this property by requiring the *sources* to be properly
tagged for them to be directly injectible, this means in the scenario
above the payload will either be properly escaped OOTB or it will
break the existing system, but either way it should not be a
*security* risk. And we can easily flag mentions of `Markup` or
`__html__` for security review.
This PR updates `qweb` to use `markupsafe.escape` as its escaping
function, and adds a warning to `t-raw`. It also updates a number of
APIs to make them markup-safe by default:
* html_escape (that's actually what `markupsafe.escape` does on all of its own)
* html_sanitize
* the output of HTML fields
* scriptsafe JSON
* nl2br (which internally escapes its input if necessary)
* qweb markup bodies e.g. `<t t-set="thing">body</t>` the value is markup-safe
* qweb *output*, however `qweb._render` returns *binary data*, for the
most part this is invisible because we keep using `to_text` and
`ustr` everywhere and those decode bytes to text automatically, but
`Markup` inherits from `str` so we can't just have qweb return a
`Markup`, it generates garbage. Therefore it returns
`MarkupSafeBytes`
`to_text` was also updated to let `Markup` instances pass through
unmolested. `ustr`, however, was not.
New wrapper types:
`MarkupSafeBytes` is used as the output of `qweb._render`. It extends
`bytes` as `Markup` extends `str`, though it doesn't redefine all the
utility escaping `Markup` does. For the most part:
* It decodes to a `Markup`.
* It defines an `__html__` which decodes to and returns a `Markup`.
`_ScriptSafe` is the new output of the scriptsafe JSON module. It
defines its own `__html__` which applies script-safe escaping at point
of use, it doesn't use the `Markup` wrapper to avoid issues of
unescaping and double-escaping related to attributes, with which
script-safe escaping is not compatible.
A few other notes:
* `to_text` was updated to let `Markup` objects pass through
unmodified, `ustr` was not updated thus.
* The `Text` fields used through `t-raw` were converted to `Html(...,
sanitized=False)` (at least when they could not be sanitized
e.g. header contents and friends).
* Digest was completely untested, but the test I added is not entirely
reliable, sometimes records go missing and I'm not really sure why.
* Running Odoo with `-b` might be a good idea: while Python 3 removed
implicit conversions between text and bytes, this means
`str(b'foo')` now generates the value `"b'foo'"`. Because `Markup`
extends `str` it has essentially the same behaviour, and passing
*bytes* to `Markup` is basically a garbage generator.