Skip to content

Implications of permitting Symbols #94

Open
stouset opened this Issue Jan 31, 2013 · 16 comments

3 participants

@stouset
stouset commented Jan 31, 2013

Given the recent chain of parameter-parsing security vulnerabilities in Rails, is it a good idea to allow Symbols as acceptable scalar values?

@fxn
Ruby on Rails member
fxn commented Feb 1, 2013

The problem with symbols is that they are not garbage collected. Once you have created them, they stick around.

Rejecting symbols as unpermitted parameters is orthogonal to that I believe, no matter whether you permit it or reject it, the symbol has already been created anyway.

@stouset
stouset commented Feb 1, 2013

That's one issue with symbols. The other is that they're frequently used as special values when passed to methods (this is used heavily in ActiveRecord, for instance). Filtering out symbols won't prevent DOS attacks, but it can help prevent the types of SQL injection and code injection attacks that were reported in CVE-2013-0156.

While higher-level components of Rails should prevent any parameters from being parsed as symbols, obviously there have been oversights that have allowed them to make it through. Having overlapping security measures like filtering them out in this component can help limit the effectiveness of these types of exploits.

And really, since we don't expect any parameters to be parsed as symbols (for the reasons already mentioned: inability to be garbage collected, SQL injection, and code injection), why explicitly allow them here?

@fxn
Ruby on Rails member
fxn commented Feb 1, 2013

That's a good point. They are there to be able to handle ordinary XML payloads... let me summon granpa @nzkoz.

@NZKoz
Ruby on Rails member
NZKoz commented Feb 1, 2013

I'd personally not allow symbols, because there's essentially no legitimate reason for them to be in the kind of hash which strong_parameters is designed to handle.

However, you're right in saying that they've already been created at this point so you're mostly dead.

I would say that we shouldn't be permitting them as keys in hashes that are permitted. That will prevent most attacks which involve tricking you into calling extract_options and pulling out a user-created hash.

@stouset
stouset commented Feb 1, 2013

@nzkoz Is there any case where we would want them as a value?

I ask, because I suspect the easiest way to address this is to simply remove Symbol from the whitelist (which would prevent it from being either a key or a value). If there's no real solid need for symbols-as-values, that could be done as a one-line change. If you know of a case where they're necessary, though, another approach would be required.

@NZKoz
Ruby on Rails member
NZKoz commented Feb 1, 2013

I could conjur up crazy user code which relies on that, but I'd prefer we pushed that interning into their application so we forced them to pass a string, and convert it in their model.

Belt and suspenders and all that.

@stouset
stouset commented Feb 1, 2013

Ok. It sounded like you were advocating filtering symbols in keys, but permitting them as values.

@NZKoz
Ruby on Rails member
NZKoz commented Feb 1, 2013

No, to avoid confusion.

at present I'm only aware of attacks and types of attacks where symbols are allowed as keys to a Hash. However in an abundance of caution I'd exclude them as values to. This will be painful for users whose apps rely on that, but it should be painful as that's a pretty terrible idea.

@fxn
Ruby on Rails member
fxn commented Feb 1, 2013

Whitelisted hashes are recursively built with indifferent access, all keys should be strings.

@fxn
Ruby on Rails member
fxn commented Feb 1, 2013

The gem currently permits a dozen types, based on what the builtin deserializers support. Today in theory you could be sending XML payloads that use any of those types. And the server side may expect any of them, for example to round-trip non-AR models or whatever.

@stouset
stouset commented Feb 1, 2013

So there's a possible backwards-compatibility issue, but if people are relying on it they're almost inevitably vulnerable to a denial of service attack by leaking symbols (which this doesn't fix).

I'll submit a PR to fix. Should just be a single line.

@stouset
stouset commented Feb 1, 2013

Does it make sense to log the case when Symbols are passed and filtered, to alert the user that they're vulnerable to a DOS?

@stouset stouset added a commit that referenced this issue Feb 1, 2013
@stouset stouset Remove Symbol from whitelisted scalar types
Fixes issue #94. Symbols should never come in through params.
8d83a34
@fxn
Ruby on Rails member
fxn commented Feb 1, 2013

Why do you open a pull request if there is ongoing discussion here?

If you have whitelisted a symbol, the symbol is already in memory. You filtered it out didn't you? As I said before, leaking symbols and strong params are orthogonal topics.

I am closing the pull request to prevent its merge until there's an agreement here.

@stouset
stouset commented Feb 1, 2013

Apologies — I thought there was consensus. I didn't realize the pull request was premature. Leaking symbols and strong params are orthogonal, but that's not the issue here. Per @nzkoz,

there's essentially no legitimate reason for them to be in the kind of hash which strong_parameters is designed to handle

By filtering out symbols in strong_parameters, it can act as another layer of defense against SQL injection and code injection attacks made possible by flaws or oversights in the params-parsing layer. This doesn't fix (and indeed it can't) symbol leakage, but it can prevent more severe types of attacks.

@fxn
Ruby on Rails member
fxn commented Feb 1, 2013

I sympathize with the sentiment of being more restrictive with the default collection, but would like to address that in a more generic way rather than just blacklisting a type. As I said, symbols round-trip in XML for example, so people may have legit use cases for them. It is not a terrible idea a priori, it depends on your objects, and since that was supported you may be using it just fine.

@stouset
stouset commented Mar 11, 2013

People might have legitimate uses for round-tripping all sorts of objects which aren't safe for parsing as params. That doesn't mean the default should be to accept them. Symbols are exactly this kind of case: people should be forced to toggle a switch saying, "I am aware of the potentially hazardous consequences of this decision" before being able to use the feature.

Do you want people to be able to reenable Symbols on an app-level basis, or for only specific param uses?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.