Security fix in 0.5.8

[Geoff Schmidt]

Now that a week has passed, we are disclosing the security fix in 0.5.8.

The problem, discovered by @jan-glx, was that it was possible for an untrusted client to choose an arbitrary selector S and an arbitrary collection C, and determine whether there was at least one document in C that matched S. Essentially, a client could do this by trying to write to the collection and seeing if it got "access denied" or "no match." This is a problem because the client could then do a binary search on an arbitrary data field and determine its value over the course of a few dozen requests. For example, the client could ask if another user's Facebook secret was greater than or less than 'm', and continue bisecting to learn an additional bit of the other users' Facebook secret with every request. Conceptually, a similar attack should be possible on the _id attribute that would eventually recover all _ids in a collection, and then a dictionary attack could be performed on field names to ultimately read out all of the data in a collection, one bit at a time.

Our first thought was to simply remove the distinction between "access denied" and "no match." But this turns out to not be sufficient, since there is at least one other circumstance in which information could leak as a result of the client being able to choose an arbitrary selector. Suppose there is a document that has some secret attributes S that a particular client is not allowed to read, and that also has at least one attribute W that that client is able to write. Then even if the client can't distinguish between "access denied" and "no match," there is still a problem, because the client can update W conditional on selectors that look at the value of S, and again leak the value of S out through W one bit at at a time.

The fix was to remove the ability of untrusted clients to attempt updates or remove that could affect more than one document at once. This functionality was simply turning out to be more trouble than it was worth. Virtually nobody used it, it is easily replaced with a method call (which if you're doing a complex operation is better style anyway), and there was significant work left to do on it to secure it from DoS attacks where the client forces the server to perform an expensive database query to determine if a write is authorized.

So now, on the wire, the DDP method calls for update and remove take a single object id (though a selector of the form {_id: foo} is still accepted for now, for compatibility.) This significantly simplifies the API for allow and deny, which is nice. Of course you can still perform updates against arbitrary selectors from trusted code, which includes methods and anything on the server.

A bit about the timeline: @jan-glx reported the issue on GitHub at 4:55pm PDT on Tuesday the 12th. We shipped 0.5.8, which included the fix, at 2:50pm on the following day. We're proud of our ability to research the issue, write and test a fix, and finish the QA for 0.5.8 (originally scheduled for Thursday) in under 24 hours from the filing of the GitHub ticket. We individually notified all production Meteor users that we are aware of shortly before pushing the release, and we also instituted secu...@meteor.com. This address goes to the MDG pager rotation and is where all security issues should be reported in the future.

We encourage all sites to update if they have not already.

Geoff Schmidt

Meteor Development Group

[Ken Yee]

On Friday, March 22, 2013 1:21:49 AM UTC-4, Geoff Schmidt wrote:

So now, on the wire, the DDP method calls for update and remove take a single object id (though a selector of the form {_id: foo} is still accepted for now, for compatibility.)

Geoff:
Does this mean that the {_id: foo} will be removed eventually as well?
I've been using methods for updates only because it's habit from using other frameworks, but I've been meaning to explore updating collections from the client.  If {_id:foo} is removed, I'm not sure how you'd identify objects that need replacing.  It almost sounds like we shouldn't use _id in any of our web pages as identifiers either?
 

[David Greenspan]

Ken, you may have misunderstood.  You can still say `MyCollection.update(foo, ...)`, where `foo` is an _id string.  It's just the selector syntax with the curly braces that's deprecated but supported for back-compat.

-- David

--
You received this message because you are subscribed to the Google Groups "meteor-talk" group.
To unsubscribe from this group and stop receiving emails from it, send an email to meteor-talk...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Ken Yee]

On Friday, March 22, 2013 4:42:25 PM UTC-4, David Greenspan wrote:

Ken, you may have misunderstood.  You can still say `MyCollection.update(foo, ...)`, where `foo` is an _id string.  It's just the selector syntax with the curly braces that's deprecated but supported for back-compat.

Ahh...thanks.  Yes, that makes sense.
I'd suggest keeping the backwards compatibility syntax...otherwise, it's not quite mongodb syntax any more and people might get confused :-)

 

[Dan Dascalescu]

Background: SO question, GitHub issue.

On Thursday, March 21, 2013 10:21:49 PM UTC-7, Geoff Schmidt wrote:

The fix was to remove the ability of untrusted clients to attempt updates or remove that could affect more than one document at once. This functionality was simply turning out to be more trouble than it was worth.

There is however an unfortunate side effect to this:

 

Virtually nobody used it, it is easily replaced with a method call (which if you're doing a complex operation is better style anyway)

My use case is integrating UI widgets (e.g., DataTables, grids, reorderable lists, trees) with Meteor. These widgets take as a parameter the collection or cursor they'll display and operate on. As a result of user action, the widget may want to update multiple records at once.

Right now we have two solutions:

1) collection.find({...}).forEach and issue updates by _id. This is obviously far less efficient than a single update with {multi: true}.
2. Define a Meteor.method to update the collection on the server with {multi: true}. The problem is you'll have to pass a collection name as a parameter, and the server will have to look it up in the global namespace. This is what @mizzao's autocomplete does:

collection = global[collName]
...
collection.find(...)

So we're back at offering arbitrary access to server collections.

Worse, the recordset that's being modified on the client is a subset of the recordset on the server. Telling the server via Meteor.methods to update based on a selector may select more records than was intended on the client.

Also, the records on the client might have arrived from multiple collections. This makes it even more complicated to tell the server what to update.

Is there a good solution to this problem? It's important because currently Meteor doesn't have any good integrations with complex widgets such as grids or trees, and solving this would enable efficient modifications of multiple records at once.

[Maxime Quandalle]

Given the attack described in the original post, I believe an update where the selector is _id and some-other-criterias should be authorized in untrusted environments. This is usefull along with mongoDB $ to edit a denormalized sub-document using the sub-document id. The query is as follow:

Posts.update({
  _id: postId,
  'comments._id': commentId
}, {
  $set: {
    'comments.$.message': newMessage
  }
});
 

Am I right assuming that this query could be accepted in untrusted environments?

[Dave Workman]

The problem with allowing an update where the selector has the _id plus some other criteria is it allows them to start running updates to figure out data they shouldn't have access to. Example they could run this:

c.update({_id: 'some ID', 'some.secret.token': /^[a-m]/}, {$set: {'profile.matches': true}});

and if they get access denied back, they know 'some.secret.token' matches /^[a-m]/, if they don't they can change it and run another query, eventually figuring out the secret.


I proposed a change¹ to the way untrusted code is ran to fix this almost a year ago in this topic (at the very bottom). But no one seemed interested in having this feature.


¹ check that the doc exists with only _id before doing the allow/deny rules, that way the document would be denied regardless of whether the document matched:

'some.secret.token': /^[a-m]/