RPM Support

CVE/BZ https://bugzilla.redhat.com/show_bug.cgi?id=1598928

I guess one option might be to sanitize the manifest path before calling the join function:

manifest_path = '/../../var/lib/pulp/../pulp/alice'
real_manifest_path = os.path.realpath(manifest_path)
cwd = os.path.realpath(os.path.curdir)
common_prefix = os.path.commonprefix([cwd, real_manifest_path])
if common_prefix != cwd:
    raise Exception('Real manifest path outside of the current working directory %s' % real_manifest_path)
symlink_filename = os.path.join(cwd, real_manifest_path.lstrip(common_prefix))

I think this would allow for manifest paths to include some relative parts while making sure those won't escape out of current working directory of the worker performing a sync.
Another option might be to always plant the manifest path under the current working directory:

# any two real paths always share at least the root element
# so this would place all content under cwd like: `/home/milan/var/lib/pulp/alice`
symlink_filename = os.path.join(cwd, real_manifest_path.lstrip(common_prefix))

(I am the submitter of this security issue)

I think Pulp should be as conservative as possible:

1. Disallow any absolute path in upstream PULP_MANIFEST or on import. This has never worked (except in the maliciously constructed examples above). There is no point in supporting that use case.

2. Disallow relative path containing "." and "..": This would be the simplest solution. I don't see a use case were one would like such a thing in a published repo.

If 2 is not possible, because you fear that this breaks current use cases of Pulp users:

I think using "realpath" is not a good idea, since it traverses the file system and resolves symlinks (which may be outside of cwd). It might be possible to force Pulp into evaluating symlinks supplied by a malicious user or to detect the presence of paths on the system.

If relative paths containing "." and ".." are really necessary, I think we should use string based operations that do not operate on real files:

"some/file/../../../some" is invalid because there is a point in the path that would be "above" cwd.

"some/file/../different_location" must be normalized to "some/different_location" and this is the path pulp must use/store. IMHO, Pulp should never write paths with ".." or "." into PULP_MANIFEST files.

gmbnomis wrote:

(I am the submitter of this security issue)

I think Pulp should be as conservative as possible:

1. Disallow any absolute path in upstream PULP_MANIFEST or on import. This has never worked (except in the maliciously constructed examples above). There is no point in supporting that use case.

2. Disallow relative path containing "." and "..": This would be the simplest solution. I don't see a use case were one would like such a thing in a published repo.

If 2 is not possible, because you fear that this breaks current use cases of Pulp users:

I think using "realpath" is not a good idea, since it traverses the file system and resolves symlinks (which may be outside of cwd). It might be possible to force Pulp into evaluating symlinks supplied by a malicious user or to detect the presence of paths on the system.

didn't cross my mind, thanks!

If relative paths containing "." and ".." are really necessary, I think we should use string based operations that do not operate on real files:

"some/file/../../../some" is invalid because there is a point in the path that would be "above" cwd.

"some/file/../different_location" must be normalized to "some/different_location" and this is the path pulp must use/store. IMHO, Pulp should never write paths with ".." or "." into PULP_MANIFEST files.

I'm wondering though how to address this in a bugfix: would we simply fail the import of a PULP_MANIFEST that contained a path such as those above?

I think using "realpath" is not a good idea, since it traverses the file system and resolves symlinks (which may be outside of cwd). It might be possible to force Pulp into evaluating symlinks supplied by a malicious user or to detect the presence of paths on the system.

What is the problem in using realpath? AFAIK it is a standard solution to fix directory traversal issues, suggested also in the OpenStack project (https://security.openstack.org/guidelines/dg_using-file-paths.html). Other solutions seem a bit hand-made to me. Even if a symlink goes out of the cwd, that means the path should be rejected. Moreover, from the attacker point of view there's no difference in a path that exists but is outside of the target directory and a path that does not exist and is still outside of the target directory. Both of them would be rejected with the same exception.

The only issue I could think of is if the attacker is able to create a directory (to bypass the "is_valid" check) and then concurrently switch it to a symlink that points outside, so that when the path is actually used to write the file, it would overwrite files outside the intended dir.

Am I missing anything?

rschiron wrote:

I think using "realpath" is not a good idea, since it traverses the file system and resolves symlinks (which may be outside of cwd). It might be possible to force Pulp into evaluating symlinks supplied by a malicious user or to detect the presence of paths on the system.

What is the problem in using realpath? AFAIK it is a standard solution to fix directory traversal issues, suggested also in the OpenStack project (https://security.openstack.org/guidelines/dg_using-file-paths.html). Other solutions seem a bit hand-made to me.

The situation here is simpler than in the OpenStack example: The basic problem is that we have a root path and only relative paths of files relative to that root. Thus, there is simply no need to use realpath. Using e.g. normpath and abspath (which are purely string based) is enough. And that's also what the OpenStack example seems to imply, if one does not need to follow symlinks, just use `os.path.abspath(path).startswith(basedir)`

Btw. the Openstack example code is not fully correct: (one should be cautious using startswith with paths; seems a bit hand-made to me ;-) )

$ pwd
/home/me/tmp/real
$ python is_safe.py ../real/is_safe.py
safe: ../real/is_safe.py
$ python is_safe.py ../real2/is_safe.py
safe: ../real2/is_safe.py

(This may allow nice attacks if you can control creation of directories and just choose them to be a prefix of what you want to see. For example, in a structure similar to home directories)

The only issue I could think of is if the attacker is able to create a directory (to bypass the "is_valid" check) and then concurrently switch it to a symlink that points outside, so that when the path is actually used to write the file, it would overwrite files outside the intended dir.

Yes, that's possible, but probably hard to exploit. But the underlying thought is much simpler: There is no need to let user supplied paths poke around the file system in this case (because the user cannot provide absolute paths anyway and anything above the root path of the repository should be completely opaque to the user). Thus, just don't do it.