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

Possible (Concealed) Creation of Files in Dangerous Path Locations #593

Closed
mgerstner opened this issue Sep 8, 2023 · 15 comments · Fixed by #697
Closed

Possible (Concealed) Creation of Files in Dangerous Path Locations #593

mgerstner opened this issue Sep 8, 2023 · 15 comments · Fixed by #697
Assignees
Labels

Comments

@mgerstner
Copy link

mgerstner commented Sep 8, 2023

If more than one file is transferred via Croc then, before the transfer starts, the receiver only sees a summary line about the files about to be received, like:

Accept 2 files (159 B)? (Y/n)

Only after confirming this dialog the full file reception list will be printed, like:

Receiving (<-[ip]:[port])
file1 100% |████████████████████| (140/140 B, 610 B/s) 1/2
file2 100% |████████████████████| ( 0/ 1 B) 2/2

The Croc protocol allows the sender to specify arbitrary paths to be transferred. Via social engineering an attacker could attempt to transfer one or more malicious files in a larger file list, that otherwise looks unsuspicious.

There is a file overwrite check in Croc that prevents existing files from being overwritten without user consent (at least by default, there's the --overwrite switch to disable the prompt). For not yet existing files there are no security boundaries though. So if e.g. $HOME/.ssh/authorized_keys is not existing yet on the receiver side, then the sender can transfer this, maybe unnoticed by the receiver. Even if the receiver notices it after the fact, it might be too late, and the attacker already had the chance to compromise the receiver's system.

Two relevant pieces of information might be missing for an attacker in this scenario: the receiver's home directory location and its current working directory. Guessing or determining the receiver's home directory path via social engineering might be well within reach though.imply implying that the CWD is the home directory might otherwise be a good guess. Also relative paths like ../.ssh/authorized_keys can be transferred. An attacker has to be careful about this, though, because if the path reaches above the home directory, then "permission denied" errors will become visible on the receiver end, which are more likely to raise alarm.

Fixing this kind of attack scenario is difficult, when trying to parse incoming file paths and to detect dangerous situations in userspace. Especially since there is also the possibility of symlinks and Croc even explicitly supports the creation of symlinks in its protocol. In other file transfer tools like Warpinator the developers ended up using isolation techniques to lock the receiver process side in a specific download directory.

Using mount namespaces (ideally an existing tool for creating a namespace jail) or Linux features like Landlock is probably the best solution for this problem. Since Croc is cross platform, implementing isolation can become a large effort though, since there are no shared APIs available for this.

@schollz
Copy link
Owner

schollz commented Sep 20, 2023

This seems pretty difficult and hard to implement cross-platform. I will leave it here in case anyone wants to take a crack at it, even a PR for just linux would be accepted.

@mgerstner
Copy link
Author

Mitre assigned CVE-2023-43619 to track this issue.

@aspann
Copy link

aspann commented Feb 3, 2024

Shouldn't CVE-2023-43619 cover 9.6.6 as well?

@attritionorg
Copy link

@aspann You are asking this on multiple tickets, when the original poster didn't even include an affected version. The advisories in question were written by GitHub proper, not the vendor. So take any advisory issues up with them first since they are making specific claims to versions affected (they won't see your comments most likely). Otherwise, please show why you feel that e.g. 9.6.6, or any other version, should or should not be included in the affected range. The owner of the project only spoke to it being difficult to solve cross-platform, they did not confirm or deny any versions being affected.

@mgerstner
Copy link
Author

Shouldn't CVE-2023-43619 cover 9.6.6 as well?

The GitHub advisory references the NVD entry for this issue, where the statement is included that versions through 9.6.5 are affected. At the time the CVEs have been filed the issues have not yet been fixed. Meanwhile a new release is available still without fixes. The CVE database has not been updated with this new information, or maybe they completely missed the fact that no fix is available currently.

Once a fix is available an update can be submitted to Mitre to note the actual version that has a fix available.

@schollz
Copy link
Owner

schollz commented Feb 5, 2024

@mgerstner looking back at this issue, I don't think its applicable.

relative paths like ../.ssh/authorized_keys can be transferred

this is actually not true. if A sends ../.ssh then B will receive it in their current folder, not in .. folder. there is only one way to overwrite ~./ssh and that is to tell the receiver to cd into ~ before they receive the file. this is not very subtle in terms of attacks and brings me to the basis of this issue -

Via social engineering an attacker could attempt

croc, by design, needs two people to communicate to transfer files. if you are suggesting that one of those two people could be an attacker - it is as possible. however, that is also true for pretty much any two people communicating with each other to run programs on their computers. A could ask B to run curl X | bash and B's computer is instantly compromised. or a myriad of other commands as well.

do curl and bash and wget and all those tools also have a responsibility to prevent a social engineering attack as well? I could easily design the same scenario that you suggest with those tools.

@mgerstner
Copy link
Author

