[OSSA 2014-034] Metadata constraints defined in openstack documents doen't match implementation (CVE-2014-7960)

I've added an incomplete security advisory task pending confirmation from Swift's core security reviewers and further information on the scope of impact.

In which version(s) of swift was this behavior observed?

I can confirm this for account and container level, but not for object level.

Whenever you do a POST with some metadata to an object, the existing metadata is replaced, thus if you POST two sets with 50 metadata entries you end up with an object with 50 metadatas (last update wins).

@Jeremy: my tests were against latest master. That said, after having a look at the source I think this affects all Swift versions (for the account and container level).

Thanks all! I've confirmed the security advisory task for this, on the assumption that any proposed solution could likely be backportable to currently supported stable releases. If this turns out not to be feasible after all, then we can revisit the decision.

Hi Christian,

The above case occurs in account and container only.

While in object case, the metadata is overwritten with new request's metadata. The old metadata is lost if we don't pass it in the new request.

There could be two possible ways to resolve this problem:
1. We can inherit the same behavior of metadata in case of account/container as of object i.e the metadata will be overwritten in every new request.

2. We can HEAD the account/container, then after confirming the constraints defined we can perform the required action:
    * If the combined metadata(new+old) complies to the constraint values, we will process the request.
    *If it doesn't comply to the constraints values, we will throw error.

Could you please suggest which solution would be better?

Thanks Rajaneesh for working on this!

To answer your questions: I would recommend not to chose option #1 because it changes the behaviour fundamentaly, and this will affect users that rely on this feature.
Option #2 sounds good to me. There is still a small possibility to store more metadata entries due to the eventual consistency, but I think this is the best we can get away with.

Thanks Christian for your prompt reply.

It means total size of metadata for account/container should be in range of 4096 Bytes with update (not overwrite case).

please confirm it.

One more query Christian:

In HEAD command we get the actual size of metadata and new request's metadata also have fixed size. Then, How could be the possibility to store more metadata entries due to the eventual consistency..??

I'm not yet clear on why this is a security issue. I agree that it's something that should probably be fixed.

Perhaps an attacker could fill up a drive, a few K at a time. But that wouldn't result in a DOS of the cluster.

@Rajaneesh: 4096 is the default size, but it might be different - depending on the setting in swift.conf.

If you execute the HEAD to get the current size of the metadata entry the request might use an older version of the account/container database with a lower number of metadata entries, but the actual metadata entry might be higher.

@John: agreed, it's not a serious security issue. But then I think it might be a good idea to fix this before making the bug public? Otherwise people might try to abuse the current behavior?

I talked with Same yesterday about this. Since the metadata blob is serialized in memory, this could cause memory exhaustion on that particular server. At best, this is a very low impact security issue.

The fix would be to do something like this:

1) accept the meta from the user and merge it with the existing metadata. If it's larger than something like eg 5x the total allowed size, reject the response with a 4xx series code.

2) with the new combined list of metadata entries, filter out all the old deleted ones, and if the resulting set is greater than the allowed max, reject the response with a 4xx series code.

This will keep the metadata bounded, but still allow for users to delete existing rows and keep updating metadata.

@John Dickinson :Thanks for your suggestion for its fix.
I have prepared patch. I will commit it after completing some test scenarios.

@Christian : Thanks for providing more clear picture about this problem and its solution.

@John: i'm still unconvinced this is a security issue. If I understand correctly, max_meta_overall_size limits the whole (key,value) space anyway so you can't just add that many of them ? Do you still think there is potential for DoS ?

Thierry: max_meta_overall_size limits the total metadata size for a given request, but it does not limit the total metadata size, so that can grow without bound. Since Swift loads that in memory all at once, there is potential for a DoS here.

