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
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)
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")
- 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.
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.
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.
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.
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.
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?
> 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).
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.
> 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).
> 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 `
> 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.
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?