require rollback permission when force receive

[shodanshok]

Force receive (zfs receive -F) can rollback or destroy snapshots and file systems that do not exist on the sending side (see zfs-receive man page). This means an user having the receive permission can effectively delete data on receiving side, even if such user does not have explicit rollback or destroy permissions.

This patch add the rollback permission requirement for force receive. To avoid changing current default behavior, a new zfs_recv_perm tunable is introduced. When set to 0 (default) the new permission check is disabled. When set to 1 rollback permission requirement is enabled.

Fixes #16943

Motivation and Context

Description

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

I am not very deep in this area, but it makes sense to me. I just worry that it changes existing behavior, that may upset many existing setups. And that permission is required even when no actual revert might be needed. Though I guess if the receiving side is mounted for writing with atime enabled, then simple traversal through it might change access times, requiring the rollback. And if user is forced to add -F even for routine operations he may also get forced to add the permission, that may devalue the whole purpose, unless user in some way make receiving side read-only or not mounted.

Also for stream generated with send -R aside of the rollback receive -F might delete old snapshots and datasets, rename, may be something else. I guess those might already need additional permissions, that are just not documented now?

It is confusing to me that those functions are sharing the same -F flag, and I actually discussed it with @ixhamza lately. I wonder if we could somehow better define the use cases to allow them in more restricted scenarios. For example, in case of backup server, I don't see a problem in receive able to rollback if only to the last snapshot, without any older snapshots deletion, that could be a part of an attack on backup server, while backup server does not care about local changes after the last snapshot. rollback command actually has an -r option to allow snapshots deletion. I wonder if that already requires destroy permission, or it is another bad design example.

[shodanshok]

I am not very deep in this area, but it makes sense to me. I just worry that it changes existing behavior, that may upset many existing setups. And that permission is required even when no actual revert might be needed. Though I guess if the receiving side is mounted for writing with atime enabled, then simple traversal through it might change access times, requiring the rollback.

True, I generally run backup server with atime=off and/or readonly=on for this reason.

And if user is forced to add -F even for routine operations he may also get forced to add the permission, that may devalue the whole purpose, unless user in some way make receiving side read-only or not mounted.

Again, true, but for such a potentially destructive operation -F really seems needed. What is missing is the additional permission check.

Also for stream generated with send -R aside of the rollback receive -F might delete old snapshots and datasets, rename, may be something else. I guess those might already need additional permissions, that are just not documented now?

Unless I am missing something, checked permissions as defined by zfs_secpolicy_recv are ZFS_DELEG_PERM_RECEIVE, ZFS_DELEG_PERM_MOUNT and ZFS_DELEG_PERM_CREATE (which matches man page).

It is confusing to me that those functions are sharing the same -F flag, and I actually discussed it with @ixhamza lately. I wonder if we could somehow better define the use cases to allow them in more restricted scenarios. For example, in case of backup server, I don't see a problem in receive able to rollback if only to the last snapshot, without any older snapshots deletion, that could be a part of an attack on backup server, while backup server does not care about local changes after the last snapshot. rollback command actually has an -r option to allow snapshots deletion. I wonder if that already requires destroy permission, or it is another bad design example.

I agree in principle, but the current situation is bad enough that should be corrected as soon a possible. As things are now, a malicious send can actually destroy snapshots on the receiving side without any specific rollback/destroy permission.

Any more fine-grained behavior (ie: do not require rollback permission for rolling back last snapshot only) depends on the inspection of the receive stream itself - which, as far I know, happens after permission checking. So, while such detailed check would be ideal, I do not see any simple means to implement it.

[amotin]

Unless I am missing something, checked permissions as defined by zfs_secpolicy_recv are ZFS_DELEG_PERM_RECEIVE, ZFS_DELEG_PERM_MOUNT and ZFS_DELEG_PERM_CREATE (which matches man page).

AFAIK user-space zfs receive -F might do multiple ioctls. That might actually check permissions only as needed, but in a different way.

So, while such detailed check would be ideal, I do not see any simple means to implement it.

We could add less aggressive forms of -F, something like --rollback or --rollback-to-last. Or may be check some permissions later indeed.

[shodanshok]

