Security vulnerability fixed - it is possible to cause the WHERE clause of a query to be omitted #15

Open
wants to merge 2 commits into
from

Projects

None yet

3 participants

@d11wtq
Contributor
d11wtq commented Jun 6, 2012

Rack parses some request value as [nil] (an array containing the value nil). When this value is used in a DataMapper query, DM executes the query without a where clause.

An example might be resetting the password of a user account for a given email:

User.first(:email => params[:email]).reset_password!

If the user crafts a request that Rack parses params[:email] to [nil], no WHERE clause is applied to the query (you would expect WHERE email IN (NULL). This becomes particularly bad with an #all query, which would simply return all records in the database.

The issue stems for the use of !value.any?, instead of value.empty? to check if the IN clause has no values in it. This is incorrect, since #any? returns false of the array contains only falsy values.

AFAIK, AR was affected by a similar bug that is now fixed: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=675429 and http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=675396

@d11wtq
Contributor
d11wtq commented Jun 6, 2012

Will write specs. I didn't think dm-do-adapter had any, since there is no spec directory, but @mbj has directed me to the shared spec that is run indirectly from another adapter's gem.

@d11wtq
Contributor
d11wtq commented Jun 6, 2012

Added specs. I think they exercise the same logic.

@dkubb
Member
dkubb commented Jun 6, 2012

@d11wtq thanks! this is merged with a few small tweaks. I'm running the tests now and will distribute dm-do-adapter 1.2.1 once the tests on the CI server pass.

Although I do think this brings up something we need to discuss on the mailing list, namely how we all handle vulnerability disclosures. Do we prefer full disclosure, or responsible disclosure? I don't see either changing how I personally deal with the problem, the patches will still come out as quickly. The community would probably be the best to decide.

The timing in this specific instance was good, but had it been a few days later I'd have been on vacation and the gem probably wouldn't have been released until early next week. It's true that @solnic could have released a hotfix in my absence, its still something to keep in mind. Maybe we need more people involved so there's less of a bottleneck.

@dkubb dkubb closed this Jun 6, 2012
@dkubb dkubb reopened this Jun 6, 2012
@dkubb
Member
dkubb commented Jun 6, 2012

Actually, I found one more problem with this that I'm fixing before releasing a patch. I thought the specs were working, but I tried one thing locally and they don't seem to exercise the code exactly as it's intended.

I'm working out a set of specs that do exercise the code properly, and then will figure out how to handle it.

@dkubb
Member
dkubb commented Jun 7, 2012

Ok, the work I'm doing on this can be tracked here: v1.2.0...release-1.2

@tpitale
Member
tpitale commented Feb 7, 2016

@d11wtq @dkubb I'd love to get this merged in. Would one of you mind updating against master and we can try to get this in 1.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment