Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default file permissions so they are not world readable #70221

Merged

Conversation

samdoran
Copy link
Contributor

@samdoran samdoran commented Jun 22, 2020

SUMMARY

CVE-2020-1736

Fixes #67794

Set the default permissions for files we create with atomic_move() to 0o0600 minus the system umask. Track which files were created by atomic_move() and warn if the module supports mode and it was not specified and the module did not call set_mode_if_different(). This allows the user to take action and specify the permissions rather than using the defaults.

A code audit is needed to find all instances of modules that call atomic_move() but do not call set_mode_if_different(). The findings need to be documented in a changelog since we are not warning in this case. Warning in those instances would be frustrating to the user since they have no way to change the module code nor can they specify permissions.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/basic.py

ADDITIONAL INFORMATION

Need to complete

  • tests
  • code audit
  • document findings of code audit

@samdoran samdoran added the security Related to a vulnerability or CVE label Jun 22, 2020
@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 22, 2020
lib/ansible/module_utils/basic.py Outdated Show resolved Hide resolved
lib/ansible/module_utils/basic.py Outdated Show resolved Hide resolved
lib/ansible/module_utils/basic.py Outdated Show resolved Hide resolved
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. packaging Packaging category and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 22, 2020
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jun 23, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 23, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 24, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 7, 2020
CVE-2020-1736

Set the default permissions for files we create with atomic_move() to 0o0660. Track
which files we create that did not exist and warn if the module supports 'mode'
and it was not specified and the module did not call set_mode_if_different(). This allows the user to take action and specify a mode rather than using the defaults.

A code audit is needed to find all instances of modules that call atomic_move()
but do not call set_mode_if_different(). The findings need to be documented in
a changelog since we are not warning. Warning in those instances would be frustrating
to the user since they have no way to change the module code.
- use a set for storing list of created files
- just check the argument spac and params rather than using another property
- improve the warning message to include the default permissions
sshnaidm added a commit to containers/ansible-podman-collections that referenced this pull request Jul 26, 2020
In podman connection when we copy file to container and work as
non-root user, need to set correct ownership for the files.
Since ansible change ansible/ansible#70221
it's broken, because of new permissions ansible set to copied files.
samdoran added a commit to samdoran/ansible that referenced this pull request Jul 29, 2020
Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.
samdoran added a commit that referenced this pull request Jul 30, 2020
…70976)