AFAIK user-space zfs receive -F might do multiple ioctls. That might actually check permissions only as needed, but in a different way.

I just tried with send -R and I can confirm that it seems to require destroy and rename privileges. However, the issue with send -i and receive -F remains as explained above: a malicious send can destroy snapshot on the backup destination even if the receive is executed by a non-root user with no privileges other than mount,create,receive. As things stand now, this seems a security issue which should be addressed.

We could add less aggressive forms of -F, something like --rollback or --rollback-to-last.

I don't feel that adding a less aggressive form of -F would mitigate the issue in the face of a malicious send stream unless -F itself is masked behind a rollback/destroy permission. Avoiding the current -F behavior seems a prerequisite to add something as --rollback-to-last, which could suffice with actual mount,create,receive privileges.

Or may be check some permissions later indeed.

It would be great, but I don't know if/how this fits in the current code, or how much time would be needed to add this kind of checks.

[amotin]

Avoiding the current -F behavior seems a prerequisite to add something as --rollback-to-last, which could suffice with actual mount,create,receive privileges.

Why prerequisite? It could be done same time or even unrelated. If you make -F require additional privileges, user might want an alternative to giving those privileges. Especially since you are breaking existing behavior.

how much time would be needed to add this kind of checks.

I am not trying to diminish the seriousness of the issue, but with all due respect I guess the issue must have existed for decades. Lets spend a week or two on thinking without extra rush.

[shodanshok]

Why prerequisite? It could be done same time or even unrelated. If you make -F require additional privileges, user might want an alternative to giving those privileges.

Sure. What I meant to say is that, in order for the (hypothetical) --rollback-to-last to be useful, -F should be modified to require additional privileges. Otherwise, simply adding a new lower-privilege/limited receive mode without changing current -F behavior is not going to be useful.

Especially since you are breaking existing behavior.

If preserving compatibility with current behavior is required, how do you feel about a tunable to enable/disable the added check?

I am not trying to diminish the seriousness of the issue, but with all due respect I guess the issue must have existed for decades. Lets spend a week or two on thinking without extra rush.

I agree, and thanks for you feedback. This PR is about a simple fix, but I am all for a better solution if possible.

[amotin]

If preserving compatibility with current behavior is required, how do you feel about a tunable to enable/disable the added check?

One of the problems here is that replication is often done through SSH, and the receive command is supplied by the sender. It means all the senders using -F will break the moment you update the receiver, unless you grant the permission, which devalues the whole point. We need some sort of plan how average user could move forward a big setup without needing to do things at the time of update. Tunable might be a part of that plan, allowing the control over the time window while the old behavior is kept, but generally I am not a big fan of tunables, since you need to know about their existence and how to use them.

[ixhamza]

@shodanshok - There is also a path that could delete a snapshot during an incremental receive even before performing the actual zfs receive operation if some destination snapshot does not match with the send stream. This requires destroy permissions, however. The following script now errors out due to missing rollback permission after your patch, but it still deletes rpool/test/dest@x before erroring out.

# ROOT
truncate -s 1G ~/a1
zpool destroy rpool
zpool create rpool ~/a1
zfs create rpool/test
zfs create rpool/test/src
zfs allow -u $USER create,hold,send,snapshot,mountpoint,mount,receive,destroy rpool/test
zfs snapshot rpool/test/src@a
zfs send -R rpool/test/src@a | zfs receive -o mountpoint=none -F rpool/test/dest

# $USER
zfs snapshot rpool/test/src@b
zfs snapshot rpool/test/dest@x
zfs send -RI rpool/test/src@a rpool/test/src@b | zfs receive -F rpool/test/dest
# cannot receive incremental stream: permission denied

[shodanshok]

@shodanshok - There is also a path that could delete a snapshot during an incremental receive even before performing the actual zfs receive operation if some destination snapshot does not match with the send stream. This requires destroy permissions, however. The following script now errors out due to missing rollback permission after your patch, but it still deletes rpool/test/dest@x before erroring out.

# ROOT
truncate -s 1G ~/a1
zpool destroy rpool
zpool create rpool ~/a1
zfs create rpool/test
zfs create rpool/test/src
zfs allow -u $USER create,hold,send,snapshot,mountpoint,mount,receive,destroy rpool/test
zfs snapshot rpool/test/src@a
zfs send -R rpool/test/src@a | zfs receive -o mountpoint=none -F rpool/test/dest

