Solution for safe_eval security issues

[nimarosa]

Hello, motivated by the chat we were having in #87 I open this issue to discuss possible solutions to safe_eval lack of security so we can stop exposing all python environment in a salary rule.

The main issue here is: safe_eval is not safe at all. It exposes real environment python objects and allows the user to run almost everything from a salary rule.

So I come up with 3 ideas that I think if we apply at least one of them it will improve security in the module rules:

1. We should write more standard options of calculations, so we can move the python mode to another module to be used only when necessary.

2. We modify safe_eval to not allowing write access to the ORM. Actually don't know how difficult this can be, but I think is an option.

3. We create a new salary rule coding language. It would involve a whole module change, but for me, is the more promising one, and I have some resources you can check to get an idea:

In python we have some libraries and derivates projects called "Python String Parsers" that in resume provide a whole package that parse strings and convert them to encapsulated python code (so it didn't expose the environment) and return the result of the operation. This packages will solve the problem since we will not write python code in the rule, we will be writing strings and functions from the parser library and the the package will take care of interpreting and processing the sting code, not exposing in any way the python environment.

For you to read I leave you some resources about it:

• ply: Lexx/yacc library. It could provide us with a internal language from rules. In the link you can also see projects writed with ply that could be useful for providing us functions to use in rules.
• pyparsing: it's a big module with all sort of functions and use cases. It can also provide us a string-like programming language to use in rules.

Also I leave some interesting article about eval safety issues. That is a problem with eval and safe_eval so that's what we should attack. Eval is really dangerous

---

So in resume I think we should take a look to all options. Also, and in a way of providing temporary security I will work in @norlinhenrik approach of locking the salary rule form from only a few cases or maybe a setting. So only trusted odoo users can edit rules.

Also I think #87 should be merged with the new payslip object fix, because adding it don't increment current security issues of salary rules, it's just another object and since we have access to all env in rules, I think we can tolerate this change. We should attack security issues from the root, which is python safe_eval not the objects.

Leave this thread to discuss with you @appstogrow @mtelahun , I can also create a new branch to work in this if you are willing to help me with this, since it's a big change and I don't have time to do it alone, I have the ideas so I think we can make a good team.. Let me know what you think and I can create a 14.0-new-rule-language branch and we can work in there until everything is ready.

[norlinhenrik]

In September, I wrote to Odoo regarding safe_eval. Here is part of the response:

What the sandbox is designed to block are things that a database admin cannot do: accessing arbitrary files on the server, executing arbitrary Python code, connecting to other databases, etc.

I am confident that safe_eval safely blocks what it is designed to block. (Here is a link to the Odoo Security page.)

If we would implement another language, we would still have the almost impossible task of safely evaluating untrusted code.

To import account statements and then run specific actions, ["set_text", "set_partner", "time_parameter"] will trigger

• _excel_post_import_set_text()
• _excel_post_import_set_partner()
• _excel_post_import_time_parameter()

Here is the hook that will read the list and call the methods. Maybe we could write a similar payroll hook and generic methods. The salary rule configuration could have a one2many relation to a new model with these fields:

• sequence
• method (select from a list)
• arguments to pass to the method (string -> safe_eval?)

[norlinhenrik]

But I think that python without "env" is safe. Can we try this?

1 Remove from the BrowsableObject:

        self.employee_id = employee_id
        self.env = env

2 Replace contract/employee/payslip with browsable objects in the localdict.

3 Re-implement necessary methods in the browsable objects (like the former payslip.rule_parameter).

[nimarosa]

@norlinhenrik Hello Henrik, thanks for the research. Very useful.

I have some points to remark:

• As odoo said to you, safe_eval limit what a database administrator can't do. That's true, with safe eval you can't run code and get to the shell of the server (something that eval() allow). But the problem is that database administrator privileges are too broad and allow to modify things in the system ORM. Like for example, running python code that deletes/creates invoices. Any code from salary rules is run as an administrator.

• The problem of removing env, is that without env, browsable objects will no longer work. Because they use env to get object fields. Without env, we will lose support for accessing model fields.

• Replacing contract/employee/payslip object with a browsable object it's a possible improvement but we will have the problem that we only have access to stored fields. Employee and contract has a lot of fields that are computed, also, these models have helper functions that retrieve useful data like work schedules and worked hours computing helper methods. So if we do this, we lose the possibility to access them.

• Reimplementing necessary methods in browsable objects is possible, but we have the problem that we can't inherit them, so other modules will not have the possibility to plug in features. Anyways I think we can reinvent browsable methods and make them inheritable, but I have to think how.

About implementing another language, the way I see it, this parser framework eval everything in their own functions. Even a sum or division is evaluated by a method, it never gets evaluated in python env with access to database cursor. But I also think it's a long feature to develop and will require rewrite all the module, so: I like the idea, but I don't know how much time could take us to do it.

I will continue searching for ideas and also waiting for yours. Maybe we can find a way to do all of this without affecting the functionalities too much.

The other option is to work directly in implementing the more standard calculation options we can, so we can make python a optional module like you suggested. But this also requires que lot of work because we have to think in the most use cases posible and develop a way to do it with fields like percentage option does. And some of them are not so easy to do that way.

Other thing is that standard calculation functions are also calculated with safe_eval so the problem still there but it's more difficult and less straightforward to hack.

Thanks for your input Henrik, I will create today a 14.0 dev branch in which we can test all our approaches and merge beta code there.

[nimarosa]

Guys, i just created the branch 14.0-dev_safe_eval_replacement in which we can start trying and testing new approaches. You can start making PRs there and we will merge them fast so we all can test them. It will not be a fast process anyways, so we can start working on it on our free time. Also the discussion here i think will be rich in ideas.

https://github.com/OCA/payroll/tree/14.0-dev-safe_eval_replacement

[norlinhenrik]

Any code from salary rules is run as an administrator.

No, safe_eval will evaulate with the permissions of the user. I just tested payslip.env["res.partner"].create({"name": "Name"}). If Contact Creation is not enabled on the user, safe_eval will return AccessError.

Without env, we will lose support for accessing model fields.

Maybe a new method could read the fields and return the values to BrowsableObject?

we will have the problem that we only have access to stored fields

Not if the new method will read also the computed fields.

work schedules and worked hours computing helper methods

Maybe those values could be computed and set in localdict before running safe_eval? Then they would be available in python.

I think we can reinvent browsable methods and make them inheritable

Maybe the same way Odoo base classes are modified to include inherited model classes.

[nimarosa]

@pedrobaeza Hi Pedro, I don't know if you are informed about safe_eval risks but we are working in a new idea to in the future replace/modify it in to improve security in payroll.

It will be a long process to do it but your ideas and opinions will be very helpful if you have any. The idea is to discuss options.

If you have time to brainstorm here with us it will be very great.

[pedrobaeza]

As said, safe_eval can be evaluated for the current user, so it's safe enough to expand current user scope. If we want to avoid any possible DB change, the way to work is to:

• Add a savepoint before calling safe_eval.
• Call safe_eval.
• Rollback to the savepoint after.

With that, I still see safe_eval safe enough. Another thing, but more for UX, is to start to add direct salary rules that implement the usual things you need.

[nimarosa]

Another thing, but more for UX, is to start to add direct salary rules that implement the usual things you need.

Yes @pedrobaeza that is one of the ideas i had. That will make more easy to write salary rules and don't have to do it with python code.

The other thing we are working now is restricting more the access to edit the code only to trusted users. That should add more security since we will trust the code because user will have to have an specific user-level permission to write the rules.

Thanks for your input Pedro.

[norlinhenrik]

Another idea is to make a "readonly" mode for executing salary rule codes. Then safe_eval can read everything that the user can access, but not change anything. I have no idea how to implement it though...

[github-actions]

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.