OpenStack Object Storage (Swift)

[OSSA 2013-022] Possibly DoS attack using object tombstones (CVE-2013-4155)

Reported by Peter Portante on 2013-07-02
264
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Critical
Peter Portante
Folsom
Undecided
Unassigned
Grizzly
Undecided
Unassigned
OpenStack Security Advisory
Medium
Thierry Carrez

Bug Description

Is it possible for an attacker can fill a disk with old tombstones, slowing down object servers in the least with DELETE requests? I am not entirely sure I understand all of the DELETE behavior.

See the following patch which is a unit test which details the behavior (this currently fails on the last check because no metadata is pulled from the tombstone, so the container update is always performed).

diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py
index e3f24d6..27f75d9 100755
--- a/test/unit/obj/test_server.py
+++ b/test/unit/obj/test_server.py
@@ -1410,6 +1410,115 @@ class TestObjectController(unittest.TestCase):
             timestamp + '.ts')
         self.assert_(os.path.isfile(objfile))

+ def test_DELETE_container_updates(self):
+ # Test swift.object_server.ObjectController.DELETE and container
+ # updates, making sure container update is called in the correct
+ # state.
+ timestamp = normalize_timestamp(time())
+ req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
+ headers={
+ 'X-Timestamp': timestamp,
+ 'Content-Type': 'application/octet-stream',
+ 'Content-Length': '4',
+ })
+ req.body = 'test'
+ resp = self.object_controller.PUT(req)
+ self.assertEquals(resp.status_int, 201)
+
+ calls_made = [0]
+
+ def our_container_update(*args, **kwargs):
+ calls_made[0] += 1
+
+ orig_cu = self.object_controller.container_update
+ self.object_controller.container_update = our_container_update
+ try:
+ # The following request should return 204, but the object won't be
+ # truly deleted (container update is not performed) because the
+ # timestamp is old. A tombstone file should have been created with
+ # this timestamp.
+ #
+ # FIXME - When we know a newer file exists, should this not return
+ # HTTPConflict, and a tombstone not be created?
+ timestamp = normalize_timestamp(float(timestamp) - 1)
+ req = Request.blank('/sda1/p/a/c/o',
+ environ={'REQUEST_METHOD': 'DELETE'},
+ headers={'X-Timestamp': timestamp})
+ resp = self.object_controller.DELETE(req)
+ self.assertEquals(resp.status_int, 204)
+ objfile = os.path.join(self.testdir, 'sda1',
+ storage_directory(object_server.DATADIR, 'p',
+ hash_path('a', 'c', 'o')),
+ timestamp + '.ts')
+ self.assert_(os.path.isfile(objfile))
+ self.assertEquals(0, calls_made[0])
+
+ # The following request should return 204, and the object should
+ # be truly deleted (container update is performed) because this
+ # timestamp is newer. A tombstone file should have been created
+ # with this timestamp.
+ sleep(.00001)
+ timestamp = normalize_timestamp(time())
+ req = Request.blank('/sda1/p/a/c/o',
+ environ={'REQUEST_METHOD': 'DELETE'},
+ headers={'X-Timestamp': timestamp})
+ resp = self.object_controller.DELETE(req)
+ self.assertEquals(resp.status_int, 204)
+ objfile = os.path.join(self.testdir, 'sda1',
+ storage_directory(object_server.DATADIR, 'p',
+ hash_path('a', 'c', 'o')),
+ timestamp + '.ts')
+ self.assert_(os.path.isfile(objfile))
+ self.assertEquals(1, calls_made[0])
+
+ # The following request should return a 404, as the object should
+ # already have been deleted, but it should have also performed a
+ # container update because the timestamp is newer, and a tombstone
+ # file should also exist for this timestamp.
+ #
+ # FIXME - A malicious client can cause a set of object servers to
+ # update containers and fill the volume with .ts files as the code
+ # stands: if the file is already deleted, should this not just
+ # return a 404 and avoid all the updates and tombstones?
+ sleep(.00001)
+ timestamp = normalize_timestamp(time())
+ req = Request.blank('/sda1/p/a/c/o',
+ environ={'REQUEST_METHOD': 'DELETE'},
+ headers={'X-Timestamp': timestamp})
+ resp = self.object_controller.DELETE(req)
+ self.assertEquals(resp.status_int, 404)
+ objfile = os.path.join(self.testdir, 'sda1',
+ storage_directory(object_server.DATADIR, 'p',
+ hash_path('a', 'c', 'o')),
+ timestamp + '.ts')
+ self.assert_(os.path.isfile(objfile))
+ self.assertEquals(2, calls_made[0])
+
+
+ # The following request should return a 404, as the object should
+ # already have been deleted, and it should not have performed a
+ # container update because the timestamp is oler, though a
+ # tombstone file should also have been created with this
+ # timestamp.
+ #
+ # FIXME - A malicious client can cause an object server to fill up
+ # the volume with .ts files as the code stands: should we not
+ # allow new tombstone files to be create without older timestamps?
+ timestamp = normalize_timestamp(float(timestamp) - 1)
+ req = Request.blank('/sda1/p/a/c/o',
+ environ={'REQUEST_METHOD': 'DELETE'},
+ headers={'X-Timestamp': timestamp})
+ resp = self.object_controller.DELETE(req)
+ self.assertEquals(resp.status_int, 404)
+ objfile = os.path.join(self.testdir, 'sda1',
+ storage_directory(object_server.DATADIR, 'p',
+ hash_path('a', 'c', 'o')),
+ timestamp + '.ts')
+ self.assert_(os.path.isfile(objfile))
+ self.assertEquals(2, calls_made[0])
+ finally:
+ self.object_controller.container_update = orig_cu
+
     def test_call(self):
         """ Test swift.object_server.ObjectController.__call__ """
         inbuf = StringIO()

