5 ms·

Rails XSS vulnerability in number formatting (CVE-2014-0081)

78 points by bensedat 13y ago

bensedat 13y ago
Also just wanted to mention: if you run a Rails app it's worth subscribing to the rubyonrails-security google group. Low traffic except for blasts like these to alert you to urgent patches.
hayksaakian 13y ago
cool helper functions, now that I want to use them, I'll need to update.
dave1010uk 13y ago
I'm not sure I understand this. Are the number helpers now escaping for a HTML context? Isn't it best practice to escape user input just before outputting it (so you know the context) rather than in every helper function? Disclaimer: not a RoR developer.
- riffraff 13y ago
  some helpers produce escaped output, some don't. Think: "link_to(name, url)" must produce html that must not be escaped again while "strip(text)" can produce unescaped text.
  - nightpool 13y ago
    Doesn't really affect this case though. Why does the numbers_to_ family need to produce HTML?
    - smileysteve 13y ago
      Currency, for one might include an html/utf8 symbol
    - wilg 13y ago
      As far as I know, it doesn't emit HTML. The Rails view layer uses a string subclass called SafeBuffer (http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/ http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/) to manage implicit escaping. I can't look at the diff for some reason (too many redirects), but I'd guess this issue was the result of some logic that was implemented before SafeBuffers were added.
  - dave1010uk 13y ago
    So why would a currency formatter generate HTML instead of plain text? That seems very strange to me. How does the helper know if I want to use the number in HTML, an email header or even a shell argument? Or are RoR helpers designed to be only usable directly with HTML? PHP's number_format() doesn't escape for HTML - is that an XSS bug too?
    - wilg 13y ago
      The view-dependent stuff is in fact separated from the business conversion logic and can be used in a non-web context if you want. Here's the version for use in a view context: https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/number_helper.rb#L107 https://github.com/rails/rails/blob/master/actionview/lib/ac... You can see that this method has some XSS prevention logic. Here's the converter itself: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/number_helper/number_to_currency_converter.rb https://github.com/rails/rails/blob/master/activesupport/lib...
dmix 13y ago
Looks like some rarely used view helpers. I doubt many of these are used in production apps. Hopefully no Bitcoin apps use the currency helper. But I imagine in the context of an exchange the numbers come from the blockchain or a wallet, and aren't user controlled in the way that could be exploited.
- jonknee 13y ago
  Why would they be rarely used? That's a very common task. A quick search on GitHub shows lots of likely uses: https://github.com/search?q=number_to_currency+in%3Afile+language%3Aruby&type=Code&ref=searchresults https://github.com/search?q=number_to_currency+in%3Afile+lan... https://github.com/search?q=number_to_human+in%3Afile+language%3Aruby&type=Code&ref=searchresults https://github.com/search?q=number_to_human+in%3Afile+langua... https://github.com/search?q=number_to_percentage+in%3Afile+language%3Aruby&type=Code&ref=searchresults https://github.com/search?q=number_to_percentage+in%3Afile+l...
