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

File-copy qrexec service is overly restrictive #8332

Closed
4 tasks
Nurmagoz opened this issue Jul 7, 2023 · 57 comments · Fixed by QubesOS/qubes-core-agent-linux#508
Closed
4 tasks

File-copy qrexec service is overly restrictive #8332

Nurmagoz opened this issue Jul 7, 2023 · 57 comments · Fixed by QubesOS/qubes-core-agent-linux#508
Assignees
Labels
affects-4.2 This issue affects Qubes OS 4.2. C: core diagnosed Technical diagnosis has been performed (see issue comments). P: major Priority: major. Between "default" and "critical" in severity. pr submitted A pull request has been submitted for this issue. release notes This issue should be mentioned in the release notes. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.

Comments

@Nurmagoz
Copy link

Nurmagoz commented Jul 7, 2023

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 #8332 (comment)

  • 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 File-copy qrexec service is overly restrictive #8332 (comment))
  • Make qubes-builderv2 disable both restrictions when copying files into dispvm
@Nurmagoz Nurmagoz added P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists. labels Jul 7, 2023
@marmarek
Copy link
Member

marmarek commented Jul 7, 2023

Indeed R4.2 introduces stricter filename validation, it needs to be (among other things) proper UTF-8 string. Maybe you use some different encoding for your file names?

@Nurmagoz
Copy link
Author

Nurmagoz commented Jul 7, 2023

Maybe you use some different encoding for your file names?

I was using Debian 11 as a standalone backup, which I had shifted from Qubes 4.1 using Qubes backup. After restoring my backup, I now want to create a new standalone based on Debian 12. I successfully created the new Debian 12 standalone and attempted to transfer my files to it. However, I encountered an issue where only files with non-English characters became stuck during the transfer process.

Workaround: you can change the file names to English before transferring them. Once the files have reached the destination, you can revert the changes by renaming them back to their original names.

@brendanhoar
Copy link

Workaround: you can change the file names to English before transferring them. Once the files have reached the destination, you can revert the changes by renaming them back to their original names.

I recommend archiving them all into a single archive file so that you can keep the original names in the destination.

B

@DemiMarie
Copy link

Workaround: you can change the file names to English before transferring them. Once the files have reached the destination, you can revert the changes by renaming them back to their original names.

I recommend archiving them all into a single archive file so that you can keep the original names in the destination.

B

Note that this loses the protection provided by qfile-unpacker’s filtering.

@DemiMarie
Copy link

The error message (“invalid or incomplete multibyte or wide character”) is horrible, not least because it provides no information as to which character was disallowed. At a minimum, there should be separate errors for “name is not valid UTF-8” and “name contains a forbidden UTF-8 character”, and the latter must state explicitly which character was forbidden.

@andrewdavidwong andrewdavidwong added C: core needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. labels Jul 8, 2023
@andrewdavidwong andrewdavidwong added this to the Release 4.2 milestone Jul 8, 2023
@andrewdavidwong andrewdavidwong added the affects-4.2 This issue affects Qubes OS 4.2. label Aug 8, 2023
@andrewdavidwong andrewdavidwong removed this from the Release 4.2 milestone Aug 13, 2023
@jamke
Copy link

jamke commented Dec 1, 2023

I recommend archiving them all into a single archive file so that you can keep the original names in the destination.

Yes, and it destroys the whole concept of security improvements that qvm-copy now should provide. The whole strict policy change makes no sense in this case, only affects user experience.

I personally think, that not being able to copy symlinks and files with non-utf-8 chars in name anymore is a major design mistake. The user data should be preserved at all costs. Ext2/3/4 allows non-utf8 chars in path and that is how it is. Only '\0' is not allowed, it is the only exception according to FS specs.

@marmarek @DemiMarie
What about adding some flags to qvm-copy to make it work as it was prior to R4.2, as expected by all current users?
Is it possible to add it before R4.2 release, that will break users experience and force users to pack everything in tar (like @brendanhoar suggested and I will also have to do) just due to the fear that qvm-copy will silently fail with the copy process, skip something or break intentionally?

@jamke
Copy link

jamke commented Dec 2, 2023

Moving my messages here:

Let's consider a directory with 100 000 files. It has 1 absolute symlink somewhere in the depth, 1 relative symlink targeting above this directory, and several files that are fine except their filenames have byte sequences that cannot be interpreted as utf-8 string.

User wants to copy it to the other qube (maybe much less secure) the same way as EVERY other copy tool works: like cp and including qvm-copy that existed for many years up to R4.1.

