Path Traversal possible when downloading an image

Bug #885167 reported by David on 2011-11-02
258
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
High
Thierry Carrez
Diablo
Undecided
Unassigned

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

CVE References

David (d--) on 2011-11-02
description: updated
David (d--) wrote :

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).

description: updated
Robert Clark (robert-clark) wrote :

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.

Changed in nova:
status: New → Confirmed
Changed in nova:
assignee: nobody → Robert Clark (robert-clark)
Thierry Carrez (ttx) wrote :

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 ?

Changed in nova:
assignee: Robert Clark (robert-clark) → nobody
importance: Undecided → High
Thierry Carrez (ttx) wrote :

Oops, concurrent change

Changed in nova:
assignee: nobody → Robert Clark (robert-clark)
David (d--) wrote :

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 ...).

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

Thierry Carrez (ttx) wrote :

@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) wrote :

The tarfile bug was filed separately as bug 894755.

Changed in nova:
status: Confirmed → Triaged
Robert Clark (robert-clark) wrote :

Vish, Please can you take a look at this.

Thanks

Changed in nova:
assignee: Robert Clark (robert-clark) → Vish Ishaya (vishvananda)
Vish Ishaya (vishvananda) wrote :

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) 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.

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) wrote :

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

Thierry Carrez (ttx) on 2011-12-01
Changed in nova:
assignee: Vish Ishaya (vishvananda) → Thierry Carrez (ttx)
status: Triaged → In Progress
Soren Hansen (soren) wrote :

Patch lgtm.

Thierry Carrez (ttx) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Soren Hansen (soren) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Thierry Carrez (ttx) wrote :

Assigned CVE-2011-4596

Thierry Carrez (ttx) on 2011-12-13
visibility: private → public

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

Changed in nova:
status: In Progress → Fix Committed

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) wrote :

Released as OSSA 2011-001

Thierry Carrez (ttx) on 2011-12-14
Changed in nova:
milestone: none → essex-2
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-04-05
Changed in nova:
milestone: essex-2 → 2012.1
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers

Patches

Bug attachments