# $USER
zfs snapshot rpool/test/src@b
zfs snapshot rpool/test/dest@x
zfs send -RI rpool/test/src@a rpool/test/src@b | zfs receive -F rpool/test/dest
# cannot receive incremental stream: permission denied

Thanks for pointing out that. If I understand it correctly, an explicit destroy permission is required, so user should at least be aware that an unexpected destroy operation can succeed. On the other side, we currently allow destroy/rollback operations without granting any such permissions.

[ixhamza]

Yes, destroy permission is required. Though failing to receive the incremental stream but still able to destroy the snapshot sounds confusing.

[shodanshok]

Yes, destroy permission is required. Though failing to receive the incremental stream but still able to destroy the snapshot sounds confusing.

Do you think it would be better to check for rollback or destroy privileges, so that if destroy is allowed, then receive can continue? On the other hand, requiring explicit destroy/rollback permissions does not seems wrong to me.

[amotin]

For a note, I brought this topic up on the yesterday's OpenZFS Leadership Meeting. The video should probably be on YouTube "soon". In general, consensus predictably was that breaking compatibility without warning is bad. It should likely be spread over at least couple releases, while alternatives in forms of new flags and permissions are provided to allow users to migrate before old functionality is blocked, if needed. I personally don't mind if some module parameter can force it sooner if user desire, but it should not be a primary solution.

[shodanshok]

For a note, I brought this topic up on the yesterday's OpenZFS Leadership Meeting. The video should probably be on YouTube "soon". In general, consensus predictably was that breaking compatibility without warning is bad. It should likely be spread over at least couple releases, while alternatives in forms of new flags and permissions are provided to allow users to migrate before old functionality is blocked, if needed. I personally don't mind if some module parameter can force it sooner if user desire, but it should not be a primary solution.

Thanks for discussing this issue. I added a new zfs_recv_perm tunable to enable the additional permission check, which is disabled by default. So current default behavior is unchanged, but user can opt-in for added safeguard against recv -F.

[amotin]

The name seems not very descriptive.

[shodanshok]

Any suggestion?

[amotin]

I can't think if everything. ;) On a quick look I see we already have a vfs.zfs.recv.* section (in FreeBSD terms). So may be something like vfs.zfs.recv.check_perm_on_force?

[shodanshok]

As linux seems to use zfs_recv_*, and as some checks are already done by current code, what about zfs_recv_force_need_perm? Otherwise, I happily use your suggestion above.

[amotin]

OK. Just "need" or "needs"?

[shodanshok]

"needs" seems better, I will use zfs_recv_force_needs_perm

[randomnetcat]

If breaking existing setups is an issue, an alternative to the tunable might be to add a new permission named, e.g., receive:append that allows only receives without -F (and sufficient warnings in the documentation to make clear when it should be used). That could also be done later, of course.

[amotin]

@randomnetcat Yes, that is kind of what I meant with "while alternatives in forms of new flags and permissions are provided". I see it as splitting -F into several more specific options, controlled by more specific permissions. We just need a good structure, and probably not only for the receive.

[randomnetcat]

Ah, got it. Makes sense.

[shodanshok]

If breaking existing setups is an issue, an alternative to the tunable might be to add a new permission named, e.g., receive:append that allows only receives without -F (and sufficient warnings in the documentation to make clear when it should be used). That could also be done later, of course.

I like something as an "append" permission.

@amotin do you think that adding a specific append permission to only permit "plain" receive would be better than a module tunable? If so, I can try to do it. Otherwise, if an entire good permission structure has to be created, I will go ahead with the (temporary) tunable path.

[amotin]

do you think that adding a specific append permission to only permit "plain" receive would be better than a module tunable?

I am not sure I like that the shortest and the most obvious permission name means more than necessary (includes rollback variants), but it would be the least invasive change for the existing users and sure better than any tunables. I'd be fine with it (it must be properly documented), but I would be glad to hear more opinions.

But as I mentioned above there are two cases of rollback: to the last snapshot and deleting some snapshots. I think we should investigate that area too while we are at this to make sure things are consistent.