What should the user do to achieve it with the new R4.2 approach?

@jamke
Copy link

jamke commented Dec 2, 2023

There are 2 cases: from terminal and from GUI (Nautilus or Dolphin).
What the user should do in both cases to copy files and avoid user's data loss?

@jamke
Copy link

jamke commented Dec 2, 2023

Would it make sense to have a "qvm-copy --insecure" tool/option (including in the file manager)?

I think it definitely would!
I understand what you are saying, I just think consequences can be worse, than what's gained, at least in my use cases. Because qvm-copy is already "secure", and the filtering is like adding additional layer. Like: why not check copied files with antivirus, or block copying shell/ELF files or something, it all looks as the same approach that copy tool should do to be called secure.

For me secure is more like:

  • for any directory/file it does not cause any code execution itself because of the file naming or data (like buffer overflow or bad parsing code),
  • for any directory/file it should not cause user data losses (or increase changes of it); other meaning of secure as in safe.

The R4.1 meets these criteria completely.


My main point:
After these changes the user will never be sure if Qubes OS copy tool will work as they expect:

  • User moves a directory with a big size, goes to drink tea or even sleep, comes back and sees that there is an error about symlink or filename that happened in the beginning of the process.
  • After that user can remove this symlink or tar it and start again, once again not being sure that it will not stop in a minute, or an hour.
  • If user is able to auto-skip such files, than they will almost certainly loose some files eventually. At least I would.

@adrelanos
Copy link
Member

adrelanos commented Dec 2, 2023 via email

@DemiMarie
Copy link

@marmarta what do you think? I don’t know enough about UX to be able to make an informed statement here.

@jamke So the problem with symlinks is:

echo > ~/QubesIncoming/x/a
# oops, overwrote ~/.bashrc (or some other important file)
cat ~/QubesIncoming/x/b
# oops, just read e.g. ~/.ssh/id_ed25519 (or some other secret key)

@andrewdavidwong
Copy link
Member

As far as I can tell, symlinks are off-topic in this issue.

@jamke

This comment was marked as off-topic.

@DemiMarie
Copy link

@jamke This would need to be a separate service (qubes.FileCopyUnsafe?) so that the prompt presented to the user indicates that this is a more dangerous action. @marmarta thoughts on the name?

@jamke
Copy link

jamke commented Dec 3, 2023

This would need to be a separate service (qubes.FileCopyUnsafe?) so that the prompt presented to the user indicates that this is a more dangerous action.

@DemiMarie Sounds interesting, I like the idea.

@DemiMarie
Copy link

This would need to be a separate service (qubes.FileCopyUnsafe?) so that the prompt presented to the user indicates that this is a more dangerous action.

@DemiMarie Sounds interesting, I like the idea.

I think this is something that could be considered. This would still preserve less metadata than tar or cp -a, because the latter two preserve e.g. file ownership, SELinux contexts, and possibly hard links as well.

@jamke
Copy link

jamke commented Dec 3, 2023

I think this is something that could be considered. This would still preserve less metadata than tar or cp -a, because the latter two preserve e.g. file ownership, SELinux contexts, and possibly hard links as well.

I agree.

  • time data (like mtime) filenames, filecontent should be preserved as is,
  • the ownership can be dropped, as user IDs can be different in general case,
  • the rights (chmod) can be preserved if it is OK,
  • symlinks should not be affected,
  • and hardlinks should be dropped for sure, as it is not a copy process inside one drive by design,
  • other metadata (like SELinux contexts) can be dropped to.

The whole process can be considered similar to copying between 2 air-gapped PCs with a flash-drive formatted to ext4. At least it sounds logical and fits architecture of qubes as separated PCs.

@UndeadDevel
Copy link

UndeadDevel commented Dec 28, 2023

Absolutely agree that there should be a "copy_unsafe" option; ran into this issue now several times and it is extremely annoying to deal with...even when a file name has only Latin characters it sometimes crops up, not even to mention non-Latin ones.

I'll also add that UX is still pretty bad with the error message as the target qube is asked with a pop up dialogue, but the error / failure message is just an echo in the terminal; the last time I did qvm-copy and it failed I actually lost the data, because I assumed that the error would be another pop up dialogue, which didn't happen, and the filename was only Latin characters (and some apparently non-standard apostrophes) and then I shut down the disposable thinking everything had been transferred when it wasn't (it was easy to recreate that data - this time -, but still...super bad UX).

@rustybird
Copy link

rustybird commented Dec 28, 2023

