feat: prevent uploading files outside session directory via symlinks

[gahyusuh]

What was the problem/requirement? (What/Why)

A potential security threat was identified where an attacker could create a symbolic link within the Session working directory pointing to a file outside the directory, and potentially gain access to files that they should not have access to by identifying the symlink as an output file to be uploaded.

What was the solution? (How)

1. When identifying output files to be uploaded, we now use os.path.realpath() to get the true file location and ensures that it is within the the Session working directory.
2. Files are opened using the _open_file_binary() context manager before uploading, which prevents time-of-check to time-of-use attacks by holding the file handle during the upload, making it impossible to modify the symlink while the file is open.
3. Unit/integration tests were added to verify the correct handling of symbolic links pointing outside the Session working directory.

What is the impact of this change?

Ensures that the Job Attachment is secure against attacks involving symlinks that could lead to unauthorized file access or data leaks. It also adds an additional layer of protection against TOCTOU attacks.

How was this change tested?

• Added unit tests and an Integration test to cover the cases which involve symlinks pointing inside and outside the Session working directory.
• Manual testing was performed to ensure that legitimate output files are still uploaded correctly, while files outside the Session working directory pointed via symlinks are ignored.

Was this change documented?

Yes.

Is this a breaking change?

No.

[baxeaz]

I realize how close to the various deadlines we now are, it would be a larger change, and this change is certainly a big improvement, but is the correct general fix to instead have sync_outputs run as job-user?

[ddneilson]

I realize how close to the various deadlines we now are, it would be a larger change, and this change is certainly a big improvement, but is the correct general fix to instead have sync_outputs run as job-user?

You're right. That would be the most fool-proof way to fix these sorts of issues. It's a much bigger architectural change, though, so it'll have to be something for the future roadmap.

[jericht]

Looks good, just one minor thing to add if possible

[marofke]

Great work here Gahyun, thanks for putting in the extra time to get this right!

🚢 🐋

[ddneilson]

This is some great work. Thank you for your diligence navigating this complex and nuanced feature.

[jericht]

Suggested change

	if hasattr(os, "O_NOFOLLOW"):
	open_flags |= os.O_NOFOLLOW
	fd = os.open(path, open_flags)
	if sys.platform == "win32":
	if hasattr(os, "O_NOFOLLOW"):
	open_flags |= os.O_NOFOLLOW
	elif sys.platform != "win32":
	# We are on a non-Windows system that does not support O_NOFOLLOW
	# We cannot guarantee security here, so log a warning and reject the file
	logger.warning(f"Job Attachments does not support files referenced by symbolic links on this system ({sys.platform}). Please refrain from using symbolic links in Job Attachment asset roots and use real files instead. The following file will be skipped: {path}."
	return None
	fd = os.open(path, open_flags)
	if sys.platform == "win32":

To be addressed in a future PR. Let's get this one merged as-is to get the mitigations we already have in place

There's still a symlink attack risk for non-Windows systems that do not support O_NOFOLLOW. I think in this case, we should reject opening the file and log a message, since we are currently leaving the user open to a symlink attack on these systems.

[ddneilson]

Do we know which Windows versions don't?

[jericht]

Windows doesn't support O_NOFOLLOW, but we have a different mitigation in place that uses Win32 APIs that verify the file descriptor is actually pointing to the path we verified to be safe to open.

My comment is about the other non-Windows systems that don't support O_NOFOLLOW. We do not have any mitigations currently for those systems and are still leaving a symlink attack window open. This should be an edge case though, unless we get a bunch of users using such systems.

[jericht]

LGTM! Thanks for the patience and hard work on this one to make sure we get it right.