@mgerstner looking back at this issue, I don't think its applicable.

relative paths like ../.ssh/authorized_keys can be transferred

this is actually not true. if A sends ../.ssh then B will receive it in their current folder, not in .. folder. there is only one way to overwrite ~./ssh and that is to tell the receiver to cd into ~ before they receive the file. this is not very subtle in terms of attacks and brings me to the basis of this issue -

I believe it is true. The paths communicated by the sender are not limited on the receiving side. I just tested it once more using this minor change to the code on the sender's side:

diff --git a/src/croc/croc.go b/src/croc/croc.go
index a4e23a1..94a75c4 100644
--- a/src/croc/croc.go
+++ b/src/croc/croc.go
@@ -451,7 +451,7 @@ func GetFilesInfo(fnames []string, zipfolder bool, ignoreGit bool) (filesInfo []
                } else {
                        fInfo := FileInfo{
                                Name:         stat.Name(),
-                               FolderRemote: "./",
+                               FolderRemote: "../",
                                FolderSource: filepath.Dir(absPath),
                                Size:         stat.Size(),
                                ModTime:      stat.ModTime(),

When receiving a file from this client then the download will happen in the folder ../.

Via social engineering an attacker could attempt

croc, by design, needs two people to communicate to transfer files. if you are suggesting that one of those two people could be an attacker - it is as possible. however, that is also true for pretty much any two people communicating with each other to run programs on their computers. A could ask B to run curl X | bash and B's computer is instantly compromised. or a myriad of other commands as well.

Of course I consider the scenario that the person I'm exchanging files with might have bad intentions. It's my job. If there would be no attacker then we could stop worrying alltogether, right?

do curl and bash and wget and all those tools also have a responsibility to prevent a social engineering attack as well? I could easily design the same scenario that you suggest with those tools.

The programs that you name here are general purpose tools used by experts. croc states about itself:

Easily and securely send things from one computer to another

A command line program these days can still be considered tailored at least for power users. However, the statement above, in my mind, gives a certain promise. Simplicity and security. Two things that are difficult to combine at times.

If the security that is provided here is only related to what happens on the wire i.e. the protection against a third party listening in or meddling on the network then I believe this restriction has to be documented more clearly. You should clearly state that you don't consider the scenario that the person sending data to you might be harmful. Because that is the main security problem to solve IMHO when looking at tools that allow to decentrally transfer data between arbitrary parties.

As an expert I'd say croc cannot be tasked with protecting me from the content that is transfered onto my computer, since it can be anything. But it should protect me from data appearing in uncontrolled places. There should be a clear and transparent workflow what files are received and where they are placed.

@FilippoBonazziSUSE
Copy link

A could ask B to run curl X | bash and B's computer is instantly compromised

A more apt comparison would be "A could ask B to run curl X, and curl would deceitfully save X in another folder". You see how that would be problematic, and would be classified as a vulnerability.

Users can hurt themselves with any tool of their choice. The point is whether the tool in question allows users to be hurt inadvertently or deceitfully.

@schollz
Copy link
Owner

schollz commented Feb 6, 2024

ah okay. I understand the bug now. seems like an easy fix.

@mgerstner I invited you to be a code collaborator of croc so it should be easy for you to make the fix.

@mgerstner
Copy link
Author

ah okay. I understand the bug now. seems like an easy fix.

@mgerstner I invited you to be a code collaborator of croc so it should be easy for you to make the fix.

I'm not quite sure what you mean. Are you suggesting that I should fix it?

More importantly I don't see this as an easy fix. Please refer to the original issue text. There are many ways a receiving can be tricked into creating files in unexpected places. Relative path components are one of them. Symlinks are another. ZIP file extraction is yet another.

@schollz
Copy link
Owner

schollz commented Feb 7, 2024

 you suggesting that I should fix it?

yes, I invited you to be a maintainer so you can take responsibility for the issues you raised.

I suggest start with relative paths, it is easy. zip shouldn't be too hard afterwards. simply check the outgoing path and sanitize them.

@mgerstner
Copy link
Author

you suggesting that I should fix it?

yes, I invited you to be a maintainer so you can take responsibility for the issues you raised.

I suggest start with relative paths, it is easy. zip shouldn't be too hard afterwards. simply check the outgoing path and sanitize them.

I can help out with small patches here and there but addressing a larger topic like this is outside the scope of my work. As I said before, to make this fully tight, isolation techniques will need to be employed.

@schollz
Copy link
Owner

schollz commented Feb 8, 2024

use whichever techniques you'd like to fix the issues you've raised! if you want me to review a pr Id be happy to, but feel free to make commits as you need.

@schollz
Copy link
Owner

schollz commented May 20, 2024

I can help out with small patches here

@mgerstner, do you need help with the patches? let me know if I can assist

@schollz
Copy link
Owner

schollz commented May 20, 2024

nvm, got it! #697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants