Remove ability to parse symbols and yaml #34

Merged
merged 6 commits into from Jan 11, 2013

Conversation

Projects
None yet
5 participants
@nate
Contributor

nate commented Jan 10, 2013

These changes are pretty much the exact ones that were done in Rails with the addition of a fix to the contribution guide and the Rakefile around generating documentation. Fixes issue #33.

@@ -10,7 +10,7 @@ task :default => :spec
namespace :doc do
require 'yard'
YARD::Rake::YardocTask.new do |task|
- task.files = ['LICENSE.md', 'lib/**/*.rb']
+ task.files = ['lib/**/*.rb', '-', 'LICENSE.md']

This comment has been minimized.

@nate

nate Jan 10, 2013

Contributor

I was getting warnings:

[warn]: Syntax error in `license.md`:(1,18): syntax error, unexpected tinteger, expecting $end
[warn]: ParserSyntaxError: syntax error in `LICENSE.md`:(1,18): syntax error, unexpected tINTEGER, expecting $end
[warn]: Stack trace:
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:537:in `on_parse_error'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:50:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:50:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:15:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/source_parser.rb:439:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/source_parser.rb:44:in `block in parse'

The code says it should be formatted like this.

@nate

nate Jan 10, 2013

Contributor

I was getting warnings:

[warn]: Syntax error in `license.md`:(1,18): syntax error, unexpected tinteger, expecting $end
[warn]: ParserSyntaxError: syntax error in `LICENSE.md`:(1,18): syntax error, unexpected tINTEGER, expecting $end
[warn]: Stack trace:
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:537:in `on_parse_error'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:50:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:50:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/ruby/ruby_parser.rb:15:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/source_parser.rb:439:in `parse'
    /Users/nate/.rvm/gems/ruby-1.9.3-p327/gems/yard-0.8.3/lib/yard/parser/source_parser.rb:44:in `block in parse'

The code says it should be formatted like this.

@nate

This comment has been minimized.

Show comment
Hide comment
@nate

nate Jan 10, 2013

Contributor

Bumping this pull request. There are 91 gems that depend on multi_xml and I'm sure countless apps, it would be great if we could get this out there.

Contributor

nate commented Jan 10, 2013

Bumping this pull request. There are 91 gems that depend on multi_xml and I'm sure countless apps, it would be great if we could get this out there.

@acook

This comment has been minimized.

Show comment
Hide comment
@acook

acook Jan 11, 2013

This is kind of a big deal.

acook commented Jan 11, 2013

This is kind of a big deal.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 11, 2013

I'm prepping an advisor for Grape that depends on multi_xml. We need a release with this, please, I already have a working exploit with no workaround for existing apps.

dblock commented Jan 11, 2013

I'm prepping an advisor for Grape that depends on multi_xml. We need a release with this, please, I already have a working exploit with no workaround for existing apps.

@nate

This comment has been minimized.

Show comment
Hide comment
Contributor

nate commented Jan 11, 2013

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Jan 11, 2013

Owner

Overall, this looks good. What do you think about also removing lines 22 and 30 from https://github.com/sferik/multi_xml/blob/master/lib/multi_xml.rb? It seems like those are inherently unsafe.

Owner

sferik commented Jan 11, 2013

Overall, this looks good. What do you think about also removing lines 22 and 30 from https://github.com/sferik/multi_xml/blob/master/lib/multi_xml.rb? It seems like those are inherently unsafe.

@nate

This comment has been minimized.

Show comment
Hide comment
@nate

nate Jan 11, 2013

Contributor

I feel like they should be left in since they're only unsafe if you're parsing untrusted input. This pull requests makes parsing default to not trusting the xml and requires users to deliberately mark each and every instance of xml parsing as trusted if they want YAML parsing. This makes the library not only secure by default, but flexible if you really need it. Also, this mimics ActiveSupport's xml parsing and I think it would be nice to keep feature parity, if possible. What do you think?

It may be worth adding more docs around the fact that using allowing all types while parsing untrusted input is dangerous, and maybe even link to the CVE, or this pull request.

Contributor

nate commented Jan 11, 2013

I feel like they should be left in since they're only unsafe if you're parsing untrusted input. This pull requests makes parsing default to not trusting the xml and requires users to deliberately mark each and every instance of xml parsing as trusted if they want YAML parsing. This makes the library not only secure by default, but flexible if you really need it. Also, this mimics ActiveSupport's xml parsing and I think it would be nice to keep feature parity, if possible. What do you think?

It may be worth adding more docs around the fact that using allowing all types while parsing untrusted input is dangerous, and maybe even link to the CVE, or this pull request.

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Jan 11, 2013

Owner

Sounds good.

Owner

sferik commented Jan 11, 2013

Sounds good.

sferik added a commit that referenced this pull request Jan 11, 2013

Merge pull request #34 from nate/hotfix/remove-ability-to-parse-symbo…
…ls-and-yaml

Remove ability to parse symbols and yaml

@sferik sferik merged commit c94b136 into sferik:master Jan 11, 2013

1 check passed

default The Travis build passed
Details

@nate nate deleted the nate:hotfix/remove-ability-to-parse-symbols-and-yaml branch Jan 11, 2013

@nate

This comment has been minimized.

Show comment
Hide comment
@nate

nate Jan 11, 2013

Contributor

For those subscribed to this thread, a new version of multi_xml has been pushed with these fixes. Get it while it's hot!

https://rubygems.org/gems/multi_xml/versions/0.5.2

Thanks @sferik!

Contributor

nate commented Jan 11, 2013

For those subscribed to this thread, a new version of multi_xml has been pushed with these fixes. Get it while it's hot!

https://rubygems.org/gems/multi_xml/versions/0.5.2

Thanks @sferik!

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Jan 11, 2013

Owner

@nate: Thanks so much for submitting this patch.

Owner

sferik commented Jan 11, 2013

@nate: Thanks so much for submitting this patch.

@nate

This comment has been minimized.

Show comment
Hide comment
@nate

nate Jan 11, 2013

Contributor

Glad to help. :)

Contributor

nate commented Jan 11, 2013

Glad to help. :)

@reedloden

This comment has been minimized.

Show comment
Hide comment
@reedloden

reedloden Jan 11, 2013

This particular issue has been assigned CVE-2013-0175.

This particular issue has been assigned CVE-2013-0175.

@nate

This comment has been minimized.

Show comment
Hide comment

@universal universal referenced this pull request in errbit/errbit Jan 11, 2013

Merged

update multi_xml version to most recent one #356

@saulius saulius referenced this pull request in alphagov/trade-tariff-frontend Jan 11, 2013

Merged

Bump multi_xml to 0.5.2 #42

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 11, 2013

Thanks @sferik and @nate, committed into Grape, ruby-grape/grape@e15b7c3

dblock commented Jan 11, 2013

Thanks @sferik and @nate, committed into Grape, ruby-grape/grape@e15b7c3

@titanous titanous referenced this pull request in jnunemaker/httparty Jan 12, 2013

Merged

Bump multi_xml dependency to 0.5.2 for CVE-2013-0175 fix #181

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