Solution for safe_eval security issues about payroll HOT 10 CLOSED

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?)

from payroll.

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).

from payroll.

@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.

from payroll.

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

from payroll.

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.

from payroll.

@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.

from payroll.

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.

from payroll.

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.

from payroll.

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...

from payroll.

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.

from payroll.