zfs: fix multi-line value in user-defined property

[batrla]

SUMMARY

Fixes ZFS module which breaks if ZFS dataset property contains multi-line value.
The root cause is a bug in zfs.py that causes parsing error of 'zfs get' output. The ZFS module uses 'zfs get -H -p -o property,value,source all dataset' to retrieve from dataset list of its property names, their values and source (whether the property was set locally or inherited for example). However, the problem is the output in value field can contain control characters like \n or \t. This causes ambiguity since the same characters are used to separate properties and fields on a line. The output of 'zfs get all' is basically not parseable as it contains values of multiple properties. One needs to use 'zfs get individual_property' to get single property value to avoid ambiguity.

Test case:

- name: basic configuration
  hosts: localhost

  tasks:
  - name: create ZFS with user defined property and multiline value
    zfs:
      name: rpool/myfs
      state: present
      extra_zfs_properties:
        my.custom.property:blah: "one\ntwo\n"

  - name: update user defined property multiline value
    zfs:
      name: rpool/myfs
      state: present
      extra_zfs_properties:
        my.custom.property:blah: "one\ntwo\nthree\nfour\n"

Without the fix

TASK [create ZFS with user defined property and multiline value] ***************

The full traceback is:
Traceback (most recent call last):

[ ... striping lines to make boring error output shorter ... ]

 line 295, in <module>\n  File \"/tmp/ansible_zfs_payload_xteg4tbc/ansible_zfs_payload.zip/ansible_collections/community/general/plugins/modules/zfs.py\", line 274, in main\n  File \"/tmp/ansible_zfs_payload_xteg4tbc/ansible_zfs_payload.zip/ansible_collections/community/general/plugins/modules/zfs.py\", line 196, in set_properties_if_changed\n  File \"/tmp/ansible_zfs_payload_xteg4tbc/ansible_zfs_payload.zip/ansible_collections/community/general/plugins/modules/zfs.py\", line 224, in get_current_properties\nValueError: not enough values to unpack (expected 3, got 2)\n",
    "module_stdout": "",
    "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error",
    "rc": 1
}

With the fix

TASK [create ZFS with user defined property and multiline value] ***************
ok: [localhost] => {
    "changed": false,
    "diff": {
        "after": {
            "extra_zfs_properties": {
                "my.custom.property:blah": "one\ntwo\n"
            }
        },
        "after_header": "rpool/myfs",
        "before": {
            "extra_zfs_properties": {
                "my.custom.property:blah": "one\ntwo\nthree\nfour\n"
            }

^ this is because of test case re-run. The ZFS dataset initially had property value of  "one\ntwo\nthree\nfour\n"

        },
        "before_header": "rpool/myfs"
    },
    "invocation": {
        "module_args": {
            "extra_zfs_properties": {
                "my.custom.property:blah": "one\ntwo\n"
            },
            "name": "rpool/myfs",
            "origin": null,
            "state": "present"
        }
    },
    "my.custom.property:blah": "one\ntwo\n",
    "name": "rpool/myfs",
    "state": "present"
}

TASK [update user defined property multiline value] ****************************

ok: [localhost] => {
    "changed": false,
    "diff": {
        "after": {
            "extra_zfs_properties": {
                "my.custom.property:blah": "one\ntwo\nthree\nfour\n"
            }
        },
        "after_header": "rpool/myfs",
        "before": {
            "extra_zfs_properties": {
                "my.custom.property:blah": "one\ntwo\n"
            }
        },
        "before_header": "rpool/myfs"
    },
    "invocation": {
        "module_args": {
            "extra_zfs_properties": {
                "my.custom.property:blah": "one\ntwo\nthree\nfour\n"
            },
            "name": "rpool/myfs",
            "origin": null,
            "state": "present"
        }
    },
    "my.custom.property:blah": "one\ntwo\nthree\nfour\n",
    "name": "rpool/myfs",
    "state": "present"
}

Result

When test case with fix completes, multi-line ZFS property is set:

$ zfs get all rpool/myfs | tail
rpool/myfs  version                  6                      -
rpool/myfs  vscan                    off                    default
rpool/myfs  writelimit               default                default
rpool/myfs  xattr                    on                     default
rpool/myfs  zoned                    off                    default
rpool/myfs  my.custom.property:blah  one
two
three
four
    local

[ansibullbot]

