Path Traversal possible when downloading an image

[David]

Because of #885165, it maybe possible for a remote attacker who can perform a man in the middle attack to provide a bucket with an image file-name which includes "/" and or "..". The path for the image file which is taken from the desired destination directory joined with the filename found in the bucket. This occurs in the static method _download_file. The _download_file method, as the name indicates also downloads the image to the respective file path.
The actual downloading of the image occurs via the key.get_contents_to_filename call. The get_contents_to_filename method will open a file at the 'local_filename' location through the following code( as found in boto/s3/key.py):

  def get_contents_to_filename(self, filename, headers=None,
      ...
     fp = open(filename, 'wb')

Which opens a new file object at the location provided. The _download_file method should ensure that the file-name is safe to use before calling the get_contents_to_filename method.

[0]
    @staticmethod
    def _download_file(bucket, filename, local_dir):
        key = bucket.get_key(filename)
        local_filename = os.path.join(local_dir, filename)
        key.get_contents_to_filename(local_filename)
        return local_filename

[David (d--)]

One more possible bug (I don't know if this is reachable) is that the tarfile.extractall method is used in the
 static method _untarzip_image. This method is also vulnerable to path traversal (as per the warning in the tarfile module documentation).

[Robert Clark (robert-clark)]

This isn't because of the other bug although the fact that it can be exploited when traffic is in the clear is significant.

It would seem to me that the main issue is that an attacker making this call (either a malicious user or a man-in-the-middle) can perform a path traversal because get_contents_to_filename doesn't validate strings correctly.

[Thierry Carrez (ttx)]

At the very least the filename retrieved from the manifest should be filtered through basename ?
That said I wonder if that code is even used since it contains a typo on line 215 (use of "ec2_utils") that should be hit with every manifest with a kernel id, and various other strange things (unused image_id and image_type fields). Vish ?

David: could you open a separate security bug so that we investigate the tarfile thing separately ?

[Thierry Carrez (ttx)]

Oops, concurrent change

[David (d--)]

Well I don't know if the tarfile bug is reachable. From what I saw the image would need to be signed?
(I can open another issue but I wasn't even sure if this code was actually used ...).

[Vish Ishaya (vishvananda)]

Thierry: The ec2_utils thing is definitely a bug. Not sure about the image_id. Checking with waldon

[Thierry Carrez (ttx)]

@DavidB: please open a bug about it, just in case, so that we can investigate further.
@Robert: Are you going to propose a change to Gerrit for this ?
@VMT: I suggest VMT Level = Medium, looks like it could be exploited by uploading a crafted manifest

[Thierry Carrez (ttx)]

The tarfile bug was filed separately as bug 894755.

[Robert Clark (robert-clark)]

Vish, Please can you take a look at this.

Thanks

[Vish Ishaya (vishvananda)]

Robert: what specifically do you want me to look at? This is definitely a bug. Were you going to propose a fix?

[Thierry Carrez (ttx)]

I think Robert is deferring to you for advice on who could do it. If nobody else picks it up, I can propose a fix.

[Vish Ishaya (vishvananda)]

I don't have someone specific in mind, but it looks like a pretty simple change.

Vish

On Nov 29, 2011, at 2:54 AM, Thierry Carrez wrote:

> I think Robert is deferring to you for advice on who could do it. If
> nobody else picks it up, I can propose a fix.
>
> --
> You received this bug notification because you are a member of Nova
> Core, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/885167
>
> Title:
> Path Traversal possible when downloading an image
>
> Status in OpenStack Compute (Nova):
> Triaged
>
> Bug description:
> Because of #885165, it maybe possible for a remote attacker who can perform a man in the middle attack to provide a bucket with an image file-name which includes "/" and or "..". The path for the image file which is taken from the desired destination directory joined with the filename found in the bucket. This occurs in the static method _download_file. The _download_file method, as the name indicates also downloads the image to the respective file path.
> The actual downloading of the image occurs via the key.get_contents_to_filename call. The get_contents_to_filename method will open a file at the 'local_filename' location through the following code( as found in boto/s3/key.py):
>
> def get_contents_to_filename(self, filename, headers=None,
> ...
> fp = open(filename, 'wb')
>
>
> Which opens a new file object at the location provided. The _download_file method should ensure that the file-name is safe to use before calling the get_contents_to_filename method.
>
>
>
> [0]
> @staticmethod
> def _download_file(bucket, filename, local_dir):
> key = bucket.get_key(filename)
> local_filename = os.path.join(local_dir, filename)
> key.get_contents_to_filename(local_filename)
> return local_filename
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/nova/+bug/885167/+subscriptions

[Thierry Carrez (ttx)]

Proposed fix, nova-core please pre-review before we make it public.
Adding stable maintainers. Once this is pre-approved we'll coordinate disclosure.

[Soren Hansen (soren)]

Patch lgtm.

[Thierry Carrez (ttx)]

Proposed advisory (merged with bug 894755):

Title: Path traversal issues registering malicious images using EC2 API
Impact: High
Announced: ...
Reporter: David Black
Products: Nova

Description:
David Black reported two issues in OpenStack Nova's support for EC2 RegisterImage action. By registering images from malicious tarballs or manifests, an attacker could potentially traverse directories and overwrite files with the rights of the user Nova runs under. Only setups allowing the EC2 API and the S3/RegisterImage method for registering images are affected.

[Mark McLoughlin (markmc)]

Issue looks valid here too - user-uploaded image manifest can't be trusted

The patch is fine, I think. But it might be worth considering an even safer option - use image.part.N as the filename, since we're downloading to a temporary directory we've just created

[Thierry Carrez (ttx)]

markmc: Your patch is better -- though for security patches the idea is to simplify the fix as much as you can (here you don't just weed out malicious filenames, you also change filenames for non-malicious files, which I suppose is OK, but maybe not).

[Vish Ishaya (vishvananda)]

I think marks patch is fine. It is a little bigger, but I do like ignoring the passed in name completely.

[Soren Hansen (soren)]

I don't have any particular preference about either approach, but only Thierry's patch comes with unit tests proving that it actually fixes the problem, so that has my vote so far.

[Mark McLoughlin (markmc)]

Hmm, I don't see unit tests? Am I missing something? I expect any unit test for this would work with my variant too anyway

I don't mind either way - we can go with the one-liner patch and add my version later

[Soren Hansen (soren)]

Mark, sorry, I was confused. I was thinking of Thierry's patch for bug 894755.

I think I prefer ttx's approach for the quick fix, and then we can go with yours from Essex and onwards.

[Thierry Carrez (ttx)]

Notification sent to downstream distros / public users.
Proposed disclosure date set to Tuesday, December 13, 2011, 1500UTC

[Thierry Carrez (ttx)]

Assigned CVE-2011-4596

[Openstack Gerrit (openstack-gerrit)]

Reviewed: https://review.openstack.org/2283
Committed: http://github.com/openstack/nova/commit/ad3241929ea00569c74505ed002208ce360c667e
Submitter: Jenkins
Branch: master

 status fixcommitted
 done

commit ad3241929ea00569c74505ed002208ce360c667e
Author: Thierry Carrez <email address hidden>
Date: Thu Dec 1 17:54:16 2011 +0100

    Sanitize EC2 manifests and image tarballs

    Prevent potential directory traversal with malicious EC2 image tarballs,
    by making sure the tarfile is safe before unpacking it. Fixes bug 894755

    Prevent potential directory traversal with malicious file names in
    EC2 image manifests. Fixes bug 885167

    Change-Id: If6109047307bd6e654ee9d1254f0d7f31cf741c1

[Openstack Gerrit (openstack-gerrit)]

Reviewed: https://review.openstack.org/2284
Committed: http://github.com/openstack/nova/commit/76363226bd8533256f7795bba358d7f4b8a6c9e6
Submitter: James E. Blair (<email address hidden>)
Branch: stable/diablo

 tag in-stable-diablo
 done

commit 76363226bd8533256f7795bba358d7f4b8a6c9e6
Author: Thierry Carrez <email address hidden>
Date: Thu Dec 1 17:54:16 2011 +0100

    Sanitize EC2 manifests and image tarballs

    Prevent potential directory traversal with malicious EC2 image tarballs,
    by making sure the tarfile is safe before unpacking it. Fixes bug 894755

    Prevent potential directory traversal with malicious file names in
    EC2 image manifests. Fixes bug 885167

    (cherry picked from commit ad3241929ea00569c74505ed002208ce360c667e)

    Change-Id: If6109047307bd6e654ee9d1254f0d7f31cf741c1

[Thierry Carrez (ttx)]

Released as OSSA 2011-001