Disallow all out-of-bounds symlinks in manifest repositories

[crenshaw-dev]

Summary

Manifest generation (and other repo-server actions like getting app details) should fail if any symlink in the repo points out of bounds (regardless of whether the symlink resolves).

Motivation

We've had some issues with symlink following in the repo server:

• Path traversal and dereference of symlinks when passing Helm value files
• Symlink following allows leaking out-of-bound manifests and JSON files from Argo CD repo-server
• Symlink following allows leaking out-of-bounds YAML files from Argo CD repo-server

It's likely that we've missed things. It's also possible that the patches aren't quite secure enough, because they are designed to prevent reading the contents of the destination, but not necessarily to prevent inferring things about the destination (like presence/absence and file type). That information can be sensitive since we're relying on randomized paths for traversal mitigation.

Finally, our code is not the only code involved. Since we call external binaries to operate on the repository contents, attackers may find ways to take advantage of behavior (buggy or otherwise) in those tools to traverse out of the repo.

Proposal

After each fetch, we should walk the entire repo contents and evaluate each symlink. If the symlink resolves to a location outside the repo root (regardless of whether there's anything at that location), we should fail the fetch operation so that the dependent operation fails. We should consider caching the success/failure to speed up future fetches of the same revision. We should also log a warning to help detect malicious symlinks.

We should identify every repo-related operation which does not perform a fetch before using the repo contents. Those operations should check the success/failure cache and then perform the symlink walk if the result isn't cached. The operation should fail and log a warning.

These checks should be disable-able by config. The config should be documented with a clear warning that disabling the check increases the risk of directory traversal attacks and should especially be avoided for Argo CD instances with multiple tenants.

This change should be made on a minor release (e.g. 2.5.0) and upgrade notes should clearly warn about the change.

[notfromstatefarm]

oh man, my force pushes really spammed the heck out of this issue. sorry!

[crenshaw-dev]

Haha no worries, GitHub real estate is cheap. :-)