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

qvm-copy: incorrect error message upon encountering symlink pointing outside copy area #8581

Closed
sjbach opened this issue Oct 8, 2023 · 26 comments
Labels
affects-4.2 This issue affects Qubes OS 4.2. C: core diagnosed Technical diagnosis has been performed (see issue comments). 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.

Comments

@sjbach
Copy link

sjbach commented Oct 8, 2023

How to file a helpful issue

Qubes OS release

4.2 rc3

Brief summary

In 4.2 when qvm-copy encounters a broken symbolic link it errors out and reports "Invalid or incomplete multibyte or wide character". The true error is that the symlink points to a nonexistent file.

(Is it actually even an error though? I suppose it is. The interface to qvm-copy appears to be very simple, no flags to tweak the copy semantics, and I assume this is purposeful/philosophical: no tricky corner cases while crossing inter-qube borders, please.)

Steps to reproduce

user@testvm:~$ mkdir parent-dir
user@testvm:~$ cd parent-dir/
user@testvm:~/parent-dir$ ln -s /tmp/foo/bar/baz broken-symlink
user@testvm:~/parent-dir$ cd ..
user@testvm:~$ qvm-copy parent-dir
# (Choose a qube)
qfile-agent: Fatal error: File copy: “Invalid or incomplete multibyte or wide character; Last file: /tmp/foo/bar/baz” (error type: Invalid or incomplete multibyte or wide character)

Expected behavior

(a) Copy across the broken symlink

or

(b) Error out and report the broken symlink

Actual behavior

Error out and falsely report an encoding error

@sjbach sjbach 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 Oct 8, 2023
@andrewdavidwong andrewdavidwong added C: core needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. affects-4.2 This issue affects Qubes OS 4.2. labels Oct 8, 2023
@rustybird
Copy link

qrexec-lib from linux-utils v4.2.5 is the last version that successfully sends the broken symlink. I see the exact same error message starting with the next version v4.2.6 of qrexec-lib (up to and including the latest v4.2.14), installed on the receiver side.

@marmarek
Copy link
Member

marmarek commented Oct 8, 2023

Indeed there is an intentional change to forbid symlinks pointing outside of copying area (it considers both relative symlinks and absolute one - banning the latter completely).

@marmarek
Copy link
Member

marmarek commented Oct 8, 2023

And yes, the error message needs fixing

@rustybird
Copy link

rustybird commented Oct 8, 2023

Besides the choice of EILSEQ, sending an errno as an integer is probably also not portable? Like if one side is Linux and the other were to be a BSD. That was already bothering me a little while pressing EREMOTEIO into service on early EOF. (Hmm I'll have to look up how that's done for Filecopy to Windows)

@jamke
Copy link

jamke commented Oct 9, 2023

Indeed there is an intentional change to forbid symlinks pointing outside of copying area (it considers both relative symlinks and absolute one - banning the latter completely).

Can this be reconsidered?

In fact it says: qvm-copy breaks user's files without warning. It kind of forces users who use symlinks to pack everything to tar before coping to prevent user data loss by this unreliable qvm-copy.

Will qvm-move completely delete those symlinks that were not copied? It will be unacceptable to remove them, to my opinion, because it will be 100% data loss.

Use case: User copies their project/configuration from one VM to another and it is deliberately corrupted by qvm-copy.
It this quite what user should expected of qvm-copy and what qvm-copy should do?

@jamke
Copy link

jamke commented Oct 9, 2023

qrexec-lib from linux-utils v4.2.5 is the last version that successfully sends the broken symlink.

@rustybird what is your opinion on coping symlinks?
I think qvm-copy/qvm-move should copy files, including symlinks exactly as they are, both relative and absolute, including those that are not pointing to existing files at the moment (maybe something can be mount tand symlink will become active, common practice).

@marmarek
Copy link
Member

marmarek commented Oct 9, 2023

qvm-copy/qvm-move is about copying/moving files to another qube in a safe way - so even if the source is malicious, it should still make it hard to trick the user. Having for example a symlink to /home/user/.bashrc (or /home/user/.gnupg/secring.gpg) is a risky thing because not careful user (or some tool/script they use) may mistakenly take it as part of the files copied from another qube.

@rustybird
Copy link

rustybird commented Oct 9, 2023