Follow up to #70221
Related to #67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6
samdoran added a commit to samdoran/ansible that referenced this pull request Jul 30, 2020
…nsible#70976)

Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry picked from commit dc79528)
samdoran added a commit to samdoran/ansible that referenced this pull request Jul 30, 2020
…nsible#70976)

Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry-picked from commit dc79528)
samdoran added a commit to samdoran/ansible that referenced this pull request Jul 30, 2020
…nsible#70976)

Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry picked from commit dc79528)
nitzmahone pushed a commit that referenced this pull request Jul 30, 2020
…70976) (#70985)

Follow up to #70221
Related to #67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry picked from commit dc79528)
samdoran added a commit to samdoran/ansible that referenced this pull request Aug 3, 2020
…nsible#70976)

Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry-picked from commit dc79528)
samdoran added a commit to samdoran/ansible that referenced this pull request Aug 3, 2020
…nsible#70976)

Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry picked from commit dc79528)
relrod pushed a commit that referenced this pull request Aug 7, 2020
…adable (#70221) (#70827)

* [stable-2.8] Change default file permissions so they are not world readable (#70221)

* Change default file permissions so they are not world readable

CVE-2020-1736

Set the default permissions for files we create with atomic_move() to 0o0660. Track
which files we create that did not exist and warn if the module supports 'mode'
and it was not specified and the module did not call set_mode_if_different(). This allows the user to take action and specify a mode rather than using the defaults.

A code audit is needed to find all instances of modules that call atomic_move()
but do not call set_mode_if_different(). The findings need to be documented in
a changelog since we are not warning. Warning in those instances would be frustrating
to the user since they have no way to change the module code.

- use a set for storing list of created files
- just check the argument spac and params rather than using another property
- improve the warning message to include the default permissions.
(cherry picked from commit 5260527)

Co-authored-by: Sam Doran <sdoran@redhat.com>

* Fix service test

* Fix lamdba_policy test

* Fix aws_lamdba test

* Fix warning for new default permissions when mode is not specified (#70976)

Follow up to #70221
Related to #67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry picked from commit dc79528)

* Use new category in changelog fragments
relrod pushed a commit that referenced this pull request Aug 7, 2020
…adable (#70221) (#70825)

* [stable-2.9] Change default file permissions so they are not world readable (#70221)

* Change default file permissions so they are not world readable

CVE-2020-1736

Set the default permissions for files we create with atomic_move() to 0o0660. Track
which files we create that did not exist and warn if the module supports 'mode'
and it was not specified and the module did not call set_mode_if_different(). This allows the user to take action and specify a mode rather than using the defaults.

A code audit is needed to find all instances of modules that call atomic_move()
but do not call set_mode_if_different(). The findings need to be documented in
a changelog since we are not warning. Warning in those instances would be frustrating
to the user since they have no way to change the module code.

- use a set for storing list of created files
- just check the argument spac and params rather than using another property
- improve the warning message to include the default permissions.
(cherry picked from commit 5260527)

Co-authored-by: Sam Doran <sdoran@redhat.com>

* Fix jboss test

* Fix lamdba_policy test

* Fix aws_lamdba test

* Fix warning for new default permissions when mode is not specified (#70976)

Follow up to #70221
Related to #67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry-picked from commit dc79528)

* Use new category in changelog fragments
zoredache pushed a commit to zoredache/ansible that referenced this pull request Aug 10, 2020
…nsible#70976)

Follow up to ansible#70221
Related to ansible#67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Aug 11, 2020
* Update kolla-ansible from branch 'master'
  - Merge "Fix post-deploy mode"
  - Fix post-deploy mode
    
    Ansible changed the default mode for files, even in stable
    releases. [1]
    
    This change restores the previous default (with the common
    umask).
    
    [1] ansible/ansible#70221
    
    Change-Id: I0f81214b4f95fe8a378844745ebc77f3c43027ab
    Closes-Bug: #1891145
openstack-mirroring pushed a commit to openstack/kolla-ansible that referenced this pull request Aug 11, 2020
Ansible changed the default mode for files, even in stable
releases. [1]

This change restores the previous default (with the common
umask).

[1] ansible/ansible#70221

Change-Id: I0f81214b4f95fe8a378844745ebc77f3c43027ab
Closes-Bug: #1891145
samdoran added a commit to samdoran/ansible that referenced this pull request Aug 12, 2020
samdoran added a commit to samdoran/ansible that referenced this pull request Aug 12, 2020
samdoran added a commit to samdoran/ansible that referenced this pull request Aug 12, 2020
relrod pushed a commit that referenced this pull request Aug 12, 2020
#70221)" (#71231)

* Revert "Change default file permissions so they are not world readable (#70221)"

This reverts commit 5260527.

* Revert "Fix warning for new default permissions when mode is not specified (#70976)"

This reverts commit dc79528.
relrod pushed a commit that referenced this pull request Aug 12, 2020
relrod pushed a commit that referenced this pull request Aug 12, 2020
samdoran added a commit to samdoran/ansible that referenced this pull request Aug 12, 2020
relrod pushed a commit that referenced this pull request Aug 12, 2020
… world readable (#70221) (#70824)" (#71236)

* [stable-2.10] Revert "Fix warning for new default permissions when mode is not specified (#70976) (#70985)"

This reverts commit 5cb9608.

* [stable-2.10] Revert "Change default file permissions so they are not world readable (#70221) (#70824)"

This reverts commit 7e4cffc.
okleinschmidt pushed a commit to okleinschmidt/kolla-ansible that referenced this pull request Aug 13, 2020
Ansible changed the default mode for files, even in stable
releases. [1]

This change restores the previous default (with the common
umask).

[1] ansible/ansible#70221

Change-Id: I0f81214b4f95fe8a378844745ebc77f3c43027ab
Closes-Bug: #1891145
relrod added a commit to relrod/ansible that referenced this pull request Aug 13, 2020
relrod added a commit that referenced this pull request Aug 13, 2020
* Revert "[stable-2.10] Revert "Change default file permissions so they are not world readable (#70221) (#70824)" (#71236)"

This reverts commit c968020.

* Revert "Remove porting guide entry related to reverted change (#71242)"

This reverts commit 006a21e.
@ansible ansible locked and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. packaging Packaging category security Related to a vulnerability or CVE support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The default permissions used by atomic_move can create files that are world readable
5 participants