Jeremy Stanley (fungi) on 2013-07-02
Changed in ossa:
status: New → Incomplete

Here is a stab at a patch. I can run this through gerrit if appropriate.

John Dickinson (notmyname) wrote :

Peter, please don't submit a patch to gerrit until the bug has been confirmed and the patch reviewed in here. That way, once the patch has been submitted, we can very quickly merge it.

Yes, I won't submit the patch until given the go-ahead.

Samuel Merritt (torgomatic) wrote :

I'm going to try to paraphrase what goes on here in order to help me understand what's going on. Please correct me if I'm wrong.

Step 1: PUT object, X-Timestamp: t # works
Step 2: DELETE object, X-Timestamp: t+1 # deletes object, creates tombstone w/timestamp at t+1
Step 3: DELETE object, X-Timestamp: t-1 # creates tombstone w/timestamp at t-1

The bug is that Swift will create a tombstone with timestamp t-1 even though there's already a tombstone with a newer time, and so by spamming DELETE requests with timestamps (t-1, t-2, t-3, ...), a user can cause the creation of tons of 0-byte files.

Is that a reasonable restatement of this bug, or am I way off base here?

I think that is a reasonable statement here.

I believe the X-timestamp header is passed through by the proxy server, such that this is possible.

And note that when many files get created in the directory, the directory listing will required a lot of memory to perform os.listdir(), and such an operation on a large directory will starve out other requests (currently not wrapped in threadpool if I remember correctly).

clayg (clay-gerrard) wrote :

Yeah X-Timestamp comes through the proxy in part for container sync, it's useful and weird, but there it is.

So the risk is using up inodes on a single file system? If you can get this going on more objects than your estimate there are disks in the cluster you could ruin some poor ops day.

Won't a replication pass clear them out once you get the attacking account cut off?

I tried to write a patch in the object server's DELETE method that would only create the new timestamp if the object was deleted already and the already deleted object's orig_timestamp was larger than the requests timestamp - but all the branching made it look ugly to me.

