Created on 2014-09-06.14:53:37 by duesenfranz, last changed 99 months ago by ced.
Release with fixes: 3.2.3, 3.0.7, 2.8.11, 2.6.14, 2.4.15
@matb you can ask to make CVE public.
New published so mark the issue as resolved and make it public.
The releases have been published.
So OK, I will backport both patch.
>> I think, this one is also a big step to improve the situation. What is your motivation to not apply to the series? >Because review5601002 is enough to fix the issue. But the way this works is broken by design: You can't be sure that this is safe, because * Some of the builtins given to the code  may leak globals on some obscure "path" (some attribute of an attribute of an attribute) * Future python version won't add some attributes to these builtins which might leak globals. Note that adding attributes wouldn't break backwards compatibility and therefore might even happen within a minor update.  https://github.com/tryton/trytond/blob/feeea16ec199441e558511cf74f9353260f8ae55/trytond/tools/misc.py#L376
Here is the review12571002 for the news.
On 24 Sep 11:32, Mathias Behrle wrote: > > review10521002 will be only applied on trunk after. > > I think, this one is also a big step to improve the situation. What is your motivation to not apply to the series? Because review5601002 is enough to fix the issue.
> Cédric Krier <email@example.com> added the comment: > > OK, so I propose this scheduling: > - a release of the current series + review5601002 the 29th September. > - publish the news the 30th September. Ok. > review10521002 will be only applied on trunk after. I think, this one is also a big step to improve the situation. What is your motivation to not apply to the series? At least I backported both reviews for 2.2 in Debian stable and all unit tests are working and I had no issues while testing on a 2.2 installation.
OK, so I propose this scheduling: - a release of the current series + review5601002 the 29th September. - publish the news the 30th September. review10521002 will be only applied on trunk after.
> This issue could have leaked partially to the public via the Tryton > commit channel, so directly requesting the CVE at mitre.org. > > safe_eval in trytond could be used to execute arbitrary commands, > mainly via the webdav interface. Several fixes are ready to be applied > to the supported versions and to be released in the next days. > usage of the internal safe_eval command could be used to call __subclasses__ > (double underscores). An analysis of the maintainer revealed two weak points, > where this function is used. > The first weak point was module webdav, where an authenticated user could misuse > the collection.domain. > The second weak point was module price_list, where an internal user with > adminstration privileges could misuse the formula field of > the module. > > There exist two approaches to fix/reduce the problem at its best: > - usage of literal_eval instead of safe_eval, where possible (i.e. webdav) > - disallowing double underscores in safe_eval > > We think, that one CVE should be enough with respect to the central problem: > the possible misusage of safe_eval (which in your examples is 2.). >> 2. safe_eval does not properly restrict the input data, and >> therefore allows shell metacharacters such as ; or ` > Please assign a CVE Use CVE-2014-6633.
And do we get a CVE?
> On 10 Sep 22:06, Mathias Behrle wrote: > > > So for me,we can start the process of creating a security update. > > > > Do you think, this is backportable for 2.2? > > No more supported. I know, that 2.2 is no more supported from Tryton side, but Debian supports at least stable releases for their lifetime. So I didn't ask you to do the backport, but to give a quick estimation. > > > Do you think it requires a CVE? > > > > Information leaked already via commit channel, so at least not via Debian. > > Nothing really useful was leaked. For me enough to not involve Debian. They assign only CVEs to complete non-disclosed issues. As a general guideline in Tryton I would propose to get a CVE for each bug of type security, that is handled with an embargo to the public (is handled with disclosure).
On 10 Sep 22:06, Mathias Behrle wrote: > > So for me,we can start the process of creating a security update. > > Do you think, this is backportable for 2.2? No more supported. > > Do you think it requires a CVE? > > Information leaked already via commit channel, so at least not via Debian. Nothing really useful was leaked.
Reviews are generally looking fine for me. > So for me,we can start the process of creating a security update. Do you think, this is backportable for 2.2? > Do you think it requires a CVE? Information leaked already via commit channel, so at least not via Debian. But generally it is always nice to assign CVEs, so let me know, if I should try to get one via MITRE Corporation (firstname.lastname@example.org).
Could you please grant me permission for the reviews. Thanks.
Here is the review5601002 that prevents double underscore. I run all the test on it without issue. So for me,we can start the process of creating a security update. Do you think it requires a CVE?
I don't know how to handle upload.py and generally your infrastructure, therefore I would appreciate you to handle this. The patch is already attached here and can be used as-is.
Why not, could you make a private review.
Yes, you can still create strings that include double underscores. But since you can't use __getattribute__ or __setattribute__, these strings are rather worthless. So can even access these attributes with str.format, but then you only have their string representation. The problem for me is that we still trust every usable class (list, str, int, float, etc.) that they don't "leak" internals, but it's much more unlikely for every of them to leak them with attributes that don't include double underscores. So while still unsafe (because basically impossible to verify as "safe"), it would be a lot harder to exploit.
This doesn't fix the problem because you can still trick the test like you did with something like '_' + '_' + 'subclassess' + '_' + '_'.
To clarify: I just wanted to disallow eval'ing any string that contains a double underscore - as we already do with the string "__subclasses__". Since I don't really know the use cases, I also don't know wether this could be a problem. If there are cases where this doesn't work, maybe we could introduce a new optional argument "allow_double_underscores=True" that gets set to False in most uses.
If you know how to do it, please provide a path.
Is there any sane reason to use double underscore for things that get safe_eval'd? While not fixing the underlying problem, disallowing them would make something like that a magnitude harder.
I removed some more safe_eval usages in other places.
The review is private, I can give access on request.
I got this fix for webdav, review10521002
Sorry for the spam again, reuploaded without the unnecessary commands
To clarify: this file doesn't do anything bad, just uses echo
For the record, safe_eval is used: - convert.py for XML evaluation (no security issue) - action.py for some field evaluation (by default only admin has write access) * - cron.py: idem * - lang.py: idem * - model.py only for migration (no security issue after migration) - rule.py: idem * - trigger.py: idem * - view.py: idem * - modelview.py: same as for view.py - currency.py: idem * - price_list.py: write access to product admin user - webdav.py: any user has write access So for me, there are 2 weak points price_list and webdav.
On 06 Sep 16:34, duesenfranz wrote: > That said, calling it "not 100% safe" is an understatement, its not safe at all. (see "executing arbitrary commands is also possible") Proof?
I agree that getting rid of safe_eval is the best solution by far. That said, calling it "not 100% safe" is an understatement, its not safe at all. (see "executing arbitrary commands is also possible") I can't tell which code is really run, but authenticated tryton users being able to own the server is an issue (in my opinion)
Also the best way will be to get ride safe_eval which is partially started in issue3211.
For me, it is not a big issue because: - it is known that safe_eval is not 100% safe - safe_eval is normaly used on code from autheticated users (and so trusted)
sorry for the spam: ban double underscores is probably easier
a quick & dirty fix would be to ban underscores as a whole. While you can create strings with underscores, I think you can't use __getattr__ or similar and therefore can't use your string with underscores. Note that this would be an easy fix and is probably breakable with a few hours time investment
executing arbitrary commands is also possible
|2014-10-03 11:17:44||yangoon||link||issue4228 superseder|
|2014-09-30 12:45:01||ced||set||status: chatting -> resolved|
messages: + msg18388
|2014-09-30 12:11:47||ced||set||status: resolved -> chatting|
messages: + msg18387
|2014-09-30 12:07:54||ced||set||status: in-progress -> resolved|
messages: + msg18386
|2014-09-29 18:29:39||ced||set||messages: + msg18372|
|2014-09-25 14:50:53||duesenfranz||set||messages: + msg18348|
|2014-09-25 13:03:40||ced||set||messages: + msg18346|
|2014-09-25 12:02:55||duesenfranz||set||messages: + msg18344|
|2014-09-24 19:45:20||ced||set||reviews: 10521002,5601002 -> 10521002,5601002,12571002|
messages: + msg18330
|2014-09-24 11:49:22||ced||set||messages: + msg18311|
Showing 10 items. Show all history (warning: this could be VERY long)