cc @bcoca @fishman @jasperla @johanwiren @jpdasma @mator @scathatheworm @troy2914 @xen0l
click here for bot help

[felixfontein]

Thanks for your contribution. Could you please add a changelog fragment? Thanks.

[batrla]

Sure, np, just added a changelog fragment. I just didn't know about it.

[github-actions]

Docs Build 📝

Thank you for contribution!✨

The docsite for this PR is available for download as an artifact from this run:
https://github.com/ansible-collections/community.general/actions/runs/4563994550

File changes:

• M collections/community/general/pipx_module.html

Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/community.general/community.general/docsbuild/base/collections/community/general/pipx_module.html b/home/runner/work/community.general/community.general/docsbuild/head/collections/community/general/pipx_module.html
index 49e81e6..27f06b7 100644
--- a/home/runner/work/community.general/community.general/docsbuild/base/collections/community/general/pipx_module.html
+++ b/home/runner/work/community.general/community.general/docsbuild/head/collections/community/general/pipx_module.html
@@ -195,7 +195,7 @@
 <a class="ansibleOptionLink" href="#parameter-force" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Force modification of the application’s virtual environment. See <code class="docutils literal notranslate"><span class="pre">pipx</span></code> for details.</p>
-<p>Only used when <em>state=install</em>, <em>state=upgrade</em>, <em>state=upgrade_all</em>, or <em>state=inject</em>.</p>
+<p>Only used when <em>state=install</em>, <em>state=upgrade</em>, <em>state=upgrade_all</em>, <em>state=latest</em>, or <em>state=inject</em>.</p>
 <p class="ansible-option-line"><span class="ansible-option-choices">Choices:</span></p>
 <ul class="simple">
 <li><p><code class="ansible-option-default-bold docutils literal notranslate"><span class="pre">false</span></code> <span class="ansible-option-choices-default-mark">← (default)</span></p></li>
@@ -208,7 +208,8 @@
 <a class="ansibleOptionLink" href="#parameter-include_injected" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Upgrade the injected packages along with the application.</p>
-<p>Only used when <em>state=upgrade</em> or <em>state=upgrade_all</em>.</p>
+<p>Only used when <em>state=upgrade</em>, <em>state=upgrade_all</em>, or <em>state=latest</em>.</p>
+<p>This is used with <em>state=upgrade</em> and <em>state=latest</em> since community.general 6.6.0.</p>
 <p class="ansible-option-line"><span class="ansible-option-choices">Choices:</span></p>
 <ul class="simple">
 <li><p><code class="ansible-option-default-bold docutils literal notranslate"><span class="pre">false</span></code> <span class="ansible-option-choices-default-mark">← (default)</span></p></li>
@@ -221,7 +222,7 @@
 <a class="ansibleOptionLink" href="#parameter-index_url" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Base URL of Python Package Index.</p>
-<p>Only used when <em>state=install</em>, <em>state=upgrade</em>, or <em>state=inject</em>.</p>
+<p>Only used when <em>state=install</em>, <em>state=upgrade</em>, <em>state=latest</em>, or <em>state=inject</em>.</p>
 </div></td>
 </tr>
 <tr class="row-odd"><td><div class="ansible-option-cell">
@@ -251,7 +252,7 @@
 <a class="ansibleOptionLink" href="#parameter-install_deps" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Include applications of dependent packages.</p>
-<p>Only used when <em>state=install</em>, <em>state=upgrade</em>, or <em>state=inject</em>.</p>
+<p>Only used when <em>state=install</em>, <em>state=latest</em>, <em>state=upgrade</em>, or <em>state=inject</em>.</p>
 <p class="ansible-option-line"><span class="ansible-option-choices">Choices:</span></p>
 <ul class="simple">
 <li><p><code class="ansible-option-default-bold docutils literal notranslate"><span class="pre">false</span></code> <span class="ansible-option-choices-default-mark">← (default)</span></p></li>
@@ -279,7 +280,7 @@
 <a class="ansibleOptionLink" href="#parameter-python" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Python version to be used when creating the application virtual environment. Must be 3.6+.</p>
-<p>Only used when <em>state=install</em>, <em>state=reinstall</em>, or <em>state=reinstall_all</em>.</p>
+<p>Only used when <em>state=install</em>, <em>state=latest</em>, <em>state=reinstall</em>, or <em>state=reinstall_all</em>.</p>
 </div></td>
 </tr>
 <tr class="row-odd"><td><div class="ansible-option-cell">

