Tryton - Issues

 

Issue4155

Title CVE-2014-6633: safe_eval is, in fact, not safe
Priority bug Status resolved
Superseder Nosy List ajacoutot, bch, ced, daniel, duesenfranz, nicoe, sharkcz, yangoon
Type security Components trytond
Assigned To ced Keywords review
Reviews 10521002,5601002,12571002
View: 10521002, 5601002, 12571002

Created on 2014-09-06.14:53:37 by duesenfranz, last changed by ced.

Files
File name Uploaded Type Edit Remove
crash_trytond.py duesenfranz, 2014-09-06.14:53:35 text/plain
disallow_underscore.patch duesenfranz, 2014-09-07.00:39:20 text/plain
run_anythin_without_noise.py duesenfranz, 2014-09-06.18:19:27 text/plain
run_anything.py duesenfranz, 2014-09-06.18:12:21 text/plain
Messages
msg18388 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-30.12:45:00
Release with fixes: 3.2.3, 3.0.7, 2.8.11, 2.6.14, 2.4.15
msg18387 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-30.12:11:46
@matb you can ask to make CVE public.
msg18386 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-30.12:07:53
New published so mark the issue as resolved and make it public.
msg18372 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-29.18:29:38
The releases have been published.
msg18348 (view) Author: [hidden] (duesenfranz) Date: 2014-09-25.14:50:52
Thanks! :)
msg18346 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-25.13:03:39
So OK, I will backport both patch.
msg18344 (view) Author: [hidden] (duesenfranz) Date: 2014-09-25.12:02:55
>> 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 [0] 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.

[0] https://github.com/tryton/trytond/blob/feeea16ec199441e558511cf74f9353260f8ae55/trytond/tools/misc.py#L376
msg18330 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-24.19:45:19
Here is the review12571002 for the news.
msg18311 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-24.11:49:21
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.
msg18308 (view) Author: [hidden] (yangoon) (Tryton translator) Date: 2014-09-24.11:32:36
> C├ędric Krier <cedric.krier@b2ck.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.
msg18303 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-24.08:53:13
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.
msg18276 (view) Author: [hidden] (yangoon) (Tryton translator) Date: 2014-09-22.20:34:45
> 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.
msg18195 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-17.22:25:26
And do we get a CVE?
msg18025 (view) Author: [hidden] (yangoon) (Tryton translator) Date: 2014-09-11.11:06:10
> 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).
msg18022 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-11.00:30:21
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.
msg18021 (view) Author: [hidden] (yangoon) (Tryton translator) Date: 2014-09-10.22:06:25
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 (cve-assign@mitre.org).
msg18017 (view) Author: [hidden] (yangoon) (Tryton translator) Date: 2014-09-10.19:19:27
Could you please grant me permission for the reviews. Thanks.
msg18016 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-10.18:48:35
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?
msg17962 (view) Author: [hidden] (duesenfranz) Date: 2014-09-07.21:47:13
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.
msg17961 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-07.16:23:03
Why not, could you make a private review.
msg17954 (view) Author: [hidden] (duesenfranz) Date: 2014-09-07.10:41:02
"internals"->"built-ins"
msg17953 (view) Author: [hidden] (duesenfranz) Date: 2014-09-07.10:32:34
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.
msg17951 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-07.09:39:24
This doesn't fix the problem because you can still trick the test like you did with something like '_' + '_' + 'subclassess' + '_' + '_'.
msg17950 (view) Author: [hidden] (duesenfranz) Date: 2014-09-07.00:39:20
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.
msg17949 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.23:46:22
If you know how to do it, please provide a path.
msg17948 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.23:30:29
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.
msg17947 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.22:14:25
I removed some more safe_eval usages in other places.
msg17946 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.20:00:10
The review is private, I can give access on request.
msg17945 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.19:59:02
I got this fix for webdav, review10521002
msg17944 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.18:19:27
Sorry for the spam again, reuploaded without the unnecessary commands
msg17943 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.18:13:32
To clarify: this file doesn't do anything bad, just uses echo
msg17942 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.18:12:21
Proof! :)
msg17941 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.18:12:04
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.
msg17940 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.17:57:56
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?
msg17939 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.16:34:02
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)
msg17938 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.16:30:11
Also the best way will be to get ride safe_eval which is partially started in issue3211.
msg17937 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-09-06.16:28:28
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)
msg17936 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.15:32:02
sorry for the spam: ban double underscores is probably easier
msg17935 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.15:30:02
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
msg17934 (view) Author: [hidden] (duesenfranz) Date: 2014-09-06.14:57:06
executing arbitrary commands is also possible
History
Date User Action Args
2014-10-03 11:17:44yangoonlinkissue4228 superseder
2014-09-30 12:45:01cedsetstatus: chatting -> resolved
messages: + msg18388
2014-09-30 12:11:47cedsetstatus: resolved -> chatting
messages: + msg18387
2014-09-30 12:07:54cedsetstatus: in-progress -> resolved
messages: + msg18386
2014-09-29 18:29:39cedsetmessages: + msg18372
2014-09-25 14:50:53duesenfranzsetmessages: + msg18348
2014-09-25 13:03:40cedsetmessages: + msg18346
2014-09-25 12:02:55duesenfranzsetmessages: + msg18344
2014-09-24 19:45:20cedsetreviews: 10521002,5601002 -> 10521002,5601002,12571002
messages: + msg18330
2014-09-24 11:49:22cedsetmessages: + msg18311

Showing 10 items. Show all history (warning: this could be VERY long)