- rst 13y ago
  Not at all rarely used. But it probably is somewhat rare to let users specify the strings which are supplied as, say, the ":units" argument to the formatting helper, which is where the vulnerability comes from. (If the user supplies a string which contains markup, and is passed as :units, :format, or :negative_format, it doesn't get escaped.)
- patio11 13y ago
  number_to_currency is used in three of three Rails apps which I know well enough to grep without grepping, though not in the form which is vulnerable here. I have an imperfect understanding of the development practices in the wider community, but my guess is that, if your app touches money, it probably uses it.
IceyEC 13y ago
the part that isn't being escaped seems to be things like the extra arguments, ie: if you want to format a param as a user defined format, the number would be properly escaped but the format wouldn't be
RailsFailsAgain 13y ago
Seriously - at what point to the stockholm syndrome fans of this trainwreck of a web framework stop making apologies for the appallingly consistent flow of security woes.
- hfghegheotqhgo 13y ago
  Actually, we are seeing a large number of new projects move off Rails to Erlang, Scala, Node and other platforms due to performance and security concerns. Less magic, more security.
  - matthewmacleod 13y ago
    That's stupid. We are seeing web development moving to other platforms - true. Part of this is down to a change in the typical requirements for a web app. Some of it is probably down to performance, although in most cases Rails performs well enough with an engineer who knows what they're doing. Almost none of that shift is down to security issues, of which there are few.
  - patrickdavey 13y ago
    Hmm... moving off "framework" to "language", "language", "language". No doubt you're a far better programmer than I am, but I sure as hell like using a framework with a bunch of very clever, very dedicated people maintaining and securing the framework I rely on... because I'm damn sure if I had to come up with the same functionality there'd be a heck of a lot of holes.
  - venus 13y ago
    If those projects have prospered enough that rewriting in a high-performance language has become a necessity, then I would say Rails fulfilled its function brilliantly. Rails is a good, probably the best, general purpose web framework, and no-one claims otherwise.
    - batiste 13y ago
      This comment is good, probably the best, general purpose comment, and no-one claims otherwise.
diziet 13y ago
I am not sure if I should feel happy or sad that we wrote internal javascript versions of these helper functions.
johne20 13y ago
off topic, but is it just me who cringes every time I see a google groups url? I know I am in for a 10 seconds wait to see any content.
grey-area 13y ago
So looking at this CVE, it probably won't affect many apps. You have to be using number_to_currency (common), but also passing in user input to the parameters format, negative_format or units which would not be a good idea anyway and is unlikely to have been done. I can't think why you would do that without verifying the unit and matching against a small set of acceptable currency units. You'd have to be doing this: <%= number_to_currency(number, format: unknown_user_text) %>

78 points by bensedat 13y ago

Also just wanted to mention: if you run a Rails app it's worth subscribing to the rubyonrails-security google group. Low traffic except for blasts like these to alert you to urgent patches.

hayksaakian 13y ago

cool helper functions, now that I want to use them, I'll need to update.

dave1010uk 13y ago

I'm not sure I understand this. Are the number helpers now escaping for a HTML context? Isn't it best practice to escape user input just before outputting it (so you know the context) rather than in every helper function? Disclaimer: not a RoR developer.

riffraff 13y ago
some helpers produce escaped output, some don't. Think: "link_to(name, url)" must produce html that must not be escaped again while "strip(text)" can produce unescaped text.
- nightpool 13y ago
  Doesn't really affect this case though. Why does the numbers_to_ family need to produce HTML?
  - smileysteve 13y ago
    Currency, for one might include an html/utf8 symbol
  - wilg 13y ago
    As far as I know, it doesn't emit HTML. The Rails view layer uses a string subclass called SafeBuffer (http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/ http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/) to manage implicit escaping. I can't look at the diff for some reason (too many redirects), but I'd guess this issue was the result of some logic that was implemented before SafeBuffers were added.
- dave1010uk 13y ago
  So why would a currency formatter generate HTML instead of plain text? That seems very strange to me. How does the helper know if I want to use the number in HTML, an email header or even a shell argument? Or are RoR helpers designed to be only usable directly with HTML? PHP's number_format() doesn't escape for HTML - is that an XSS bug too?
  - wilg 13y ago
    The view-dependent stuff is in fact separated from the business conversion logic and can be used in a non-web context if you want. Here's the version for use in a view context: https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/number_helper.rb#L107 https://github.com/rails/rails/blob/master/actionview/lib/ac... You can see that this method has some XSS prevention logic. Here's the converter itself: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/number_helper/number_to_currency_converter.rb https://github.com/rails/rails/blob/master/activesupport/lib...

dmix 13y ago

Looks like some rarely used view helpers. I doubt many of these are used in production apps. Hopefully no Bitcoin apps use the currency helper. But I imagine in the context of an exchange the numbers come from the blockchain or a wallet, and aren't user controlled in the way that could be exploited.

jonknee 13y ago
Why would they be rarely used? That's a very common task. A quick search on GitHub shows lots of likely uses: https://github.com/search?q=number_to_currency+in%3Afile+language%3Aruby&type=Code&ref=searchresults https://github.com/search?q=number_to_currency+in%3Afile+lan... https://github.com/search?q=number_to_human+in%3Afile+language%3Aruby&type=Code&ref=searchresults https://github.com/search?q=number_to_human+in%3Afile+langua... https://github.com/search?q=number_to_percentage+in%3Afile+language%3Aruby&type=Code&ref=searchresults https://github.com/search?q=number_to_percentage+in%3Afile+l...
rst 13y ago
Not at all rarely used. But it probably is somewhat rare to let users specify the strings which are supplied as, say, the ":units" argument to the formatting helper, which is where the vulnerability comes from. (If the user supplies a string which contains markup, and is passed as :units, :format, or :negative_format, it doesn't get escaped.)
patio11 13y ago
number_to_currency is used in three of three Rails apps which I know well enough to grep without grepping, though not in the form which is vulnerable here. I have an imperfect understanding of the development practices in the wider community, but my guess is that, if your app touches money, it probably uses it.

IceyEC 13y ago

the part that isn't being escaped seems to be things like the extra arguments, ie: if you want to format a param as a user defined format, the number would be properly escaped but the format wouldn't be

RailsFailsAgain 13y ago

Seriously - at what point to the stockholm syndrome fans of this trainwreck of a web framework stop making apologies for the appallingly consistent flow of security woes.

hfghegheotqhgo 13y ago
Actually, we are seeing a large number of new projects move off Rails to Erlang, Scala, Node and other platforms due to performance and security concerns. Less magic, more security.
- matthewmacleod 13y ago
  That's stupid. We are seeing web development moving to other platforms - true. Part of this is down to a change in the typical requirements for a web app. Some of it is probably down to performance, although in most cases Rails performs well enough with an engineer who knows what they're doing. Almost none of that shift is down to security issues, of which there are few.
- patrickdavey 13y ago
  Hmm... moving off "framework" to "language", "language", "language". No doubt you're a far better programmer than I am, but I sure as hell like using a framework with a bunch of very clever, very dedicated people maintaining and securing the framework I rely on... because I'm damn sure if I had to come up with the same functionality there'd be a heck of a lot of holes.
- venus 13y ago
  If those projects have prospered enough that rewriting in a high-performance language has become a necessity, then I would say Rails fulfilled its function brilliantly. Rails is a good, probably the best, general purpose web framework, and no-one claims otherwise.
  - batiste 13y ago
    This comment is good, probably the best, general purpose comment, and no-one claims otherwise.

diziet 13y ago

I am not sure if I should feel happy or sad that we wrote internal javascript versions of these helper functions.

johne20 13y ago

off topic, but is it just me who cringes every time I see a google groups url? I know I am in for a 10 seconds wait to see any content.

grey-area 13y ago

So looking at this CVE, it probably won't affect many apps. You have to be using number_to_currency (common), but also passing in user input to the parameters format, negative_format or units which would not be a good idea anyway and is unlikely to have been done. I can't think why you would do that without verifying the unit and matching against a small set of acceptable currency units. You'd have to be doing this: <%= number_to_currency(number, format: unknown_user_text) %>