[14.0] [IMP] payroll: improve security

[nimarosa]

Hello, this commit purpose is to improve security rules and add more restriction specially to editing python rules.

I will be making another tweaks to it in the following days, for now the changes are:

• Created a new group, which is a hidden one (can only be activated in the user form, while being in developer mode and with a checkbox)
• Users with this group, will be the only who can see the "python code" fields.
• Other users, including the payroll manager, will see an error message it's shown in the following screenshot:

This is to make the user understand why he can't see the python code field, and don't believe it's a bug.
I have the intention of doing some tweaks to the other existing groups, so we can have very detailed and extensive security rules and groups, that could prevent the most any exploit of python safe_eval functionality. We will achieve that by limiting the roles, specially with all python related stuff, but not limited to.

Keep posted for more commits.

[OCA-git-bot]

Hi @appstogrow,
some modules you are maintaining are being modified, check this out!

[norlinhenrik]

Wow, I didn't see this UI tweak before.

The security setting says "Payroll: Allow editing python code in salary rules", but it also applies for reading. Either let the user read the code, or change the text, e.g. "Payroll: Show python code in salary rules" or "Payroll: Access to python code in salary rules".

I think the big pink box is too much of a warning. And "You are NOT allowed" might be discouraging to the user. I would replace all the text with a simple sentence:
"For security reasons, access to [or: editing] python code is restricted."
I don't mind including the last sentence: "If you think there is an error, please ask your system administrator." (add "please" and fix "adminsitrator").

[nimarosa]

@norlinhenrik

Wow, I didn't see this UI tweak before.

It's very good, I use it a lot to show messages. It can also be warning and info decoration.

The security setting says "Payroll: Allow editing python code in salary rules", but it also applies for reading. Either let the user read the code, or change the text, e.g. "Payroll: Show python code in salary rules" or "Payroll: Access to python code in salary rules".

Great, yes, I was thinking about changing the tests. So gonna accept the suggestion.

I think the big pink box is too much of a warning. And "You are NOT allowed" might be discouraging to the user. I would replace all the text with a simple sentence:

"For security reasons, access to [or: editing] python code is restricted."

I don't mind including the last sentence: "If you think there is an error, please ask your system administrator." (add "please" and fix "adminsitrator").

Great will make the changes. I can also make the box be yellow using the warning class. What do you think? It can be blue if I change it for info decoration.

[norlinhenrik]

I think this is information, not a warning.

[norlinhenrik]

@mtelahun Any opinions?

[nimarosa]

I think this is information, not a warning.

Okay so a info will be. Info box it's light blue. Will change it and post screenshot to make more easy to us to decide.

[mtelahun]

Hello @nimarosa, Thank you for starting this PR and proposing a solution. However, what you are suggesting is what people in the security industry call "security through obscurity". That means trying to make something more secure by hiding it. This is a practice that is not recommended and is discouraged as a security measure because it doesn't provided any meaningful security. With respect to payroll rules the security problem we have is not that it is easy to see or edit. The problem is that it allows salary rules to run python code against the odoo ORM without any restrictions. Simply making administrators run through hoops to administer their system just pisses them off and makes it harder for them to do their jobs without providing any meaningful obstacle to adversaries. So, I am not in favor of this type of security. As I mentioned before, short of introducing a domain specific language for payroll rules I don't know if there is any easy solution. This requires some careful thought and consideration and possibly some security people we can bounce our ideas off of. Regards, Mike. Message ID: ***@***.***>

[mtelahun]

@mtelahun Any opinions?

Hi @apps2grow ,
Please see my reply to @nimarosa. I appreciate the enthusiasm and the willingness to tackle this problem, but it will require careful consideration and the input of security people if we want to do it right.

[nimarosa]

@mtelahun yes, I know this don't fix the security problem which is safe_eval like. I think the only solution is get rid of it writing a new language like I stated in issue #94. It will be great to start discussing it there so we can start working in it in a new branch dedicated to build that feature.

About this PR, I understand your point of view but I really think limiting the access to only specific users allowed to write python is indeed good for reducing the risks.

Of course, an experimented hacker will always find the way to hack the system, because safe_eval is not safe at all. But this change will prevent a normal user at least to edit the rules to make harmful changes to the system. I see it not as a solution but a palliative.

For now, the only way to make safe_eval safe, is to trust user input, so I think limiting the functionality to only specific allowed users (and not the whole payroll manager group which can be composed by lot of users). With this change, you can specifically allow only trusted users to edit rules, or even you can allow it only for making changes and disallowing it after.

I know it's not really a security improvement but I think is positive to have it, even if it's requires more work from system administrator. The reality is rules don't change every day, so when all rules are created it's you will only change it if you need to adapt it to a new case o change something in it.

Also this continue allowing payroll managers to use and edit the standard functions in rules, it only limit python fields.

Finally, the PR is not finished but apart of hiding the field I'm planning of checking security group also in save and update methods, so if anyone manages to see the field without having the security group, it will not be allowed to save the form anyways.

Finally, I think it's not too much hassle to turn on a checkbox in the user to allow the user to edit the rule. How many people really in a payroll or hr department edit rules on daily basis? In my experience it's only a few people in the businesses that create and code rules, so it makes sense to not allow it to all "normal" users.

Like I said, it's not perfect and I accept more suggestions to this PR. But I think is necessary to implement something like this at least until we can replace safe_eval completely, but it's a large project and we should provide more security to payroll users.

[nimarosa]

The whole point of this is not fighting an experienced hacker. An experienced hacker will always found a way to crack the system and also not only in this module. We have other models in odoo that allow to write code with safe_eval.

The purpose of it is to reduce the amount of possible treats from normal users, or from someone in the business who know a bit of python and can exploit it.

Like I said, it didn't add security, but I think it reduce the chances that every payroll user could exploit it. For normal users, if they can't see it or edit it, they will desist in exploiting it. So I think it's more secure that allowing all payroll managers security group to access it.

[nimarosa]

Changes in last commits:

• Accepted suggestion by @norlinhenrik. Now the "error" is a informational one, and made some tweaks to make it look good.

Now it looks like this:

@norlinhenrik What do you think?

[nimarosa]

/ocabot rebase

[OCA-git-bot]

Congratulations, PR rebased to 14.0.

[mamcode]

Please make squash of commits