File-copy qrexec service is overly restrictive

TNTBOMBOM commented 1 year ago

Qubes OS release

4.2

Brief summary

moving/coping files was working ok in 4.1 regardless their name, now its rejected due to the name of the file used.

Steps to reproduce

Try to move/copy a file in none latin letters

Expected behavior

To move/copy file regardless what characters used to name it.

Actual behavior

nonenglishcharct

Tasks

More detailed design in https://github.com/QubesOS/qubes-issues/issues/8332#issuecomment-1912403043

[ ] Extend qfile-unpacker to have extra options for disabling various restrictions (independently): file names validation, symlink target restriction (refusing absolute symlinks or pointing outside of target directory)
[ ] Add a new qubes.UnsafeFilecopy qrexec service with file names validation disabled (but symlinks validation enabled)
[ ] Modify qvm-copy to select the service based on chosen files (see https://github.com/QubesOS/qubes-issues/issues/8332#issuecomment-1912403043)
[ ] Make qubes-builderv2 disable both restrictions when copying files into dispvm

marmarek commented 3 months ago

If you look at related discussions, another way of looking at it is that change in R4.2 to make it refuse some file names is a regression or at least incompatible change that shouldn't happen in minor release...

jamke commented 3 months ago

I agree that "unsafe" way of copying can be default, especially if more safe way causes issues for users. Fighting with unicode by making limitations that would work for everybody is hard and if at all possible :) I also agree that "unsafe" is still very safe, after all filenames in ext4 and other Linux filesystems allow all chars except \0 and \\, even not valid utf8 combinations by design.

And I agree with @unman and @andrewdavidwong , that it can be a good idea to announce this change in the release notes more loud.

Anyway, I am OK with any solution, I also consider the current state as a regression or at least a possible issue for some users.

andrewdavidwong commented 3 months ago

If you look at related discussions, another way of looking at it is that change in R4.2 to make it refuse some file names is a regression or at least incompatible change that shouldn't happen in minor release...

Fair enough. In that case, it would just be a bug fix. Since it could be classified either way, I think it's your call, @marmarek. Either way, I suppose there's no reason not to include it in the release notes.

marmarek commented 3 months ago

@DemiMarie please document at https://www.qubes-os.org/doc/vm-interface/#qubes-rpc the allow-unsafe-characters argument

@marmarta the merged version defaults to the "unsafe" version without any policy change - thanks to policy allowing any argument. Should we have a GUI option for disabling the "unsafe" one? Technically, qubes.Filecopy with empty argument is "safe" and with allow-unsafe-characters argument (so, qubes.Filecopy+allow-unsafe-characters) has some "unsafe" characters. To switch a single rule to disallow unsafe, simply change * in arg column to + (IOW, change "any arg" to "only empty"). To disallow globally, add a deny rule for qubes.Filecopy+allow-unsafe-characters before other rules.

andrewdavidwong commented 3 months ago

If you look at related discussions, another way of looking at it is that change in R4.2 to make it refuse some file names is a regression or at least incompatible change that shouldn't happen in minor release...

Fair enough. In that case, it would just be a bug fix. Since it could be classified either way, I think it's your call, @marmarek. Either way, I suppose there's no reason not to include it in the release notes.

Since this change is being included in Qubes OS 4.2.2, which is a patch release, I infer that the decision has been made to classify this issue as a bug rather than an enhancement. Updated labels and issue title accordingly.

andrewdavidwong commented 3 months ago

No, the idea is to make the "unsafe" variant the default. I use "unsafe" in quotes because the practical impact is rather small (exploiting this requires somebody finding another bug in a font rendering engine - those happen, but are very rare). On the other hand, as proven in several reports, hitting issues with copying files with for example Arabic names, users fallback to much less safe options (like packing into zip).

It sounds like you don't believe that it's truly unsafe. If you did, I don't believe you would be willing to make it the default or classify this change as a bug fix. In light of this, I believe that naming the new service allow-unsafe-characters is a mistake. Naming it "unsafe":

Does not seem to accurately reflect the security impact of using the service.
Does not seem to accurately reflect the Qubes core developers' opinion of the service.
Will likely lead many users to believe that the service is truly unsafe.
Will likely lead many users to seek the "safe" alternative, even if it would not benefit them and may even cause problems for them.
May cause some users to question why the "unsafe" version is being made the default (rather than opt-in).
May cause some users to have doubts about whether Qubes OS ships with secure defaults in general.
May further encourage some users to tinker with their systems in ways they do not fully understand in an effort to improve upon the default level of security, which may break their systems and undermine their security in various ways.
May cause some users to lose confidence in the way the Qubes OS Project uses words like "safe" and "unsafe." (They won't know what we mean, and they won't be able to trust that we're using such words in the same way they understand them.)
Effectively "hard codes" a value judgment into the service that should instead be left for the user to decide. Different users have different priorities and risk tolerances. Several users in this thread have expressed a strong desire to use the new service. They probably don't really believe that what they want to do is unsafe for them. Qubes OS generally tries to avoids hard coding such value judgments. (E.g., the meanings of color labels are intentionally left to the user to define.)

I discussed this with @DemiMarie, who suggested that allow-all-bytes may be a more accurate name.

DemiMarie commented 3 months ago

I discussed this with @DemiMarie, who suggested that allow-all-bytes may be a more accurate name.

allow-all-names would be even better.

QubesOS / qubes-issues