I assumed that the error would be another pop up dialogue, which didn't happen

That should be fixed by QubesOS/updates-status#4240 + QubesOS/updates-status#4233

Edit: Those are for GUI file copy

@andrewdavidwong andrewdavidwong added P: major Priority: major. Between "default" and "critical" in severity. and removed P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. labels Dec 28, 2023
@andrewdavidwong andrewdavidwong changed the title Cant copy/move files with non-english characters (invalid or incomplete multibyte or wide character) Allow inter-qube file copy/move when filename contains disallowed characters Dec 28, 2023
@andrewdavidwong andrewdavidwong added the diagnosed Technical diagnosis has been performed (see issue comments). label Jun 24, 2024
@andrewdavidwong
Copy link
Member

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
Copy link
Member

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
Copy link

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

allow-all-names would be even better.

marmarek added a commit to marmarek/qubes-core-admin that referenced this issue Jun 24, 2024
It was renamed from allow-unsafe-characters to allow-all-names

See QubesOS/qubes-core-agent-linux#508

QubesOS/qubes-issues#8332
@github-project-automation github-project-automation bot moved this from In progress to Done in Current team tasks Jun 25, 2024
marmarek pushed a commit to QubesOS/qubes-core-agent-linux that referenced this issue Jun 25, 2024
As pointed out by Andrew David Wong the latter name is unnecessarily
alarming.  No backwards compatibility is provided because users should
not need to remember to blocklist two different strings in their qrexec
policies.  Denying "+allow-all-names" should be sufficient.

Reported-by: Andrew David Wong <adw@qubes-os.org>
Fixes: QubesOS/qubes-issues#8332 (for real this time)
(cherry picked from commit 59d94f3)
@andrewdavidwong andrewdavidwong added the pr submitted A pull request has been submitted for this issue. label Jun 25, 2024
andrewdavidwong added a commit to QubesOS/qubes-posts that referenced this issue Jun 25, 2024
andrewdavidwong added a commit to QubesOS/qubes-doc that referenced this issue Jun 25, 2024
marmarek added a commit to QubesOS/qubes-core-admin that referenced this issue Jun 25, 2024
marmarek added a commit to QubesOS/qubes-core-admin that referenced this issue Jun 25, 2024
Symlinks pointing outside of copied directory should be denied in both
filecopy modes

QubesOS/qubes-issues#8332

(cherry picked from commit 0aa1382)
marmarek added a commit to QubesOS/qubes-core-admin that referenced this issue Jun 25, 2024
It was renamed from allow-unsafe-characters to allow-all-names

See QubesOS/qubes-core-agent-linux#508

QubesOS/qubes-issues#8332
(cherry picked from commit 872119f)
marmarek added a commit to QubesOS/qubesos.github.io that referenced this issue Jun 27, 2024
_posts:
    gpg: Good signature from "Andrew David Wong (Qubes Documentation Signing Key)" [ultimate]
    object 21f106602a71f1b938ffe9179b6dd8ccb5dcf016
    type commit
    tag adw_21f10660
    tagger Andrew David Wong <adw@qubes-os.org> 1719471777 -0700

    Tag for commit 21f106602a71f1b938ffe9179b6dd8ccb5dcf016

    21f1066 Merge branch '4.2.2-rc1'
    02c932f Specify post date
    5e07e83 Explain how the fix works; clarify relationship between services
    4e90dcf Update QubesOS/qubes-issues#8332 service name and explanation
    d501c15 Release: Qubes OS 4.2.2-rc1
marmarek added a commit to QubesOS/qubesos.github.io that referenced this issue Jun 27, 2024
_doc:
    gpg: Good signature from "Andrew David Wong (Qubes Documentation Signing Key)" [ultimate]
    object 941ef460540d9f20e8e845952c194cff98803dd2
    type commit
    tag adw_941ef460
    tagger Andrew David Wong <adw@qubes-os.org> 1719471912 -0700

    Tag for commit 941ef460540d9f20e8e845952c194cff98803dd2

    941ef460 Merge branch '4.2.2'
    beac6f7e Explain how the fix works; clarify relationship between services
    8da080c1 Update QubesOS/qubes-issues#8332 service name and explanation
    cb4ec61e Update release notes for Qubes OS 4.2.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-4.2 This issue affects Qubes OS 4.2. C: core diagnosed Technical diagnosis has been performed (see issue comments). P: major Priority: major. Between "default" and "critical" in severity. pr submitted A pull request has been submitted for this issue. release notes This issue should be mentioned in the release notes. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.