Questions about the Prohibition of String Subclasses in responses

Questions about the Prohibition of String Subclasses in responses

[Michael Koziarski]

Hey Guys,

As I prepare to merge the new on-by-default XSS protection into rails
I'm bumping up against a constraint in rack, and I'm having trouble
figuring out why it's there.

http://github.com/chneukirchen/rack/blob/master/lib/rack/lint.rb#L465-475

Our XSS safe responses will fail this test because they return an
instance of ActionView::SafeBuffer which is a subclass of String.  We
use a subclass so that we can make all the concat and append
operations escape their arguments.

What's the rationale for preventing me from sending a subclass of
string?  Can we just change this stuff to use Object#kind_of? instead?

Cheers

Koz

Re: Questions about the Prohibition of String Subclasses in responses

[Eric Wong]

Michael Koziarski <koziarski@gmail.com> wrote:
> 
> Hey Guys,
> 
> As I prepare to merge the new on-by-default XSS protection into rails
> I'm bumping up against a constraint in rack, and I'm having trouble
> figuring out why it's there.
> 
> http://github.com/chneukirchen/rack/blob/master/lib/rack/lint.rb#L465-475
> 
> Our XSS safe responses will fail this test because they return an
> instance of ActionView::SafeBuffer which is a subclass of String.  We
> use a subclass so that we can make all the concat and append
> operations escape their arguments.
> 
> What's the rationale for preventing me from sending a subclass of
> string?

Not speaking for anyone else here, but this may break C extensions at
this point.  It was probably done this way to make extensions easier to
implement.  The core Ruby IO functions all call rb_obj_as_string() to
convert their arguments to strings, but some extensions out there may
not[1].


[1] *my* C extensions are safe against this, of course :)

-- 
Eric Wong

Re: Questions about the Prohibition of String Subclasses in responses

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Michael Koziarski <koziarski@gmail.com> wrote:
> > Our XSS safe responses will fail this test because they return an
> > instance of ActionView::SafeBuffer which is a subclass of String.  We
> > use a subclass so that we can make all the concat and append
> > operations escape their arguments.

What's the overhead of metadef-ing #concat/#append each String response
on an individual basis vs subclassing?  That could be an option...

Also (not knowing much about Rails or view internals), would it be
possible to make the response an Array-like object and then make all
concats/appends work on the Array-like object instead of the String-like
object?

  class SafeBuffer < Array

    def <<(string)
      super(escape(string))
    end

    ...

  end

And then probably do a #join when responding for performance reasons:

  [ status, headers, [ safe_buffer.join('') ] ]

> > What's the rationale for preventing me from sending a subclass of
> > string?
> 
> Not speaking for anyone else here, but this may break C extensions at
> this point.  It was probably done this way to make extensions easier to
> implement.  The core Ruby IO functions all call rb_obj_as_string() to
> convert their arguments to strings, but some extensions out there may
> not[1].
> 
> [1] *my* C extensions are safe against this, of course :)

But I just read the patch attached to LH ticket #78 and that relaxes
the Hash type check, too.  I actually depend on that being a real
Hash in at least C one extension :x

I'll of course update that extension for the new spec if I need to :>

-- 
Eric Wong

Re: Questions about the Prohibition of String Subclasses in responses

[Michael Koziarski]

> What's the overhead of metadef-ing #concat/#append each String response
> on an individual basis vs subclassing?  That could be an option...

That's not really an option, we're not going to work around rack by
dynamically extending every buffer used in every rails app :).

> Also (not knowing much about Rails or view internals), would it be
> possible to make the response an Array-like object and then make all
> concats/appends work on the Array-like object instead of the String-like
> object?
>
>  class SafeBuffer < Array
>
>    def <<(string)
>      super(escape(string))
>    end
>
>    ...
>
>  end

We could do this, however ERB uses .to_s EVERYWHERE and it's very
tricky to get anything but a string out the other end.  I'd rather not
go down that path if we can avoid it at all.

I guess fundamentally I'm still not seeing a reason for the current
restriction to no-child-classes beyond 'might break C-extensions'.
What kind of code is in those c extensions which might break? We could
verify our subclasses with that same kind of code and see what
happens.  The whole purpose of inheritance is to ensure subtypes can
be used interchangably, we've put some effort into making sure we're
'liskov-approved'[1] and I'd be surprised if anything breaks.

Having said that, I've been surprised before

> But I just read the patch attached to LH ticket #78 and that relaxes
> the Hash type check, too.  I actually depend on that being a real
> Hash in at least C one extension :x

I'm not too worried on the Hash front, but it still seems wrong to
make the assumption that consumer classes can't handle subtypes

