tag! implementation in Builder can cause inadvertent code to be executed

Can you supply a test or actual scenario to prove this?

Replying to manfred:

Can you supply a test or actual scenario to prove this?

try this in a builder view...

xml.tag!("test","test") # expected result would be <test>test</test> but results in crash

- or -

xml.tag!("initialize","test") # expected result would be <initialize>test</initialize> but results in the xmlbase::initialize getting re-called

Ok, I believe a programmer can crash her own program. Is there a practical situation where you would send user input directly to a builder object so that it would create a remote vulnerability? For instance, can you point me to a piece of code in Rails where this happens. I'm not convinced.

This is a very interesting edge case. Why should Builder resort to method_missing when it could have just easily created the tags with Strings?

Replying to manfred:

Ok, I believe a programmer can crash her own program. Is there a practical situation where you would send user input directly to a builder object so that it would create a remote vulnerability? For instance, can you point me to a piece of code in Rails where this happens. I'm not convinced.

I discovered this in a scenario where I am generating XML tags from database fields. Thus the logic is....

xml.tag!(dbrecord.tagname, dbrecord.tagvalue)

... which would seem like a fairly reasonable scenario.

Edge case? I disagree. This is simply a bug in Builder. You have tags (and nowhere in the docs that say not to use them) that will cause crashes or other such bugs - due to the internal implementation technique. The use of a tag named "test" is completely legitimate. It is not a case of whether *Rails* does this or not, this is a feature supplied in Rails to allow a view to output XML.

Also, in terms of sending user input directly to a Builder object, if one cannot do that, then it implies that every time we make any call to any Rails method, that we need to have safeguarded every case, instead of the method itself knowing what is valid input for its implementation. This is no different than a method not checking for buffer overrun or other security/attack situations. But further, in this case, it is a deficiency in Builder, because the purpose of the method tag! is to take the name to use for an XML element, and the value for that element, and output them into text - they should never be running those names as code in the first place. i.e. because that is an implementation trick you can do in Ruby, doesn't mean it should impact the caller's use of that method.

Can anyone explain why this happens:

>> x=Builder::XmlMarkup.new
=> inspect
>> x.test
=> "<inspect/><test/>"
>> x.__send__('test')
ArgumentError: wrong number of arguments
	from (irb):30:in `test'
	from (irb):30:in `__send__'
	from (irb):30

So calling x.test is not the same as calling x.send('test') ?

Actually the problem seems to be that even though XmlBuilder thinks it is creating a blank slate object Kernel methods still work on it. 'test' is a Kernel method. I tried, and it does not seem to be possible to "undef_method('test')" to hide that method, and since XmlBuilder uses send the method being private also does not help.

So basically if someone used XmlBuilder in a way that would allow clients to specify the tags:

>> x=Builder::XmlMarkup.new
=> inspect
>> x.tag!('system', 'echo 1')
1
=> true

Imagine that, but instad of 'echo 1' 'rm -rf'.

Can't see a solution that would allow keeping the current implementation, adding a check against using Kernel methods as tag names would artificially limit it, but actually supporting those names as tags would require an alternative implementation for them and then there would be little reason for not using that alternative implementation for other tags aswell.

This issue is also addressed in ticket #9294 where I have provide a diff with code solving the problem and test cases.