Refuse to extract symbolic links in archives that point outside the course directory.

[drgrice1]

Currently when extracting zip or tar archives in the file manager, any symbolic link in the archive is extracted assuming the link itself is in the course directory. That is a security vulnerability as a link could point to something like /etc and give unsecure access to system files.

So this refuses to extract symbolic links in archives that point outside the course directory. The way that this works is it actually does create the links, but then it prunes any links that point outside the course directory. This is so that realpath can be used, and is the only way to properly validate the links. Naively attempting to collapse .. instances in the path does not work reliably (see the note for the canonpath method at https://metacpan.org/pod/File::Spec::Unix), and I was able to subvert that to achieve a functional link to outside of the course directory. Note that broken symbolic links must also be rejected. This is because realpath does not work for these, and so there is no way to validate them as being links that point inside the course directory. In fact, it is possible to devise a broken symbolic link that combined with a valid symbolic link in another archive file, becomes a link to a location outside of the course directory (and I did so in testing).

Note this means that the only symbolic links allowed in a course are the required symbolic links (Library, Contrib, and Student_Orientation at this point), and those that are created by a system administrator. Also note that symbolic links in a course archive are still restored as before when unarchiving a course. It is considered the responsibility of the system administrator to validate links in a course archive.

This fixes the last of the security vulnerabilities that @Alex-Jordan found. Note that this was initially coded by Claude, but I largely rewrote what Claude created. Also note that this is a change in the allowed behavior for archive extraction via the file manager, but it is a necessary change to ensure system security.

[taniwallach]

Due to the security implications, I think this should be merged in before the release of WW 2.21.

I tested with with small test archives with one symbolic links pointing outside the course templated directory and one inside. The zip files was created with --symlinks (as in zip --symlinks links.zip badlink goodlink). For both zip and tgz formats, the "in course" link would be extracted (if it did not already exist) and the bad one was removed after creation as expected.

There is potentially a brief time where a bad symbolic link was created and remains accessible before it is deleted. I suspect that if the bad link is at the start of a very large archive file, the existence time of the bad link may sufficiently long to be potentially exploitable by triggering many read attempts of the link before and while the extraction is being done. This could be mitigated by running the "prune" call immediately after each link is created and collecting @unsafe_links in stages. I do not know if this is urgent enough to defer the current code from being merged.

I recommend considering whether it would be feasible to record the links which were requested and then deleted (name + intended target location) into some sort of log file. That would make it easier for an administrator to review links which could not be extracted.

[drgrice1]

Unfortunately, the links need to all exist at the same time for proper validation. The point is that a link could have another link in its target path. So to properly validate the links both need to exist.

[taniwallach]

Got it. This is as good as we can do.

[drgrice1]

I wouldn't say that this is as good as we can do. I would say that this is an attempt to still allow some complicated scenarios with symbolic links that might be out there, while removing the outright vulnerabilities.

We could not allow dependent links, i.e., links that have another link in their path. That is not something that I really have a use case for. It was just something that I observed with Claude's initial attempt at collapsing .. instances in a path that could be exploited. But if we simply don't allow dependent links, then links could be deleted immediately after testing.

I also had thought of the possible exploit with the links existing momentarily, but for now considered it a minor risk. Although, even with not particularly large archive files and an asynchronous scripted approach, it would probably be possible to, for example, read the /etc/passwd file.

[taniwallach]

No matter what is decided about dependent links - some links from existing archive files will no longer extract. If you are not aware of a good reason to allow dependent links - then by not allowing them and checking each link immediately after creation - the potential to exploit a link which briefly exists will be reduced. That would close the vulnerability as well as possible for now, while still permitting "in course" links to be extracted from archive files.

Essentially any link which can be created as a dependent link could also be (manually) created either in that manner, or be set up as a direct link to an allowed "in course" target in an additional archive file. So I think that people who need in course links which in the past were set up via dependent links can find fixes which do not depend on getting help from someone with console system level access on the server.

[somiaj]

My main use case is links at the admin/archive level, so I'm okay with being very restrictive of extracting links from the file manager when extracting archives. I would be okay with not allowing links at all from archives from instructors (though this might mean they need to work with their admins more) from the file manager. Though if just not allowing links to links makes things a bit safer, that might be a decent compromise.

[pstaabp]

I created a zip file starting in a course template with a symbolic link called to_etc that links to /etc and a regular file. Then deleted both.

When unzipping it, the symbolic link turned into a directory called to_etc. Looking at the code I think I was expecting a warning that the link was invalid and not be created.

[somiaj]

@pstaabp I'm unsure how well zips support symbolic links, and sometimes when creating the archive that is changed. It looks like you need to specific an option when creating the zip to preserve symbolic links, so I would look at your zip creation.

[somiaj]

Oh you used the "Make Archive" to create the .zip file, which appears to deal with links correctly. How did you create the to_etc link in the first place? Manually in the courses directory of your server?

[somiaj]

@pstaabp I tried to do what you did, and I got a warning about to_etc pointing outside the course directory or being broken, so might be something with how you created the link and/or zip.

[drgrice1]

One thing that would be a bit odd if I reworked this to not allow dependent links. Technically, it wouldn't do that. It would allow dependent links still, but whether or not the dependent link is allowed would depend on the order the links occur in the archive. If a link depends on another link and is after it in the archive, then it would still be allowed. But if it is before the link it depends on, it would not be allowed. This was actually the reason for the deferral of pruning links. The reason is that this uses the realpath method of Mojo::File, and if the link comes after the link it depends on, then realpath will resolve to the actual target on disk, and if that is in the course directory it will be allowed. But if it is before the dependent link, then it will be seen as a broken link, and will be pruned.

[pstaabp]

To clarify,

1. on the command line I made a link in [TMPL] to /etc.
2. I used "make archive" in the File Manager with this link and a regular file.
3. I deleted both in [TMPL]
4. Using "unpack archive", the regular file came back and the link came back as a file with a single line /etc.

Note: when I did an unzip on the file using the unzip command, then link came back.

So, we're not getting a link outside the course, but strange that I'm not seeing a warning.

[drgrice1]

@pstaabp: I think I did exactly what you said.

1. I made a link from the command line to /etc with ln -s /etc etc from the course templates directory.
2. I created an archive in the file manager selecting the link and another regular file and using the "Make Archive" button.
3. I deleted the link and the regular file.
4. I selected the zip file that was created in the file manager, and clicked "Unpack Archive".

The result was a message stating that the link in the archive points outside the course directory or is broken, and was not created, and another message stating that 1 file was extracted. I also now see the file again in the templates directory, and there is nothing for the link. No directory or symbolic link at all.

[somiaj]

Works for me. I too cannot reproduce @pstaabp issue.