> I'll of course update that extension for the new spec if I need to :>
>
> --
> Eric Wong
>

[1] http://en.wikipedia.org/wiki/Liskov_substitution_principle


-- 
Cheers

Koz

Re: Questions about the Prohibition of String Subclasses in responses

[Eric Wong]

Michael Koziarski <michael@koziarski.com> wrote:
> I guess fundamentally I'm still not seeing a reason for the current
> restriction to no-child-classes beyond 'might break C-extensions'.
> What kind of code is in those c extensions which might break? We could
> verify our subclasses with that same kind of code and see what
> happens.  The whole purpose of inheritance is to ensure subtypes can
> be used interchangably, we've put some effort into making sure we're
> 'liskov-approved'[1] and I'd be surprised if anything breaks.

Actually, I think the vast majority of subclasses are safe.

Very contrived, but only(?) subclasses implemented in C that redefine
their internal representation to something incompatible with their
(expected) superclass would get broken (when accessed by other C
extensions).

Fortunately I don't know of any extensions that crazy as there's no
point in subclassing at that point.  And if there are, maybe people
using them deserve to get their apps broken :>

> Having said that, I've been surprised before

Part of me wants to apply this change and stick it into a release just
to see what (if anything it) breaks.  Fortunately for the rest of you
I'm not part of the Rack core team :)

-- 
Eric Wong

Re: Questions about the Prohibition of String Subclasses in responses

[Michael Koziarski]

> Fortunately I don't know of any extensions that crazy as there's no
> point in subclassing at that point.  And if there are, maybe people
> using them deserve to get their apps broken :>

Seems very unlikely yeah.

>> Having said that, I've been surprised before
>
> Part of me wants to apply this change and stick it into a release just
> to see what (if anything it) breaks.  Fortunately for the rest of you
> I'm not part of the Rack core team :)

ditto on all points :)

-- 
Cheers

Koz

Re: Questions about the Prohibition of String Subclasses in responses

[James Tucker]

On 6 Oct 2009, at 07:40, Michael Koziarski wrote:

>
>> Fortunately I don't know of any extensions that crazy as there's no
>> point in subclassing at that point.  And if there are, maybe people
>> using them deserve to get their apps broken :>
>
> Seems very unlikely yeah.
>
>>> Having said that, I've been surprised before
>>
>> Part of me wants to apply this change and stick it into a release  
>> just
>> to see what (if anything it) breaks.  Fortunately for the rest of you
>> I'm not part of the Rack core team :)
>
> ditto on all points :)

I've just moved us over to kind_of in Lint from instance_of, in all  
cases of instance_of. Generally speaking the spec needs to ensure that  
the types supplied can be sent straight to IO.

I think if anyone uses a type to breach this, they should find the  
bugs appear in their own apps for reasonably well expected reasons,  
although most errors will show up in the handlers themselves.

I've also cherry picked for the 1.0.1 release.

>
> -- 
> Cheers
>
> Koz

Re: Questions about the Prohibition of String Subclasses in responses

[James Tucker]

On 6 Oct 2009, at 00:22, Eric Wong wrote:

>
> Eric Wong <normalperson@yhbt.net> wrote:
>> Michael Koziarski <koziarski@gmail.com> wrote:
>>> Our XSS safe responses will fail this test because they return an
>>> instance of ActionView::SafeBuffer which is a subclass of String.   
>>> We
>>> use a subclass so that we can make all the concat and append
>>> operations escape their arguments.
>
> What's the overhead of metadef-ing #concat/#append each String  
> response
> on an individual basis vs subclassing?  That could be an option...

Busting the method cache afaik.

>
> Also (not knowing much about Rails or view internals), would it be
> possible to make the response an Array-like object and then make all
> concats/appends work on the Array-like object instead of the String- 
> like
> object?
>
>  class SafeBuffer < Array
>
>    def <<(string)
>      super(escape(string))
>    end
>
>    ...
>
>  end
>
> And then probably do a #join when responding for performance reasons:
>
>  [ status, headers, [ safe_buffer.join('') ] ]
>
>>> What's the rationale for preventing me from sending a subclass of
>>> string?
>>
>> Not speaking for anyone else here, but this may break C extensions at
>> this point.  It was probably done this way to make extensions  
>> easier to
>> implement.  The core Ruby IO functions all call rb_obj_as_string() to
>> convert their arguments to strings, but some extensions out there may
>> not[1].
>>
>> [1] *my* C extensions are safe against this, of course :)
>
> But I just read the patch attached to LH ticket #78 and that relaxes
> the Hash type check, too.  I actually depend on that being a real
> Hash in at least C one extension :x
>
> I'll of course update that extension for the new spec if I need to :>
>
> -- 
> Eric Wong