@jamke I wouldn't call it corruption or data loss because qvm-copy doesn't silently drop broken symlinks, it exits nonzero and prints a message. (Even though thanks to glibc's* error string for EILSEQ that message looks weird at the moment.) And Filecopy was already restricted to only regular files, directories, symlinks - not device nodes, fifos, sockets; and hardlinks/reflinks/sparseness aren't preserved as such. So validating symlinks now (and also UTF-8 for filenames) doesn't seem out of place to me.


* For reference: glibc's list of error strings, musl's. ENOLINK ("Link has been severed" in both) would be a funny one

@andrewdavidwong andrewdavidwong added diagnosed Technical diagnosis has been performed (see issue comments). and removed needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. labels Oct 9, 2023
@jamke
Copy link

jamke commented Oct 9, 2023

@rustybird thank you for noting that copying will be broken with error message, I agree that it is better than to stop working silently, but still worse than simply work as user expects.

And Filecopy was already restricted to only regular files, directories, symlinks - not device nodes, fifos, sockets; and hardlinks/reflinks/sparseness aren't preserved as such.

All this is obvious and expected by user. How can block device or socket be "copied" or "moved" to other VM? User should not expected it. And hardlinks by design cannot be preserved when copied or moved to another partition/disk. Well known by people who use hardlinks.

But misbehaving for symlinks has no obvious reason to be happening. Symlinks are used all other the place by a lot of people, including me. I have a lot of symlinks that target some dirs that are not available most of the time. It is and always was excepted behavior to copy files, dirs and symlinks (both types).

Currently I am talking about changes that will break things, that will affect copying process badly, in a way which is NOT expected by user.

I will report it as a bug ticket if it exists, even if the ticket will (should?) be closed immediately as won't fix . Because users should know that devs broke something intentionally that was working perfectly well and seems to have no justified reason to be broken in the first place.

So validating symlinks now (and also UTF-8 for filenames) doesn't seem out of place to me.

I understand the possible reasons for such validation, but it is not right. You can open FS specification and EXT2/3/4 has only one forbidden char - '\0', nothing else. I see no reason forcing other limitations for qvm-copy, as it will just look that this command is not able to properly work with such paths, that's all. Fortunately, this is more rare case than broken copying of symlinks that happens all the time.

@jamke
Copy link

jamke commented Oct 9, 2023

Maybe there can be some flag to make qvm-copy and qvm-move work as they were and as they should - properly copying files, including user's symlinks?

Looks like some alternative for people who actually want to copy symlinks, too. I will simply replace qvm-copy with a wrapper to avoid consequences of these breaking changes all together.

@rustybird
Copy link

rustybird commented Oct 9, 2023

How can block device or socket be "copied" or "moved" to other VM? User should not expected it. And hardlinks by design cannot be preserved when copied or moved to another partition/disk.

You totally can preserve device files or hardlinks with cp -a, even for other machines with tar or rsync if you pass the right options. (For a device file that means recreating it with the same block/character class + major + minor number. For hardlinks it means if the source had files a and b hardlinked to the same inode and they're both being copied, the destination should get one inode with two names/hardlinks as well. No idea if sockets can be copied in a meaningful way.)

only one forbidden char - '\0', nothing else. I see no reason forcing other limitations for qvm-copy

Generally, inter-VM Filecopy is intended to be rock solid secure. You should be able to have the nastiest source VM send whatever garbage to a good destination VM without that alone compromising the destination.

Except that if you don't filter filenames, then just a simple ls ~/QubesIncoming/sourcevm/ (or opening a file manager in the directory) could trigger an exploit against Unicode libraries, ancient xterm code, etc. Most people aren't going to have that on their radar - "I haven't even opened the file yet!" - so it's sensible to start out with a restrictive default.

But yes it also would be nice to make that easily configurable. Maybe it could be tri-state: unrestricted filenames / valid UTF-8 only / printable ASCII plus space only

I will simply replace qvm-copy with a wrapper

That's a real downside of restrictive Filecopy: Users will fall back on ad-hoc splicing in GNU tar to parse decades worth of very complex archive formats. It's always horrifying to read yet another forum post where a source VM is tarring data, to be untarred in dom0...


Anyway, sorry for doing my part to turn this into the Filecopy omni-ticket, when the immediate question is really just "what's a better errno than EILSEQ"

@marmarek
Copy link
Member

marmarek commented Oct 9, 2023

sending an errno as an integer is probably also not portable?

Technically, it doesn't need to be. It's "file copy error code", which happen to match subset of Linux's errno values. Non-Linux implementation would need to translate (like windows agent does).

"what's a better errno than EILSEQ"

Exactly, that's the important question here. Disallowed symlink target should have a different error code than disallowd file name. EXDEV ("Cross-device link") is kinda logical here (normally it's for hardlinks in similar situation, but we'd apply it here to symlinks), but it wouldn't cover the case of symlink target with disallowed characters. Should those have different error code (symlink target pointing outside of dest dir vs symlink target with disallowed characters) ?

Technically it may require a bit of refactor, to distinguish between reasons for rejecting symlink target.

  • For reference: glibc's list of error strings, musl's. ENOLINK ("Link has been severed" in both) would be a funny one

@DemiMarie @rustybird any opinion?

@jamke
Copy link

jamke commented Oct 10, 2023

You totally can preserve device files or hardlinks with cp -a

I would expect qvm-copy to work as cp, to properly copy files, dirs and symlinks. I would not expect it to copy devices nor sockets. And it what cp does by default.

Generally, inter-VM Filecopy is intended to be rock solid secure. You should be able to have the nastiest source VM send whatever garbage to a good destination VM without that alone compromising the destination.

Compromising is not done by coping but by software in target qube that is not able to properly work with non-utf8 chars.
I am not against showing error for such filenames by default, because it is a rare case and I understand that in most cases it is not what user would want, to have such broken filenames.

But symlinks is what user often wants to have properly copied. Isn't it true?

But yes it also would be nice to make that easily configurable. Maybe it could be tri-state: unrestricted filenames / valid UTF-8 only / printable ASCII plus space only

I agree, it would be nice to have options like that. Though I am more worried for broken symlinks. What is the security problem with symlinks in the first place? That users do not understand how they work?

That's a real downside of restrictive Filecopy: Users will fall back on ad-hoc splicing in GNU tar to parse decades worth of very complex archive formats. It's always horrifying to read yet another forum post where a source VM is tarring data, to be untarred in dom0...

I agree once again, but what should users do if they want something to be copied as it is, binary-perfect? qvm-copy has no trust for that.

@DemiMarie
Copy link

@marmarek I still think that rejecting symlinks that point outside the destination area is the right thing to do, because it prevents the following attack:

cat ~/QubesIncoming/evil-qube/x > ~/QubesIncoming/evil-qube/y
# oops!  now ~/.profile has been overwritten by `x`, attacker wins

Nevertheless, the error message is not merely unhelpful but outright misleading, and that is my fault. EXDEV would be a bit better, but even better would be to write a message to stderr explaining what went wrong:

note: absolute symbolic link target /a/b rejected
note: for security reasons, qvm-copy doesn't support absolute symbolic links
note: please see <some page in documentation> for details

@marmarek
Copy link
Member

@marmarek I still think that rejecting symlinks that point outside the destination area is the right thing to do

Yes, I agree here. We may have alternative service (disabled or even not installed by default) that is more permissive, but the default one should be reasonably strict.

As for the error - we still need some error code. And then we can make a custom message for it on the client side (as we do for EEXIST for example).

@DemiMarie
Copy link

As for the error - we still need some error code. And then we can make a custom message for it on the client side (as we do for EEXIST for example).

EXDEV seems like the most suitable answer, provided that it will not “legitimately” happen (due to e.g. an I/O error).

@rustybird
Copy link

rustybird commented Oct 14, 2023

Since Linux errnos are all low numbers, why not use some range at the upper* end of the uint32_t filecopy error code for non-system errors. Without overloading ESOMETHING names at all. For example:

#define FILECOPY_ERROR_ABSOLUTE_SYMLINK (INT32_MAX - 1000 + 1) 
...
#define FILECOPY_ERROR_UNEXPECTED_EOF   (INT32_MAX - 1000 + 23)
...

* Edit: Changed UINT32_MAX to INT32_MAX for backwards compatiblity with old senders who would still pass these numbers through to strerror(), which expects an int. (glibc prints Unknown error <the integer> in this case.)

@jamke
Copy link

jamke commented Oct 15, 2023

cat ~/QubesIncoming/evil-qube/x > ~/QubesIncoming/evil-qube/y
# oops!  now ~/.profile has been overwritten by `x`, attacker wins
  1. Can you please elaborate, who runs this cat command? If some malicious software inside the qube, what stops it from doing this:
cat ~/anything_bad > ~/.profile
# oops!  now ~/.profile has been overwritten by `anything_bad`, attacker wins

Maybe I do not understand the scenario when preventing copying symlinks helps at all.

  1. @DemiMarie can you please tell, how will you personally copy dirs with scripts and symlinks now? Using tar was criticized above due to being outdated and insecure. What is the way Qubes OS provides then?

@sjbach sjbach changed the title qvm-copy: incorrect error message upon encountering broken symlink qvm-copy: incorrect error message upon encountering symlink pointing outside copy area Oct 15, 2023
@DemiMarie
Copy link

cat ~/QubesIncoming/evil-qube/x > ~/QubesIncoming/evil-qube/y
# oops!  now ~/.profile has been overwritten by `x`, attacker wins
  1. Can you please elaborate, who runs this cat command?

It’s meant as an example of something the user might do by accident. cd ~/QubesIncoming/someqube; cp a b would have the same effect, since cp will follow symbolic links to existing files.

Maybe I do not understand the scenario when preventing copying symlinks helps at all.

It’s meant to prevent the user from doing something insecure by accident. That’s why symbolic links that do not point outside of ~/QubesIncoming/VMNAME/x/ are allowed. The idea is that copying a directory tree should work, so long as every symlink in that tree points is relative, is reasonably named, and points somewhere else in that tree.

Copying a directory tree with symlinks that point outside of itself across VMs doesn’t make much sense, because such a directory tree is not self-contained: the meaning of these symlinks depends on the contents of other parts of the filesystem.

  1. @DemiMarie can you please tell, how will you personally copy dirs with scripts and symlinks now? Using tar was criticized above due to being outdated and insecure. What is the way Qubes OS provides then?

It depends on what the purpose of the copy is.

If I am copying an entire root filesystem, I will use tar, because qvm-copy cannot handle extended attributes, device nodes, named pipes, sockets, files owned by multiple users, absolute symlinks, hard links, and probably other stuff I don’t recall right now. Absolute symlinks worked before my hardening changes, but the rest did not, and I don’t expect that to change. It’s quite likely that the resulting filesystem would not boot because e.g. ownership or SELinux labels were wrong (among many other reasons).

If I am copying a directory tree with scripts, I would replace the absolute symbolic links with relative ones. This is good practice anyway, because it means that the directory tree will mean the same thing on the destination VM as it does on the source VM.

@jamke
Copy link

jamke commented Oct 15, 2023

It’s meant as an example of something the user might do by accident. cd ~/QubesIncoming/someqube; cp a b would have the same effect, since cp will follow symbolic links to existing files.

So, no malicious code is being limited in any way by this change. And if something malicious were running inside the qube, this change would have absolutely zero effect.

Preventing user from making possible rare mistake but putting them in a confusion why qvm-copy is not working properly and as user expects based on previous contract of qvm-copy implementation. Also it makes it work differently than cp, because it not dealing with symlinks now.

If I am copying a directory tree with scripts, I would replace the absolute symbolic links with relative ones.

And if you need to have absolute symbolic paths? I had such cases many times intentionally.
It is fine to use the relative symlinks to the top (..\..\) and absolute symlinks, nothing bad at all, this approach is used in GNU/Linux all over the place out of box.

And those symlinks will be breaking qvm-copy process now, if I understand correctly. And is probably will have no pre-check (right?) so the error will happen in the middle of 100 GiB copying process to make user suffer even more.

Is it possible to add flag at this point to allow copying symlinks "as is", in the way current qvm-copy and cp do it?

@marmarek
Copy link
Member

And if you need to have absolute symbolic paths? I had such cases many times intentionally.

Absolute symlinks make sense only in context of the filesystem they are placed in, if they point outside of the directory you are copying, the target file/dir (usually) won't exists in the target qube. And even if they point inside the directory you are copying, they will (usually) be broken, because the files are placed into a different location than the source.

There may be some rare and very specific cases where copying absolute symlinks to another qube make sense, but we won't make default less safe for everybody else just because of that. But having an alternative mechanism that is less strict and disabled by default is a possibility.

@DemiMarie
Copy link

cat ~/QubesIncoming/evil-qube/x > ~/QubesIncoming/evil-qube/y
# oops!  now ~/.profile has been overwritten by `x`, attacker wins
  1. Can you please elaborate, who runs this cat command?

It’s meant as an example of something the user might do by accident. cd ~/QubesIncoming/someqube; cp a b would have the same effect, since cp will follow symbolic links to existing files.

Maybe I do not understand the scenario when preventing copying symlinks helps at all.

It’s meant to prevent the user from doing something insecure by accident. That’s why symbolic links that do not point outside of ~/QubesIncoming/VMNAME/x/ are allowed. The idea is that copying a directory tree should work, so long as every symlink in that tree points is relative, is reasonably named, and points somewhere else in that tree.

Copying a directory tree with symlinks that point outside of itself across VMs doesn’t make much sense, because such a directory tree is not self-contained: the meaning of these symlinks depends on the contents of other parts of the filesystem.

  1. @DemiMarie can you please tell, how will you personally copy dirs with scripts and symlinks now? Using tar was criticized above due to being outdated and insecure. What is the way Qubes OS provides then?

It depends on what the purpose of the copy is.

If I am copying an entire root filesystem, I will use tar, because qvm-copy cannot handle extended attributes, device nodes, named pipes, sockets, files owned by multiple users, absolute symlinks, hard links, and probably other stuff I don’t recall right now. Absolute symlinks worked before my hardening changes, but the rest did not, and I don’t expect that to change. It’s quite likely that the resulting filesystem would not boot because e.g. ownership or SELinux labels were wrong (among many other reasons).

If I am copying a directory tree with scripts, I would replace the absolute symbolic links with relative ones. This is good practice anyway, because it means that the directory tree will mean the same thing on the destination VM as it does on the source VM.

@jamke
Copy link

jamke commented Oct 16, 2023

Absolute symlinks make sense only in context of the filesystem they are placed in, if they point outside of the directory you are copying, the target file/dir (usually) won't exists in the target qube. And even if they point inside the directory you are copying, they will (usually) be broken, because the files are placed into a different location than the source.

  1. I understand symlinks very well and use them intensively. Absolutes symlinks do work perfectly well when copied between qubes. Qubes share templates, so the hierarchy of many files is the same in the target qubes. Also many user's qubes commonly have similar directory structure (at least I do).
  2. Even if symlinks will point to a non-existent file in the target qubes, it does not make it useless. It keeps information about the location it was pointing to in the source qube, which is sometimes useful and is in fact user's data, that should not be lost.
  3. Broken symlink is not a problem, it can be in this state on purpose, e.g. mount something and this symlink will become valid and working. So, they are not actually broken, just pointing to currently inaccessible file.
    Broken copy utility is a problem, though.

Consider this use case #1:
/home/user/youtube-dl file, binary.
/home/user/onepiece/youtube-dl relative symlink to ../youtube-dl
/home/user/onepiece/download_and_update.sh file, script that uses youtube-dl
/home/user/naruto/youtube-dl relative symlink to ../youtube-dl
/home/user/naruto/download_and_update.sh file, script that uses youtube-dl

How should user copy naruto directory to other qube?
Is there actually any reason for qvm-copy to fail copying it?

Consider this use case #2:
The same, but symlinks to youtube-dl are absolute to some fixed user location like /home/user/youtube/youtube-dl. Using relative symlinks like ../youtube/youtube-dl will be a worse approach, because it will break the ability to move the directories like naruto and onepiece to other directories for no reason.


we won't make default less safe for everybody else just because of that.

I really do not think it increases security or safeness. No real case of increasing security except limiting user's actions:

  • Source qube cannot copy files and symlinks unless user explicitly selects target qube.
  • Target qube will not execute any command over any symlinks unless user manually execute any command.

To me it looks more as inability to copy symlinks properly anymore. At least it will affect me exclusively in this way.

But OK, I see that you and @DemiMarie look at this in a different way and made up your minds, so, I won't try to convince you any longer.

But having an alternative mechanism that is less strict and disabled by default is a possibility.

Yes, ability to copy files as is would be great.


@DemiMarie for some reason I see a duplicate reply from you, the same as previous one, not sure it was intentional.

@DemiMarie
Copy link

What is the actual task to be done here? I assume it is not just fixing the error message. @marmarek: would it be possible for you to update this issue to reflect what actually needs to be done, so that I know what to implement?

@marmarek
Copy link
Member

This issue is about the error message

@DemiMarie
Copy link

@marmarek I confused this with #8332.

DemiMarie added a commit to DemiMarie/qubes-linux-utils that referenced this issue May 8, 2024
The error message is not great, but at least it is not actively
misleading anymore.

Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker
and qfile-agent.
DemiMarie added a commit to DemiMarie/qubes-linux-utils that referenced this issue May 9, 2024
The error message is not great, but at least it is not actively
misleading anymore.

Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker
and qfile-agent.
DemiMarie added a commit to DemiMarie/qubes-linux-utils that referenced this issue May 12, 2024
The error message is not great, but at least it is not actively
misleading anymore.

Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker
and qfile-agent.
DemiMarie added a commit to DemiMarie/qubes-linux-utils that referenced this issue May 20, 2024
The error message is not great, but at least it is not actively
misleading anymore.

Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker
and qfile-agent.
@github-project-automation github-project-automation bot moved this from Ready to Done in Current team tasks Jun 16, 2024
marmarek pushed a commit to QubesOS/qubes-linux-utils that referenced this issue Jun 22, 2024
The error message is not great, but at least it is not actively
misleading anymore.

Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker
and qfile-agent.

(cherry picked from commit 34172df)
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: 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.
Projects
Archived in project
Development

No branches or pull requests

6 participants