(Note that I haven't tried it out yet, but based on some quick code reading, I believe the bug report to be correct.)

Ah, OK, I misread the doc ("max_meta_overall_size [is] the max number of bytes in the utf8 encoding of the metadata (keys + values"). I guess that can be abused, then (free storage!)

@Rajaneesh: could you attach the patch to this bug first ? We can review it locally on the private bug so that we limit the public vulnerability window.

Welp, so much for secrecy: https://review.openstack.org/#/c/121073/

Hi All,

I was not aware of the procedure for submitting patch for any kind of such security issues. I have already submitted a review patch for this bug.

I am attaching the patch although. Please have a look.

Sorry for the inconvenience caused.

Since the bug is now public, there's no need to attach patches to it and they can just be reviewed through the normal code review process instead.

In the future, please refrain from submitting changes for public review which mention private bugs (or any other disclosure of private bugs for that matter). Thanks, and apologies for any confusion.

Would be great to wrap up the review and include it in upcoming Swift release

Ok, I will commit new patch ASAP.

Proposed impact description follows. Rajaneesh, do you want to credit a specific company, in addition to your name ?

Title: Swift metadata constraints are not correctly enforced
Reporter: Rajaneesh Singh (?)
Products: Swift
Versions: up to 2.1.0

Description:
Rajaneesh Singh from ? reported a vulnerability in Swift enforcement of metadata contraints. By adding metadata in several separate calls, an authenticated attacker can bypass the max_meta_count constraint, potentially resulting in the storage of more metadata than allowed in configuration.

Reviewed: https://review.openstack.org/125360
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=5b2c27a5874c2b5b0a333e4955b03544f6a8119f
Submitter: Jenkins
Branch: master

commit 5b2c27a5874c2b5b0a333e4955b03544f6a8119f
Author: Richard (Rick) Hawkins <email address hidden>
Date: Wed Oct 1 09:37:47 2014 -0400

    Fix metadata overall limits bug

    Currently metadata limits are checked on a per request basis. If
    multiple requests are sent within the per request limits, it is
    possible to exceed the overall limits. This patch adds an overall
    metadata check to ensure that multiple requests to add metadata to
    an account/container will check overall limits before adding
    the additional metadata.

    Change-Id: Ib9401a4ee05a9cb737939541bd9b84e8dc239c70
    Closes-Bug: 1365350

Hi Christian Schwede, John Dickinson, Richard Hawkins,

I was wondering whether it is possible to submit a patch if someone else is assigned to.
I have been working on this for last one week and due to some reasons I was not able to submit new patch set.

But how is it possible that without any prior notification a new patch for this bug has already been merged into master branch?

@Rajaneesh Singh: Typically, people don't submit competing patches. That is, if person A is working on fixing a particular bug, person B will not also work on fixing that bug.

However, as you've seen here, there are exceptions. In this case, you turned this bug from a private security vulnerability, for which the fix could take a while without a problem, and turned into a public security vulnerability by publishing a patch to Gerrit, thus requiring the fix to be merged expeditiously. Now, the patch you submitted broke existing unit tests, functional tests, and even the pep8 checks. However, since you took no action on fixing the test failures for two weeks, and a release deadline was coming up, and the CVE was public, someone else had to step in and fix the bug properly. Here, Richard Hawkins stepped in and submitted an alternate patch for this security hole, and had he not done so, I would have.

Note that this was only necessary due to the broken embargo on this security vulnerability. As I said earlier, normally people stay out of each others' way.

Rajaneesh,

In general, we don't try to work on tasks that others have already claimed (eg a bug assigned). However, it happened in this case for a couple of different reasons.

First, last week we were wrapping up the dev work for Swift that would be included in the OpenStack Juno integrated release. Since the Juno release is time-based, we needed to get everything done last week. Since the patch had already been reported publicly and we hadn't head from you in many days, we wanted to ensure that it got included.

Secondly, last week many Swift devs (including Rick who wrote the merged patch), were together at a Swift hackathon. While there we wrapped up many patches that needed to be included in the release, including one to fix this bug you found.

Us developing this patch and merging it last week in no way is a reflection on you or your code. We simply needed to have a patch (with tests) merged last week, and we took advantage of sitting in the same room together.

Thank you for identifying the bug and starting on the patch. If you'd like to continue to stay involved in Swift's dev work (and I hope you do!), then join us in #openstack-swift on freenode IRC. You can also take a look at https://wiki.openstack.org/wiki/Swift/PriorityReviews for what we're currently prioritizing in reviews and https://wiki.openstack.org/wiki/Swift/ideas for a list of some smaller, more isolated ideas of new stuff to work on.

Anyway, for swift community it is fine and i am also part of this community.

But for the individual or new person (this was my first patch) it is not quite good. It really demotivates a person who was working on this from last couple of weeks. Due to some issue I could not submit new patch set.

I could commit if i know about deadline. I should have prior notification for this patch submission.

Thanks John Dickinson, Samuel Merritt for your clear and satisfactory reply.

Fix proposed to branch: feature/ec
Review: https://review.openstack.org/126595

disregard previous comment - something goofy with my git and it keeps tagging unrelated bugs for some reason

Reviewed: https://review.openstack.org/126595
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=06800cbe446ce4c937a57b69517b55c3bba9b6e1
Submitter: Jenkins
Branch: feature/ec

commit 7528f2b22169e90fe8ddd19b7ef7d46ecff5d231
Author: Christian Schwede <email address hidden>
Date: Mon Oct 6 10:01:03 2014 +0000

    Fix minor typo

    Fixes minor typo in one method and adds missing parameter in other
    method. Only checked swift/container/reconciler.py for now.

    Change-Id: I5c648010f09b6e4b1fb0380bc97b266e680602f8

commit 94fd95ba30c72fbcb03367aaa8da407a408948d5
Author: OpenStack Proposal Bot <email address hidden>
Date: Sat Oct 4 06:07:47 2014 +0000

    Imported Translations from Transifex

    Change-Id: I31b5e6b0f2922150902e1bfa52144302ee0c7a8e

commit d6a827792619f3343af07fc2519f4253fbdc67f7
Author: John Dickinson <email address hidden>
Date: Fri Oct 3 10:17:00 2014 -0400

    updated AUTHORS and CHANGELOG for 2.2.0

    Change-Id: I6c0bc1570f6a48439de5a029a86f1b582f30f8a6

commit 5b2c27a5874c2b5b0a333e4955b03544f6a8119f
Author: Richard (Rick) Hawkins <email address hidden>
Date: Wed Oct 1 09:37:47 2014 -0400

    Fix metadata overall limits bug

    Currently metadata limits are checked on a per request basis. If
    multiple requests are sent within the per request limits, it is
    possible to exceed the overall limits. This patch adds an overall
    metadata check to ensure that multiple requests to add metadata to
    an account/container will check overall limits before adding
    the additional metadata.

    Change-Id: Ib9401a4ee05a9cb737939541bd9b84e8dc239c70
    Closes-Bug: 1365350

commit 301a96f664d58b4ccad8e3cbf5d5a889cc76790f
Author: Jay S. Bryant <email address hidden>
Date: Tue Sep 30 15:08:59 2014 -0500

    Ensure sys.exit called in fork_child after exception

    Currently, the fork_child() function in auditor.py does not
    handle the case where run_audit() encounters an exception
    properly.

    A simple case is where the /srv directory is set
    with permissions such that the 'swift' user cannot access it.
    Such a situation causes a os.listdir() to return an OSError
    exception. When this happens the fork_child() process does not
    run to completion and sys.exit() is not executed. The process
    that was forked off continues to run as a result. Execution goes
    back up to the audit_loop function which restarts the whole process. The
    end result is an increasing number of processes on the system
    until the parent is terminated. This can quickly exhaust the
    process descriptors on a system.

    This change wraps run_audit() in a try block and adds an
    exception handler that prints what exception was encountered.
    The sys.exit() was moved to a finally: block so that it will
    always be run, avoiding the creation of zombies.

    Change-Id: I89d7cd27112445893852e62df857c3d5262c27b3
    Closes-bug: 1375348

commit 6d49cc3092168de6d22378557b2c37ea4063beeb
Author: Samuel Merritt <email address hidden>
Date: Thu Oct 2 17:14:58 2014 -0400

    Fix ring-builder crash.

    If you adjust ...

BAckport at https://review.openstack.org/#/c/126645/

Reviewed: https://review.openstack.org/126645
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=2c4622a28ea04e1c6b2382189b0a1f6cccdc9c0f
Submitter: Jenkins
Branch: stable/icehouse

commit 2c4622a28ea04e1c6b2382189b0a1f6cccdc9c0f
Author: Richard (Rick) Hawkins <email address hidden>
Date: Wed Oct 1 09:37:47 2014 -0400

    Fix metadata overall limits bug

    Currently metadata limits are checked on a per request basis. If
    multiple requests are sent within the per request limits, it is
    possible to exceed the overall limits. This patch adds an overall
    metadata check to ensure that multiple requests to add metadata to
    an account/container will check overall limits before adding
    the additional metadata.

    This is a backport to the stable/icehouse branch for commit SHA
    5b2c27a5874c2b5b0a333e4955b03544f6a8119f.

    Closes-Bug: 1365350

    Conflicts:
     swift/common/db.py
     swift/container/server.py

    Change-Id: Id9fca209c9c1216f1949de7108bbe332808f1045