Replace atomic_move() with a higher level function

[samdoran]

SUMMARY

CVE-2020-1736

Related to #70221, #71200
Fixes #67794

The original fix for this CVE which changed the default permissions to 600 unless mode was explicitly specified in the task was too disruptive. This is an attempt at a better solution, but it will be a more invasive change to the code.

The CVE has two main problems that need to be addressed:

1. Default permissions of temporary files
2. Permissions of newly created files

The previous fix addressed the first item, but did not adequately address the second item. Asking that tasks explcitly specify mode was too much of a change requirement. Furthermore, it resulted in overwhelming surprise from the community to find that in the absense of mode the default umask of the system was not used. That’s a sign it was not a good fix.

Here is my plan for how to fix this:

• deprecate atomic_move()
• rewrite atomic_move() so it is easier to read and maintain
• create a higher level function that calls the new move_file() method as well as set_mode_if_different()

The main reasoning behind this is that atomic_move() is not a good API. It has a very strong, but unenforced, relationship to set_fs_attributes_if_different() and it lacks information about the intended final permissions of the file.

In order to ensure that temporary files are created securely, and that they are never set to a mode greater than the mode specified in the task, if present, the new method needs to know about the final intended permissions, not just the system defaults.

The new method will accept src, dest, and mode at a minimum. It may accept the same file_args that set_fs_attributes_if_different() accepts, but I am not sure at this time.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

lib/ansible/module_utils/basic.py

ADDITIONAL INFORMATION

In its current state, I am in the process of writing move_file() as a function rather than (yet another) method of AnsibleModule. Ultimately, I believe this approach will be too disruptive and will have to be done at a later date. There are many methods, particularly run_command() that will require a lot of work to remove from AnsibleModule.

I will most likely end up creating new methods so this change may be backported, then do the work of moving those methods to functions. Apologies to my future self. 😄

[samdoran]

bot_skip