[felixfontein]

CC @froebela @sam-lunt @hobnob11 you did fixes to this modules in the past, maybe you could take a look at this one.

[sam-lunt]

Personally, I don't think this is a change that should be made, for two reasons.

First, it's going to be much slower. Currently, there is one call to zfs get: zfs get -H -p -o property,value,source, which retrieves the list of properties as well as their values. With this change, there will be N + 1 calls to zfs get: one to get the list of properties, then one call per property to retrieve the value. This is clearly less efficient and introduces some potential corner cases if the properties are somehow modified concurrently by another process.

However, the bigger issue is that this is supporting a use case that I don't think that ZFS really intends to support, namely property names or values that contain special characters \t and/or \n. Quoting from the zfs-get man page:

-H
Display output in a form more easily parsed by scripts. Any headers are omitted, and fields are explicitly separated by a single tab instead of an arbitrary amount of space.

The fact that fields are tab-delimited and records are newline-delimited pretty clearly indicates to me that the ZFS maintainers don't intend or expect property names/values to include those characters. I don't think it makes sense to increase code complexity and worsen performance just to support a use case that almost no one uses and that the ZFS maintainers don't seem to intentionally support.

@batrla, you don't mention the use case you have that requires embedding newlines in your property values, but I'd suggest trying to refactor your logic to avoid literal tabs or newlines in your property names/values. (Python makes this easy using the str.encode("unicode_escape") feature, for example).

[batrla]

Hi @sam-lunt and thanks for having look at the code.

Current parsing in zfs.py is naive and evil administrator or common user (also evil) with ZFS delegated admin privileges can easily exploit it. The scope of problem I am trying to fix goes beyond user-defined properties and impacts property that allows path names, like e.g. the mountpoint property, where traditionally only \0 and '/' characters are not allowed. The mountpoint property provides whole range of options how to break zfs.py, either to break parsing by containing \t or \n in mountpoint value or by containing fake lines that look like output of zfs get to confuse zfs.py to think that property X has fake value of Y.

This is just an example that breaks current zfs.py:

zfs create -o mountpoint=$'/this\nbreaks\nansible' rpool/pokus

No user-defined property needed.

Yes, the suggested fix is less efficient as it has calls initially zfs get to find out all property names. Then it calls another zfs get to find value of a property, but that call is only done for modified properties (in playbook), so time complexity falls into O(n) range, where n is number of properties. I think it's reasonable, since zfs get single_prop is the only way to avoid ambiguity in parsing.

To your point of zfs man page, yes, I had look at it before. The description of -H and -p options is unfortunate, because it sets expectations and fails to deliver it. What zfs get is missing is quoting of control characters and characters that are used as separators in the output, but that's something we'll have to live with, since even if someone fixes it later, the history won't be rewritten (there would be still systems with old output format).

I don't think we have a luxury of not accepting this PR.

[batrla]

I'm sorry for typo, I wrote '/' is not allowed in mountpoint, of course mountpoint property allows '/' by definition... I was thinking about a file name (path component) when writing it.

[sam-lunt]

OK, I hadn't realized this was meant to address a security concern. That's helpful context to have, thanks. That said, I don't think this is a particularly compelling concern, since the only way to write ZFS properties is to have root access (or to be granted them via zfs-delegate, as you mentioned, but I think anyone using that feature is aware that it's not something that should be done with an untrusted user).

Since the only way to construct a malicious property value requires elevated privileges, I don't think it's something worth worrying about. It's analogous to worrying about a malicious /root/.bashrc file: if someone malicious can write that file, then they won't bother to do so, since they can just directly do whatever actively malicious task they want

[sam-lunt]

Also, while Unix/Linux allow any character in a file path besides \0 and /, that's generally viewed as a too-lenient restriction and many common *nix utilities are more strict (e.g. entries of PATH cannot have : characters)

[batrla]

Well, I can't agree with statement:

It's analogous to worrying about a malicious /root/.bashrc

ZFS delegated admin can add privileges to unprivileged users to set user properties on filesystems they create, but have no permissions to touch datasets of other users. This is can be done recursively and even on a public machine. The administrator (super-user) might think that he securely allowed ZFS delegated admin permissions and had no knowledge malicious user could break his ansible stuff running on some other host... It's not like /root/.bashrc case. Instead it's more like /tmp directory with a+rwxt permission where user can store his files but cannot touch / remove other's files. Then an unprivileged user creates some 'weird files' in /tmp directory and waits if some script run by root traversing /tmp is buggy or not. Whether you want to address this parsing bug in zfs.py in community.general is something I don't know...