I'm sort of inclined to just say:

    diff --git a/swift/obj/server.py b/swift/obj/server.py
    index 4bbbb4e..f1f486e 100644
    --- a/swift/obj/server.py
    +++ b/swift/obj/server.py
    @@ -996,7 +996,7 @@ class ObjectController(object):
                 return HTTPPreconditionFailed(
                     request=request,
                     body='X-If-Delete-At and X-Delete-At do not match')
    - orig_timestamp = disk_file.metadata.get('X-Timestamp')
    + orig_timestamp = int(disk_file.metadata.get('X-Timestamp', 0))
             if disk_file.is_deleted() or disk_file.is_expired():
                 response_class = HTTPNotFound
             metadata = {
    @@ -1007,7 +1007,7 @@ class ObjectController(object):
                 self.delete_at_update('DELETE', old_delete_at, account,
                                       container, obj, request, device)
             disk_file.put_metadata(metadata, tombstone=True)
    - disk_file.unlinkold(metadata['X-Timestamp'])
    + disk_file.unlinkold(max(int(metadata['X-Timestamp']), orig_timestamp)
             if not orig_timestamp or \
                     orig_timestamp < request.headers['x-timestamp']:
                 self.container_update(

First, can we confirm that this is an issue? I think it is, but I would like to have someone else go on record as agreeing that it is a problem. ;)

Second, the patch I posted above solves this problem. I am not sure Clay's is sufficient, because we still issue a container update when we don't have to, no?

Mike Barton (redbo) wrote :

It might be good for the object server and replicator to just share the code to remove old files, since there's a repetition of logic. Also, does anyone else think it's kind of gross to allow anyone to set X-Timestamp through the API just to allow container sync?

Thierry Carrez (ttx) wrote :

Swift experts: do you see this as a practical vulnerability ? I.e. are vulnerable configurations likely ? If yes, we'll probably have to bless the fix on this bug first, then run through the embargoed disclosure process (https://wiki.openstack.org/wiki/VulnerabilityManagement) before pushing it publicly.

clayg (clay-gerrard) wrote :

Yeah my patch was stupid, ignore my patch.

Samuel Merritt (torgomatic) wrote :

So, a few things:

First: after a replication pass, all the superfluous tombstones are cleaned up.

Second, I can pretty easily reproduce this with these commands:

    curl -v -X PUT -H "X-Timestamp: 1373004425" --data-binary "helloworld" -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj
    curl -v -X DELETE -H "X-Timestamp: 1373004424" -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj
    curl -v -X DELETE -H "X-Timestamp: 1373004423" -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj
    curl -v -X DELETE -H "X-Timestamp: 1373004422" -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj
    curl -v -X DELETE -H "X-Timestamp: 1373004421" -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj
    curl -v -X DELETE -H "X-Timestamp: 1373004420" -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj
    curl -v -X GET -H "X-Auth-Token: $TOKEN" http://192.168.22.2:8080/v1.0/AUTH_test/con/obj # gets "helloworld" as it should

Obviously, fill in your own Swift cluster's IP and auth token.

So, the bug is real. Is it a security vulnerability? I don't know.

Let's do some napkin math here. Let's say you can create bogus tombstones at a rate of 10 per second, and that a replication pass takes two days. That means you can get 86400 * 2 * 10 = 1728000 tombstones before they get cleaned up. Assuming that an os.listdir() returns strings that take 100 bytes of memory each (for reference counting, overhead, and whatever) and you get that an os.listdir() will require ~165 MiB to return its results. That's bad, but not the end of the world on decently-sized hardware, so I don't think that counts as a DoS attack. And that's worst-case, too... if your req/s is lower, or your replicators are faster, or whatever, then the memory usage goes down.

I'm not enough of an XFS expert to comment on the inode exhaustion problem.

Pete Zaitcev (zaitcev) wrote :

I was more afraid that enormous directories would boggle everything down. I know on ext4 we have "hashed" directories just for cases like these, not sure about XFS.

I think the memory usage is not the only consideration, but how long it takes to read and parse that many tombstones for os.listdir() and mind you, one has to sort them all as well.

I don't mind declassifying this as a security issue, I just did not want to do that without other folks input.

Who makes the call here?

And also consider that an attacker could fill just one object's directory, and then flood the system with GET requests, which will cause all servers to do that listdir/sort operation at the same time (starving out other requests, etc.).

Am I being paranoid or unrealistic?

John Dickinson (notmyname) wrote :

We make the call together on if this a security issue or not. (So "everybody and nobody", and that leaves me to make the final call).

On the one hand, this does seem like something that could be an issue for existing clusters (+1 to security issue)

One the other, threadpools completely mitigate this issue (-1 to security)

One the other other, threadpools aren't on by default (+0 for security?)

John Dickinson (notmyname) wrote :

Whoops. Further discovery has revealed that the listdir that matters (line 216 of obj/server.py) is not in a threadpool. This means threadpools don't help.

I vote for keeping this a security issue because it would allow an attacker to hang a storage node (as Peter described with DELETEs and GETs) or to cause replication and general cluster performance issues by creating so many filesystem objects.

John Dickinson (notmyname) wrote :

So my first thought is that PUTs would be vulnerable to this as well (create a lot of empty objects with older and older timestamps). However it's not. When given a PUT request, the proxy will first to a HEAD if x-timestamp was given on the request. And if the HEAD response is newer than the PUT x-timestamp value, the proxy short-circuits the response (line 865 in the object controller).

This check is not done on a DELETE. Adding it may make a better patch (????)

Changed in swift:
status: New → Confirmed
Samuel Merritt (torgomatic) wrote :

Okay, so this seems like it actually constitutes a DoS attack. Ugh.

How's this sound for a proposed fix:

Make it so the object server returns 409 Conflict if you try to make something with an X-Timestamp older than the new thingy on disk. Strictly speaking, this is probably the only bit that has to be done to fix the hole. That's DELETE, PUT, and POST.

This is basically what Peter's patch does, only for more verbs.

Sounds good. I'll add those verbs to the patch, and repost the patch here for review.

Patch commit message:

    Fix handling of DELETE obj reqs with old timestamp

    The DELETE object REST API was creating tombstone files with old
    timestamps, potentially filling up the disk, as well as sending
    container updates.

    Here we now make DELETEs with a request timestamp return a 409 (HTTP
    Conflict) if a data file exists with a newer timestamp, only creating
    tombstones if they have a newer timestamp.

    The key fix is to actually read the timestamp metadata from an
    existing tombstone file (thanks to Pete Zaitcev for catching this),
    and then only create tombstone files with newer timestamps.

    We also prevent PUT and POST operations using old timestamps as well.

Thierry Carrez (ttx) wrote :

I agree if there is a practical attack scenario we should issue an OSSA about this. The next steps are:
- review the attached patch on the bug, so that we can fast-track its approvals when we make everything public (swift-core team)
- propose and review a backport of the fix for Swift 1.8.0 (grizzly release), which I assume is affected as well (swift-core)
- produce an impact description (VMT team)
- review proposed impact description (everyone)
- request CVE (VMT team)
- coordinate public advisory release and patch merging (VMT team)

John: Will the advisory with the patch be enough, or are you going to want to do a 1.9.1 over this ?

Changed in ossa:
importance: Undecided → Medium
status: Incomplete → Confirmed
Mike Barton (redbo) wrote :

re-use get_hashes cleanup logic when writing new files in object server

Thierry Carrez (ttx) wrote :

Proposed impact description (will be used in CVE request and public advisory):

----------------------------------
Title: Swift Denial of Service using superfluous object tombstones
Reporter: Peter Portante
Products: Swift
Affects: All versions

Description:
Peter Portante reported a vulnerability in Swift. By issuing a lot of DELETE requests an authenticated attacker can fill an object server with superfluous object tombstones, which may significantly slow down subsequent requests to that object server, facilitating a Denial of Service attack against Swift clusters.
----------------------------------

@all: please check that the description is accurate.
@Peter: do you want us to additionally credit the company you work for, if any ?

Changed in ossa:
assignee: nobody → Thierry Carrez (ttx)
status: Confirmed → Triaged

@Thierry: yes, please, Red Hat, Inc.

John Dickinson (notmyname) wrote :

@Peter, please compare your work against that which Mike proposed

@Thierry, yes, we should roll a 1.9.1 with this patch when it lands.

I'd update the CVE description to the following (I'm sure it could be improved):

Peter Portante at Red Hat, Inc. reported a vulnerability in Swift. By issuing requests with an old X-Timestamp value, an authenticated attacker can fill an object server with superfluous object tombstones, which may significantly slow down subsequent requests to that object server, facilitating a Denial of Service attack against Swift clusters. The patch prevents this behavior by rejecting requests that would add older objects on disk.

@John, yes, I'll review and compare Mike's proposed patch today.

Mike Barton (redbo) wrote :

They're not mutually exclusive, and I can do mine as a regular change later. I'm good with Peter's idea of rejecting requests that have no effect. It'd probably be good from a end-user perspective as well.

But it's silly that we have a good version of "cleanup object directory" used mostly by the replicator and a shitty version in the object server. Just using the one good version would prevent these kinds of problems.

@John: I agree with Mike, it is not mutually exclusive, and can be applied later.

@Mike: have you thought about the implication that every PUT, POST and DELETE operation will list the directory twice with your patch? Once at DiskFile instantiation, and then again in the final_put() method? This might cause a general slow-down we should consider and measure. Let me know if I am not understanding the patch correctly.

Mike Barton (redbo) wrote :

I replaced the unlinkold call for PUT and DELETE, which was already doing the same listdir.

I'm adding cleanup logic to POST where it didn't exist before, but that might actually fix another DoS attack in this category, if it's writing new .meta files without checking for existing ones. I'll have to read through the code to see if that's the case.

@Mike: Isn't there still the os.listdir() call in the DiskFile constructor still?

Thierry Carrez (ttx) wrote :

CVE descriptions talk about the vulnerability, not the patch. Proposed final wording:

----------------------------------
Title: Swift Denial of Service using superfluous object tombstones
Reporter: Peter Portante (Red Hat)
Products: Swift
Affects: All versions

Description:
Peter Portante from Red Hat reported a vulnerability in Swift. By issuing requests with an old X-Timestamp value, an authenticated attacker can fill an object server with superfluous object tombstones, which may significantly slow down subsequent requests to that object server, facilitating a Denial of Service attack against Swift clusters.
----------------------------------

I'll request the CVE once we get nearer to an acceptable patch.

Mike Barton (redbo) wrote :

Yeah, we listdir the hash directory twice.

We probably don't need to do the first listdir for PUTs, POSTs and DELETEs, since we could just write the new file then do a cleanup op. But then I don't think we can do the other patch to reject operations that have no effect.

Or we could re-use the results of the first listdir to clean up, but that could leave multiple .data files laying around for concurrent PUTs.

Really, I'm not that excited about putting a lot of work into it, since the second listdir should generally be serviced by the page cache.

So can we confirm that we have an acceptable propose patch and follow-on work to cleanup a set of usage patterns?

Has anybody taken the proposed patch and verified it themselves?

Are we going to get this moving? Rebasing patches consumes developer time.

FWIW: I have a branch with the propose patch in one commit, with Mike's patch layered on top, and have been rebasing them as the tree moves forward.

Jeremy Stanley (fungi) wrote :

Since it looks like there hasn't been any subsequent feedback on Thierry's updated impact description in comment #32, just going on record to state that it seems accurate to me.

John Dickinson (notmyname) wrote :

Thierry's description seems accurate to me, too.

Vanilla patch rebased to c9de9f2b8da23538b82f23c724b482404d0ea916.

Thierry Carrez (ttx) wrote :

CVE requested

Changed in ossa:
status: Triaged → In Progress
John Dickinson (notmyname) wrote :

Peter's patch in #39 lgtm

John Dickinson (notmyname) wrote :

after we get a second core-dev approval on peter's patch, thierry and I will work together to get the swift 1.9.1 release out with this patch included

Latest patch applied to Master (Havana)

$ git log --oneline --decorate -10
e7bb33b (HEAD, bug-1196932) Fix handling of DELETE obj reqs with old timestamp
db2a478 (gerrit/master, master) Unify _commit_puts for accounts and containers
76f12c8 Merge "Ignore coverage HTML directory and MANIFEST."
898e344 Merge "remove assert syntax"
934cd9d Ignore coverage HTML directory and MANIFEST.
3e82858 Merge "Catch swob responses that are raised."
741c316 fix name 'recon_container' to 'rcache'
ac6ff8d Merge "Tempurl md use of split_path in _get_account"
bf95bb0 Merge "Corrected many style violations in the tests."
df7fc96 Catch swob responses that are raised.

Thierry Carrez (ttx) wrote :

Please get another +2 on the proposed patches.

Then the plan we agreed with John is to:
- define a public disclosure date
- send the patches as an embargoed disclosure to the distros and public cloud providers so that they coordinate fixes
- on public disclosure date, push the patch to the branches and cut a 1.9.1 RC (milestone-proposed branch)
- Publish 1.9.1 when you're all good with it

Samuel Merritt (torgomatic) wrote :

Code looks good; both unit and functional tests pass, and I can't make superfluous tombstones any more.

+2 from me.

$ git log --oneline --decorate -3
2e7de1a (HEAD, bug-1196932-redbo) *** Mike Barton's suggested patch for bug-1196932 ***
c248bd1 (bug-1196932) Fix handling of DELETE obj reqs with old timestamp
b7907f3 (gerrit/master, master) Merge "give value [] if disallowed_metadata_keys is None"
$

And here is MIke Barton's (redbo) additional patch which does not have to go downstream, but we should apply immediately as well.

$ git log --oneline --decorate -3
2e7de1a (HEAD, bug-1196932-redbo) *** Mike Barton's suggested patch for bug-1196932 ***
c248bd1 (bug-1196932) Fix handling of DELETE obj reqs with old timestamp
b7907f3 (gerrit/master, master) Merge "give value [] if disallowed_metadata_keys is None"
$

John Dickinson (notmyname) wrote :

lgtm.

@ttx, leooks good to go for downstream release

Thierry Carrez (ttx) wrote :

Patches sent to downstream stakeholders.
Proposed public disclosure date/time: Wednesday, August 7, 1500UTC.
We'll push public patches then and cut a 1.9.1 RC1 shortly after.

Changed in ossa:
status: In Progress → Fix Committed
Chuck Thier (cthier) wrote :

Did the CVE email include the patch for Havana? I was just asked by a package maintainer that said that the email sent didn't include a patch for that.

Jeremy Stanley (fungi) wrote :

The "havana" patch was attached as swift-master.patch (keep in mind that the havana stable release is still a couple months away).

Chuck Thier (cthier) wrote :

Do we have a planned date for when the patch will land in Master?

Thierry Carrez (ttx) wrote :

@Chuck: the patch will be proposed on review.o.o at the public disclosure date: Wednesday, August 7, 1500UTC

Then we'll need a swift-core member to fast-track it through review (based on the pre-approvals we've gathered on this bug) and it will land in master shortly afterwards.

Being 4 hours behind on the East Coast, I plan to be submitting the patch for review at 1100EDT. Just would like one other core member to double check the rebase that will have to occur.

Thierry Carrez (ttx) on 2013-08-07
information type: Private Security → Public Security
Thierry Carrez (ttx) wrote :
Changed in swift:
status: Confirmed → In Progress
Thierry Carrez (ttx) wrote :

OSSA 2013-022 is out, just waiting for final patch landing before closing the OSSA task

summary: - Possibly DoS attack using object tombstones
+ [OSSA 2013-022] Possibly DoS attack using object tombstones
+ (CVE-2013-4155)
Changed in ossa:
status: Fix Committed → Fix Released
status: Fix Released → Fix Committed
Thierry Carrez (ttx) on 2013-08-07
Changed in swift:
status: In Progress → Fix Committed
Thierry Carrez (ttx) wrote :

All patches in, thx everyone!

Changed in ossa:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-08-07
Changed in swift:
status: Fix Committed → Fix Released
To post a comment you must log in.