[sam-lunt]

Do you have an example of this kind of exploit being used/attempted "in the wild"? I don't get the impression that zfs-allow is a frequently used feature, especially not on Linux (since it can't allow mount or unmount).

Secondly, I don't think there is a particular danger here, even if we do allow the possibility of malicious users having been delegated. The malicious user can create fake properties or override the values of existing properties, but that can't be used for a privilege escalation. At most it can cause an extra call to zfs set that wouldn't have been made otherwise or a call to zfs set to be skipped. But in this scenario, the malicious user can already call zfs set themselves, so they don't need to trick Ansible into doing it for them.

I think that trying to address security concerns has a much higher bar. There needs to be a proof-of-concept that shows there really is a security vulnerability, and there needs to be confidence that any fix address the security vulnerability and doesn't create any new security holes in the process. Finally, if there is a vulnerability, the question should be asked of whether it is Ansible that should address it or ZFS. It seems to be that if this really is a risk (which I'm not yet convinced is the case), then it is likely something that ZFS should address, perhaps with a format that uses null characters as the delimiters.

[batrla]

No, I don't have particular example of exploit, but I may create one if it's desired. While most of ZFS installation won't be using ZFS delegated admin permissions, it doesn't mean there are no sophisticated deployments out there using it.

but that can't be used for a privilege escalation

yes, I would agree. At first glance I see no risk of privilege escalation. On the other hand if ansible is used to control something and relies on status of zfs module, then there might be risk of denial of service.

then it is likely something that ZFS should address, perhaps with a format that uses null characters as the delimiters.

Yes and no. Yes for new ZFS enhancement for new output format that allows printing multiple properties in a safe way that can be parsed by other programs. And "no" for ansible, as I mentioned in my second comment, we can't rewrite history, if there are already systems using old format and ansible controls them, then it can't be done without breaking compatibility. Alternative fix is to use some python binding to libzfs and get properties directly from libzfs but I'm unsure how simple it is.

[sam-lunt]

Thanks, an example of an exploit would be great. I think that would make this discussion more concrete.

[felixfontein]

nd "no" for ansible, as I mentioned in my second comment, we can't rewrite history, if there are already systems using old format and ansible controls them, then it can't be done without breaking compatibility.

You can always first figure out the version and then depending on that either use the new, faster method, or fall back to the old, slower one.

In any case, I agree this should be fixed, but the performance penality is bad indeed. How about making the behavior configurable, so that folks who care about speed (and trust their environment) can continue using the old way, while others can use the slow way if they are not sure (or if they don't specify anything - I would make it the new default)?

[batrla]

This pull request is stale. Let me please know if there's any interest to get the problem fixed.

[russoz]

This pull request is stale. Let me please know if there's any interest to get the problem fixed.

Hi @batrla Thanks for your contribution, and apologies for not giving this its due attention over time, it fell through the cracks.

That being said, as I have little to none experience with ZFS and the PR clearly raised its fair share of controversy, I think it might be beneficial for the module to have a test suite - there are no unit test nor integration tests today, unfortunately - but that would clearly be the work for another PR.

About the one failing test, I have no idea why it is happening (@felixfontein might have some clue ;-) ), but I would suggest you rebasing the PR to minimize the chance that any CI/CD change between then and now impact the PR.

Other than that, LGTM.

[ansibullbot]

@batrla this PR contains the following merge commits:

• 082633e

Please rebase your branch to remove these commits.

click here for bot help

[batrla]

Hi @russoz, just rebased the PR. I agree with what you wrote. It would be clearly beneficial to have test coverage for the ZFS module. I wish I could help more beyond this incremental bug fix, but can't do so now, it's ENOTIME from me due to my other commitments.

[russoz]

LGTM

[russoz]

I was thinking a), but yeah, you got a point. Anyways, we can always add docs later if needed. Both ZFS and OpenZFS are not things I know details about, so I won't press the point.

[patchback]

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/1f92a69992010152157c6084c29194a33f7fddf8/pr-6264

Backported as #9718

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@batrla thanks for your contribution!
@sam-lunt @russoz thanks for reviewing!