Facebooker talk - Feb 2009 - Possible Security Hole in Facebooker -- Please Update!

Hi all ---

In the course of developing our Facebook connect app, we realized that there
was a  security hole in Facebooker that allows any malicious user to change
the state of the Facebooker module and crash any controller/view that uses
Facebooker to capture a Facebook session. For Facebook connect apps, this
could potentially be in any view that uses the "set_facebook_session"
before_filter.

All the malicious user has to do is send a malformed HTTP request similar
to:

http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned

The problem comes in the ''set_adapter'' method of
''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
attempt to load an adapter from the params hash if fb_sig_api_key is in the
request (ignoring the configuration found in the facebooker.yml file). In
this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
any subsequent call to Facebooker would try and use "you_are_pwned" as
the
api_key, causing it to crash the site.

Kevin Lochner''s already pushed an update to github, so update to the
latest
commit:

6a954874369354324d87b2fe09c24db4bd485faf
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf

Cheers,

Vince

----
Vincent Chu
Department of Applied Physics
Geballe Laboratory of Advanced Materials
McCullough Bldg. 318
476 Lomita Mall
Stanford, CA, 94305

Consider this:
"The smallest positive integer not definable in under eleven words."
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090224/97f76ad6/attachment.html>

Does this change simply remove support for multiple adapters?

Dave



On Tue, Feb 24, 2009 at 6:06 PM, vincent chu <vincentchu at gmail.com>
wrote:
> Hi all ---
>
> In the course of developing our Facebook connect app, we realized that
> there was a  security hole in Facebooker that allows any malicious user to
> change the state of the Facebooker module and crash any controller/view
that
> uses Facebooker to capture a Facebook session. For Facebook connect apps,
> this could potentially be in any view that uses the
"set_facebook_session"
> before_filter.
>
> All the malicious user has to do is send a malformed HTTP request similar
> to:
>
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>
> The problem comes in the ''set_adapter'' method of
> ''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
> attempt to load an adapter from the params hash if fb_sig_api_key is in the
> request (ignoring the configuration found in the facebooker.yml file). In
> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
> api_key, causing it to crash the site.
>
> Kevin Lochner''s already pushed an update to github, so update to
the latest
> commit:
>
> 6a954874369354324d87b2fe09c24db4bd485faf
>
>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>
> Cheers,
>
> Vince
>
> ----
> Vincent Chu
> Department of Applied Physics
> Geballe Laboratory of Advanced Materials
> McCullough Bldg. 318
> 476 Lomita Mall
> Stanford, CA, 94305
>
> Consider this:
> "The smallest positive integer not definable in under eleven
words."
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090224/4a96e022/attachment.html>

I forked the repo and fixed this issue without removing the functionality.

I sent a pull request from

http://github.com/digidigo/facebooker/tree/master

In the future I would appreciate a little more discretion around security
issues.  Publicizing it in this way required me to fix it immediately on my
production environment rather than being able to wait for morning.

Dave

On 2/24/09, David Clements <digidigo at gmail.com>
wrote:
>
> Does this change simply remove support for multiple adapters?
>
> Dave
>
>
>
> On Tue, Feb 24, 2009 at 6:06 PM, vincent chu <vincentchu at
gmail.com> wrote:
>
>> Hi all ---
>>
>> In the course of developing our Facebook connect app, we realized that
>> there was a  security hole in Facebooker that allows any malicious user
to
>> change the state of the Facebooker module and crash any controller/view
that
>> uses Facebooker to capture a Facebook session. For Facebook connect
apps,
>> this could potentially be in any view that uses the
"set_facebook_session"
>> before_filter.
>>
>> All the malicious user has to do is send a malformed HTTP request
similar
>> to:
>>
>> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>
>> The problem comes in the ''set_adapter'' method of
>> ''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
>> attempt to load an adapter from the params hash if fb_sig_api_key is in
the
>> request (ignoring the configuration found in the facebooker.yml file).
In
>> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
>> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
>> api_key, causing it to crash the site.
>>
>> Kevin Lochner''s already pushed an update to github, so update
to the
>> latest commit:
>>
>> 6a954874369354324d87b2fe09c24db4bd485faf
>>
>>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>>
>> Cheers,
>>
>> Vince
>>
>> ----
>> Vincent Chu
>> Department of Applied Physics
>> Geballe Laboratory of Advanced Materials
>> McCullough Bldg. 318
>> 476 Lomita Mall
>> Stanford, CA, 94305
>>
>> Consider this:
>> "The smallest positive integer not definable in under eleven
words."
>>
>> _______________________________________________
>> Facebooker-talk mailing list
>> Facebooker-talk at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090224/c29ddd6f/attachment.html>

How would you recommend this be handled? Vincent reported the issue  
privately last week and waited to publicly report it until a fix was  
in the main branch. It was my call to report it publicly now. Is there  
some way we can do this better?

Mike

On Feb 25, 2009, at 12:31 AM, David Clements wrote:
> I forked the repo and fixed this issue without removing the  
> functionality.
>
> I sent a pull request from
>
> http://github.com/digidigo/facebooker/tree/master
>
> In the future I would appreciate a little more discretion around  
> security issues.  Publicizing it in this way required me to fix it  
> immediately on my production environment rather than being able to  
> wait for morning.
>
> Dave
>
> On 2/24/09, David Clements <digidigo at gmail.com> wrote:
> Does this change simply remove support for multiple adapters?
>
> Dave
>
>
>
> On Tue, Feb 24, 2009 at 6:06 PM, vincent chu <vincentchu at
gmail.com>
> wrote:
> Hi all ---
>
> In the course of developing our Facebook connect app, we realized  
> that there was a  security hole in Facebooker that allows any  
> malicious user to change the state of the Facebooker module and  
> crash any controller/view that uses Facebooker to capture a Facebook  
> session. For Facebook connect apps, this could potentially be in any  
> view that uses the "set_facebook_session" before_filter.
>
> All the malicious user has to do is send a malformed HTTP request  
> similar to:
>
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>
> The problem comes in the ''set_adapter'' method of
''facebooker/lib/
> facebooker/rails/controller.rb'' where Facebooker will attempt to  
> load an adapter from the params hash if fb_sig_api_key is in the  
> request (ignoring the configuration found in the facebooker.yml  
> file). In this case, Facebooker would dutifully set the api_key to  
> "you_are_pwned" and any subsequent call to Facebooker would try
and
> use "you_are_pwned" as the api_key, causing it to crash the site.
>
> Kevin Lochner''s already pushed an update to github, so update to
the
> latest commit:
>
> 6a954874369354324d87b2fe09c24db4bd485faf
>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>
> Cheers,
>
> Vince
>
> ----
> Vincent Chu
> Department of Applied Physics
> Geballe Laboratory of Advanced Materials
> McCullough Bldg. 318
> 476 Lomita Mall
> Stanford, CA, 94305
>
> Consider this:
> "The smallest positive integer not definable in under eleven
words."
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
--
Mike Mangino
http://www.elevatedrails.com



-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/2917dba5/attachment-0001.html>

Sorry I was a little grumpy last night,  probably since I created the
security issue in the first place.

Not sure if I missed something like this but it would have helped me get on
top of it sooner if there was an email simply stating that there was a
security fix in the main branch.  Getting the email with the steps to
reproduce made it feel much more urgent to me.

This kinda hand holding is probably more important to me since I am
maintaining Facebook sites and not as active in development currently.  So I
am not watching what is going on in the branch.


What I should have said was,   Thanks for finding this and fixing it.  Sorry
about that.

Dave


On 2/25/09, Mike Mangino <mmangino at elevatedrails.com>
wrote:
>
> How would you recommend this be handled? Vincent reported the issue
> privately last week and waited to publicly report it until a fix was in the
> main branch. It was my call to report it publicly now. Is there some way we
> can do this better?
> Mike
>
> On Feb 25, 2009, at 12:31 AM, David Clements wrote:
>
> I forked the repo and fixed this issue without removing the functionality.
>
> I sent a pull request from
>
> http://github.com/digidigo/facebooker/tree/master
>
> In the future I would appreciate a little more discretion around security
> issues.  Publicizing it in this way required me to fix it immediately on my
> production environment rather than being able to wait for morning.
>
> Dave
>
> On 2/24/09, David Clements <digidigo at gmail.com> wrote:
>>
>> Does this change simply remove support for multiple adapters?
>>
>> Dave
>>
>>
>>
>> On Tue, Feb 24, 2009 at 6:06 PM, vincent chu <vincentchu at
gmail.com>wrote:
>>
>>> Hi all ---
>>>
>>> In the course of developing our Facebook connect app, we realized
that
>>> there was a  security hole in Facebooker that allows any malicious
user to
>>> change the state of the Facebooker module and crash any
controller/view that
>>> uses Facebooker to capture a Facebook session. For Facebook connect
apps,
>>> this could potentially be in any view that uses the
"set_facebook_session"
>>> before_filter.
>>>
>>> All the malicious user has to do is send a malformed HTTP request
similar
>>> to:
>>>
>>>
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>>
>>> The problem comes in the ''set_adapter'' method of
>>> ''facebooker/lib/facebooker/rails/controller.rb''
where Facebooker will
>>> attempt to load an adapter from the params hash if fb_sig_api_key
is in the
>>> request (ignoring the configuration found in the facebooker.yml
file). In
>>> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
>>> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
>>> api_key, causing it to crash the site.
>>>
>>> Kevin Lochner''s already pushed an update to github, so
update to the
>>> latest commit:
>>>
>>> 6a954874369354324d87b2fe09c24db4bd485faf
>>>
>>>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>>>
>>> Cheers,
>>>
>>> Vince
>>>
>>> ----
>>> Vincent Chu
>>> Department of Applied Physics
>>> Geballe Laboratory of Advanced Materials
>>> McCullough Bldg. 318
>>> 476 Lomita Mall
>>> Stanford, CA, 94305
>>>
>>> Consider this:
>>> "The smallest positive integer not definable in under eleven
words."
>>>
>>> _______________________________________________
>>> Facebooker-talk mailing list
>>> Facebooker-talk at rubyforge.org
>>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>>
>>>
>>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
>  --
> Mike Mangino
> http://www.elevatedrails.com
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/497ce083/attachment.html>

In case it got lost in my grumpiness last night.

The patch to fix this issue simply turned off adapter support.  Is that
correct?

I sent a pull request from my fork
http://github.com/digidigo/facebooker/tree/master  which should fix the
issue and preserve the behavior.  If anyone is using Facebooker to run
multiple apps or Bebo it would be great if you could check it out and make
sure that it didn''t break.

Thanks,

Dave

On 2/24/09, vincent chu <vincentchu at gmail.com>
wrote:
>
> Hi all ---
>
> In the course of developing our Facebook connect app, we realized that
> there was a  security hole in Facebooker that allows any malicious user to
> change the state of the Facebooker module and crash any controller/view
that
> uses Facebooker to capture a Facebook session. For Facebook connect apps,
> this could potentially be in any view that uses the
"set_facebook_session"
> before_filter.
>
> All the malicious user has to do is send a malformed HTTP request similar
> to:
>
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>
> The problem comes in the ''set_adapter'' method of
> ''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
> attempt to load an adapter from the params hash if fb_sig_api_key is in the
> request (ignoring the configuration found in the facebooker.yml file). In
> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
> api_key, causing it to crash the site.
>
> Kevin Lochner''s already pushed an update to github, so update to
the latest
> commit:
>
> 6a954874369354324d87b2fe09c24db4bd485faf
>
>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>
> Cheers,
>
> Vince
>
> ----
> Vincent Chu
> Department of Applied Physics
> Geballe Laboratory of Advanced Materials
> McCullough Bldg. 318
> 476 Lomita Mall
> Stanford, CA, 94305
>
> Consider this:
> "The smallest positive integer not definable in under eleven
words."
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/7762bd70/attachment.html>

I was short on time and unfamiliar with the code when I put the fix  
in, which is why I went with the nuclear option of removing the before  
filter.   I was a little surprised to see all tests passing with the  
before filter removed.

In addition to user-verification of the fix, we could use a test  
breaking the old version and working under david''s new patch.   
Unfortunately I''m low on spare cycles . . .

  - kevin

On Feb 25, 2009, at 11:00 AM, David Clements wrote:
> In case it got lost in my grumpiness last night.
>
> The patch to fix this issue simply turned off adapter support.  Is  
> that correct?
>
> I sent a pull request from my fork
http://github.com/digidigo/facebooker/tree/master
>   which should fix the issue and preserve the behavior.  If anyone  
> is using Facebooker to run multiple apps or Bebo it would be great  
> if you could check it out and make sure that it didn''t break.
>
> Thanks,
>
> Dave
>
> On 2/24/09, vincent chu <vincentchu at gmail.com> wrote:
> Hi all ---
>
> In the course of developing our Facebook connect app, we realized  
> that there was a  security hole in Facebooker that allows any  
> malicious user to change the state of the Facebooker module and  
> crash any controller/view that uses Facebooker to capture a Facebook  
> session. For Facebook connect apps, this could potentially be in any  
> view that uses the "set_facebook_session" before_filter.
>
> All the malicious user has to do is send a malformed HTTP request  
> similar to:
>
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>
> The problem comes in the ''set_adapter'' method of
''facebooker/lib/
> facebooker/rails/controller.rb'' where Facebooker will attempt to  
> load an adapter from the params hash if fb_sig_api_key is in the  
> request (ignoring the configuration found in the facebooker.yml  
> file). In this case, Facebooker would dutifully set the api_key to  
> "you_are_pwned" and any subsequent call to Facebooker would try
and
> use "you_are_pwned" as the api_key, causing it to crash the site.
>
> Kevin Lochner''s already pushed an update to github, so update to
the
> latest commit:
>
> 6a954874369354324d87b2fe09c24db4bd485faf
>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>
> Cheers,
>
> Vince
>
> ----
> Vincent Chu
> Department of Applied Physics
> Geballe Laboratory of Advanced Materials
> McCullough Bldg. 318
> 476 Lomita Mall
> Stanford, CA, 94305
>
> Consider this:
> "The smallest positive integer not definable in under eleven
words."
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/f19d7831/attachment-0001.html>

Hi David ---

I took a look at your fix. Though I''m somewhat unfamiliar with exactly
what
you want to do, I think it would be prudent to validate that the incoming
params hash actually originates from facebook before using the parameters to
reset the adapter. This way, you never touch the adapter until you''re
sure
that it''s Facebook sending the request, and not some malicious actor.

Cheers,

Vince
----
Vincent Chu
Department of Applied Physics
Geballe Laboratory of Advanced Materials
McCullough Bldg. 318
476 Lomita Mall
Stanford, CA, 94305


Consider this:
"The smallest positive integer not definable in under eleven words."


On Wed, Feb 25, 2009 at 9:02 AM, kevin lochner <klochner at gmail.com>
wrote:
> I was short on time and unfamiliar with the code when I put the fix in,
> which is why I went with the nuclear option of removing the before filter.
> I was a little surprised to see all tests passing with the before filter
> removed.
> In addition to user-verification of the fix, we could use a test breaking
> the old version and working under david''s new patch. 
Unfortunately I''m low
> on spare cycles . . .
>
>  - kevin
>
>
> On Feb 25, 2009, at 11:00 AM, David Clements wrote:
>
> In case it got lost in my grumpiness last night.
>
> The patch to fix this issue simply turned off adapter support.  Is that
> correct?
>
> I sent a pull request from my fork
> http://github.com/digidigo/facebooker/tree/master  which should fix the
> issue and preserve the behavior.  If anyone is using Facebooker to run
> multiple apps or Bebo it would be great if you could check it out and make
> sure that it didn''t break.
>
> Thanks,
>
> Dave
>
> On 2/24/09, vincent chu <vincentchu at gmail.com> wrote:
>>
>> Hi all ---
>>
>> In the course of developing our Facebook connect app, we realized that
>> there was a  security hole in Facebooker that allows any malicious user
to
>> change the state of the Facebooker module and crash any controller/view
that
>> uses Facebooker to capture a Facebook session. For Facebook connect
apps,
>> this could potentially be in any view that uses the
"set_facebook_session"
>> before_filter.
>>
>> All the malicious user has to do is send a malformed HTTP request
similar
>> to:
>>
>> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>
>> The problem comes in the ''set_adapter'' method of
>> ''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
>> attempt to load an adapter from the params hash if fb_sig_api_key is in
the
>> request (ignoring the configuration found in the facebooker.yml file).
In
>> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
>> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
>> api_key, causing it to crash the site.
>>
>> Kevin Lochner''s already pushed an update to github, so update
to the
>> latest commit:
>>
>> 6a954874369354324d87b2fe09c24db4bd485faf
>>
>>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>>
>> Cheers,
>>
>> Vince
>>
>> ----
>> Vincent Chu
>> Department of Applied Physics
>> Geballe Laboratory of Advanced Materials
>> McCullough Bldg. 318
>> 476 Lomita Mall
>> Stanford, CA, 94305
>>
>> Consider this:
>> "The smallest positive integer not definable in under eleven
words."
>>
>> _______________________________________________
>> Facebooker-talk mailing list
>> Facebooker-talk at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>
>>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/97334731/attachment.html>

Cool thanks for taking a look.  Looks like I can just add a condition to
that call

if request_comes_from_facebook?

I''ll try to get to it later today.

Dave

On 2/25/09, vincent chu <vincentchu at gmail.com>
wrote:
>
> Hi David ---
>
> I took a look at your fix. Though I''m somewhat unfamiliar with
exactly what
> you want to do, I think it would be prudent to validate that the incoming
> params hash actually originates from facebook before using the parameters
to
> reset the adapter. This way, you never touch the adapter until
you''re sure
> that it''s Facebook sending the request, and not some malicious
actor.
>
> Cheers,
>
> Vince
> ----
> Vincent Chu
> Department of Applied Physics
> Geballe Laboratory of Advanced Materials
> McCullough Bldg. 318
> 476 Lomita Mall
> Stanford, CA, 94305
>
>
> Consider this:
> "The smallest positive integer not definable in under eleven
words."
>
>
> On Wed, Feb 25, 2009 at 9:02 AM, kevin lochner <klochner at
gmail.com> wrote:
>
>> I was short on time and unfamiliar with the code when I put the fix in,
>> which is why I went with the nuclear option of removing the before
filter.
>> I was a little surprised to see all tests passing with the before
filter
>> removed.
>> In addition to user-verification of the fix, we could use a test
breaking
>> the old version and working under david''s new patch. 
Unfortunately I''m low
>> on spare cycles . . .
>>
>>  - kevin
>>
>>
>> On Feb 25, 2009, at 11:00 AM, David Clements wrote:
>>
>> In case it got lost in my grumpiness last night.
>>
>> The patch to fix this issue simply turned off adapter support.  Is that
>> correct?
>>
>> I sent a pull request from my fork
>> http://github.com/digidigo/facebooker/tree/master  which should fix the
>> issue and preserve the behavior.  If anyone is using Facebooker to run
>> multiple apps or Bebo it would be great if you could check it out and
make
>> sure that it didn''t break.
>>
>> Thanks,
>>
>> Dave
>>
>> On 2/24/09, vincent chu <vincentchu at gmail.com> wrote:
>>>
>>> Hi all ---
>>>
>>> In the course of developing our Facebook connect app, we realized
that
>>> there was a  security hole in Facebooker that allows any malicious
user to
>>> change the state of the Facebooker module and crash any
controller/view that
>>> uses Facebooker to capture a Facebook session. For Facebook connect
apps,
>>> this could potentially be in any view that uses the
"set_facebook_session"
>>> before_filter.
>>>
>>> All the malicious user has to do is send a malformed HTTP request
similar
>>> to:
>>>
>>>
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>>
>>> The problem comes in the ''set_adapter'' method of
>>> ''facebooker/lib/facebooker/rails/controller.rb''
where Facebooker will
>>> attempt to load an adapter from the params hash if fb_sig_api_key
is in the
>>> request (ignoring the configuration found in the facebooker.yml
file). In
>>> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
>>> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
>>> api_key, causing it to crash the site.
>>>
>>> Kevin Lochner''s already pushed an update to github, so
update to the
>>> latest commit:
>>>
>>> 6a954874369354324d87b2fe09c24db4bd485faf
>>>
>>>
http://github.com/mmangino/facebooker/commit/6a954874369354324d87b2fe09c24db4bd485faf
>>>
>>> Cheers,
>>>
>>> Vince
>>>
>>> ----
>>> Vincent Chu
>>> Department of Applied Physics
>>> Geballe Laboratory of Advanced Materials
>>> McCullough Bldg. 318
>>> 476 Lomita Mall
>>> Stanford, CA, 94305
>>>
>>> Consider this:
>>> "The smallest positive integer not definable in under eleven
words."
>>>
>>> _______________________________________________
>>> Facebooker-talk mailing list
>>> Facebooker-talk at rubyforge.org
>>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>>
>>>
>> _______________________________________________
>> Facebooker-talk mailing list
>> Facebooker-talk at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>
>>
>>
>> _______________________________________________
>> Facebooker-talk mailing list
>> Facebooker-talk at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/178641f7/attachment.html>

I''m not convinced we have a good solution yet.  here''s the
relevant
code:
>       def request_comes_from_facebook?
>         request_is_for_a_facebook_canvas? ||  
> request_is_facebook_ajax? || request_is_fb_ping?
>       end
>
>       def request_is_fb_ping?
>         !params[''fb_sig''].blank?
>       end
>
>       def request_is_for_a_facebook_canvas?
>         !params[''fb_sig_in_canvas''].blank?
>       end
>
>       def request_is_facebook_ajax?
>         params["fb_sig_is_mockajax"]=="1" ||  
> params["fb_sig_is_ajax"]=="1"
>       end
>
So calling request_comes_from_facebook isn''t really a security  
feature, and doesn''t change the behavior w.r.t. the test case that  
vince outlined, since they could just as easily fake params["fb_sig"]
>>
>> All the malicious user has to do is send a malformed HTTP request  
>> similar to:
>>
>> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
I think the before filter should be:

       def set_adapter
         Facebooker.load_adapter(facebook_params)  
if(params[:fb_sig_api_key])
       end

since facebook_params calls verify_signature & ensures the request  
comes from facebook.

Alternatively, we could be verify the params signature in  
request_comes_from_facebook.  That may be a better solution since the  
request_comes_from_facebook method is a little misleading (as david  
has just demonstrated).

Also, we throw an exception on a bad signature, so we should have a  
catch somewhere in the process.

- kevin




On Feb 25, 2009, at 1:10 PM, David Clements wrote:
> Cool thanks for taking a look.  Looks like I can just add a  
> condition to that call
>
> if request_comes_from_facebook?
>
> I''ll try to get to it later today.
>
> Dave
>
> On 2/25/09, vincent chu <vincentchu at gmail.com> wrote:
> Hi David ---
>
> I took a look at your fix. Though I''m somewhat unfamiliar with  
> exactly what you want to do, I think it would be prudent to validate  
> that the incoming params hash actually originates from facebook  
> before using the parameters to reset the adapter. This way, you  
> never touch the adapter until you''re sure that it''s
Facebook sending
> the request, and not some malicious actor.
>
> Cheers,
>
> Vince
> ----
>
>
>>
>>
>> On 2/24/09, vincent chu <vincentchu at gmail.com> wrote:
>> Hi all ---
>>
>> In the course of developing our Facebook connect app, we realized  
>> that there was a  security hole in Facebooker that allows any  
>> malicious user to change the state of the Facebooker module and  
>> crash any controller/view that uses Facebooker to capture a  
>> Facebook session. For Facebook connect apps, this could potentially  
>> be in any view that uses the "set_facebook_session"
before_filter.
>>
>> All the malicious user has to do is send a malformed HTTP request  
>> similar to:
>>
>> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>
>> The problem comes in the ''set_adapter'' method of
''facebooker/lib/
>> facebooker/rails/controller.rb'' where Facebooker will attempt
to
>> load an adapter from the params hash if fb_sig_api_key is in the  
>> request (ignoring the configuration found in the facebooker.yml  
>> file). In this case, Facebooker would dutifully set the api_key to  
>> "you_are_pwned" and any subsequent call to Facebooker would
try and
>> use "you_are_pwned" as the api_key, causing it to crash the
site.
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/4b9e5275/attachment-0001.html>

Do we need to know which adapter to use before we can verify the  
params? Don''t other implementation use different params?

Maybe we could require each adapter to verify that the request is  
valid. Would that fix this issue?

Mike


On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
> I''m not convinced we have a good solution yet.  here''s
the relevant
> code:
>
>>       def request_comes_from_facebook?
>>         request_is_for_a_facebook_canvas? ||  
>> request_is_facebook_ajax? || request_is_fb_ping?
>>       end
>>
>>       def request_is_fb_ping?
>>         !params[''fb_sig''].blank?
>>       end
>>
>>       def request_is_for_a_facebook_canvas?
>>         !params[''fb_sig_in_canvas''].blank?
>>       end
>>
>>       def request_is_facebook_ajax?
>>         params["fb_sig_is_mockajax"]=="1" ||  
>> params["fb_sig_is_ajax"]=="1"
>>       end
>>
>
> So calling request_comes_from_facebook isn''t really a security  
> feature, and doesn''t change the behavior w.r.t. the test case that
> vince outlined, since they could just as easily fake
params["fb_sig"]
>
>>>
>>> All the malicious user has to do is send a malformed HTTP request  
>>> similar to:
>>>
>>> http://my.rails.app.com/some_controller/? 
>>> fb_sig_api_key=you_are_pwned
>
> I think the before filter should be:
>
>       def set_adapter
>         Facebooker.load_adapter(facebook_params)  
> if(params[:fb_sig_api_key])
>       end
>
> since facebook_params calls verify_signature & ensures the request  
> comes from facebook.
>
> Alternatively, we could be verify the params signature in  
> request_comes_from_facebook.  That may be a better solution since  
> the request_comes_from_facebook method is a little misleading (as  
> david has just demonstrated).
>
> Also, we throw an exception on a bad signature, so we should have a  
> catch somewhere in the process.
>
> - kevin
>
>
>
>
> On Feb 25, 2009, at 1:10 PM, David Clements wrote:
>
>> Cool thanks for taking a look.  Looks like I can just add a  
>> condition to that call
>>
>> if request_comes_from_facebook?
>>
>> I''ll try to get to it later today.
>>
>> Dave
>>
>> On 2/25/09, vincent chu <vincentchu at gmail.com> wrote:
>> Hi David ---
>>
>> I took a look at your fix. Though I''m somewhat unfamiliar with
>> exactly what you want to do, I think it would be prudent to  
>> validate that the incoming params hash actually originates from  
>> facebook before using the parameters to reset the adapter. This  
>> way, you never touch the adapter until you''re sure that
it''s
>> Facebook sending the request, and not some malicious actor.
>>
>> Cheers,
>>
>> Vince
>> ----
>>
>>
>>>
>>>
>>> On 2/24/09, vincent chu <vincentchu at gmail.com> wrote:
>>> Hi all ---
>>>
>>> In the course of developing our Facebook connect app, we realized  
>>> that there was a  security hole in Facebooker that allows any  
>>> malicious user to change the state of the Facebooker module and  
>>> crash any controller/view that uses Facebooker to capture a  
>>> Facebook session. For Facebook connect apps, this could  
>>> potentially be in any view that uses the
"set_facebook_session"
>>> before_filter.
>>>
>>> All the malicious user has to do is send a malformed HTTP request  
>>> similar to:
>>>
>>> http://my.rails.app.com/some_controller/? 
>>> fb_sig_api_key=you_are_pwned
>>>
>>> The problem comes in the ''set_adapter'' method of
''facebooker/lib/
>>> facebooker/rails/controller.rb'' where Facebooker will
attempt to
>>> load an adapter from the params hash if fb_sig_api_key is in the  
>>> request (ignoring the configuration found in the facebooker.yml  
>>> file). In this case, Facebooker would dutifully set the api_key to
>>> "you_are_pwned" and any subsequent call to Facebooker
would try
>>> and use "you_are_pwned" as the api_key, causing it to
crash the
>>> site.
>>
>>
>>
>>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
--
Mike Mangino
http://www.elevatedrails.com



-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/f472b09c/attachment.html>

I don''t think I am clear anymore what the risk is of loading an adapter
from
some malicious attack?   Before adapters there was simply one config.  All
the adapter does is allow for multiple configs.

What are we trying to protect against?


Dave

On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
<mmangino at elevatedrails.com>wrote:
> Do we need to know which adapter to use before we can verify the params?
> Don''t other implementation use different params?
> Maybe we could require each adapter to verify that the request is valid.
> Would that fix this issue?
>
> Mike
>
>
> On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
>
> I''m not convinced we have a good solution yet.  here''s
the relevant code:
>
>       def request_comes_from_facebook?
>         request_is_for_a_facebook_canvas? || request_is_facebook_ajax? ||
> request_is_fb_ping?
>       end
>
>       def request_is_fb_ping?
>         !params[''fb_sig''].blank?
>       end
>
>       def request_is_for_a_facebook_canvas?
>         !params[''fb_sig_in_canvas''].blank?
>       end
>
>       def request_is_facebook_ajax?
>         params["fb_sig_is_mockajax"]=="1" ||
params["fb_sig_is_ajax"]=="1"
>       end
>
>
> So calling request_comes_from_facebook isn''t really a security
feature, and
> doesn''t change the behavior w.r.t. the test case that vince
outlined, since
> they could just as easily fake params["fb_sig"]
>
>
> All the malicious user has to do is send a malformed HTTP request similar
> to:
>
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>
>
> I think the before filter should be:
>
> *      def set_adapter*
> *        Facebooker.load_adapter(facebook_params)
> if(params[:fb_sig_api_key])*
> *      end*
>
> since facebook_params calls verify_signature & ensures the request
comes
> from facebook.
>
> Alternatively, we could be verify the params signature in
> request_comes_from_facebook.  That may be a better solution since the
> request_comes_from_facebook method is a little misleading (as david has
just
> demonstrated).
>
> Also, we throw an exception on a bad signature, so we should have a catch
> somewhere in the process.
>
> - kevin
>
>
>
>
> On Feb 25, 2009, at 1:10 PM, David Clements wrote:
>
> Cool thanks for taking a look.  Looks like I can just add a condition to
> that call
>
> if request_comes_from_facebook?
>
> I''ll try to get to it later today.
>
> Dave
>
> On 2/25/09, vincent chu <vincentchu at gmail.com> wrote:
>>
>> Hi David ---
>>
>> I took a look at your fix. Though I''m somewhat unfamiliar with
exactly
>> what you want to do, I think it would be prudent to validate that the
>> incoming params hash actually originates from facebook before using the
>> parameters to reset the adapter. This way, you never touch the adapter
until
>> you''re sure that it''s Facebook sending the request,
and not some malicious
>> actor.
>>
>> Cheers,
>>
>> Vince
>> ----
>>
>>
>>>
>>>
>>> On 2/24/09, vincent chu <vincentchu at gmail.com> wrote:
>>>>
>>>> Hi all ---
>>>>
>>>> In the course of developing our Facebook connect app, we
realized that
>>>> there was a  security hole in Facebooker that allows any
malicious user to
>>>> change the state of the Facebooker module and crash any
controller/view that
>>>> uses Facebooker to capture a Facebook session. For Facebook
connect apps,
>>>> this could potentially be in any view that uses the
"set_facebook_session"
>>>> before_filter.
>>>>
>>>> All the malicious user has to do is send a malformed HTTP
request
>>>> similar to:
>>>>
>>>>
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>>>
>>>> The problem comes in the ''set_adapter'' method
of
>>>>
''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
>>>> attempt to load an adapter from the params hash if
fb_sig_api_key is in the
>>>> request (ignoring the configuration found in the facebooker.yml
file). In
>>>> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
>>>> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
>>>> api_key, causing it to crash the site.
>>>>
>>>
>>>
>>>
>>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
> --
> Mike Mangino
> http://www.elevatedrails.com
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/580dd842/attachment.html>

B/c of the way facebooker stores loaded configs the attack will crash
all requests until a good config is set again (yes?).

A better question is why one would need to change configs via GET
params.  Multiple facebook apps?  Does that really need to be supported?

-- 
Matthew Beale :: 607 227 0871
Resume & Portfolio @ http://madhatted.com

On Wed, 2009-02-25 at 12:13 -0700, David Clements wrote:
> I don''t think I am clear anymore what the risk is of loading an
> adapter from some malicious attack?   Before adapters there was simply
> one config.  All the adapter does is allow for multiple configs.
> 
> What are we trying to protect against? 
> 
> 
> Dave
> 
> On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
> <mmangino at elevatedrails.com> wrote:
>         Do we need to know which adapter to use before we can verify
>         the params? Don''t other implementation use different
params?
>         
>         
>         Maybe we could require each adapter to verify that the request
>         is valid. Would that fix this issue?
>         
>         
>         Mike
>         
>         
>         
>         
>         On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
>         
>         
>         > 
>         > I''m not convinced we have a good solution yet. 
here''s the
>         > relevant code:
>         > 
>         > 
>         > 
>         > >       def request_comes_from_facebook?
>         > >         request_is_for_a_facebook_canvas? ||
>         > > request_is_facebook_ajax? || request_is_fb_ping?
>         > >       end
>         > > 
>         > > 
>         > >       def request_is_fb_ping?
>         > >         !params[''fb_sig''].blank?
>         > >       end
>         > >       
>         > >       def request_is_for_a_facebook_canvas?
>         > >        
!params[''fb_sig_in_canvas''].blank?
>         > >       end
>         > >       
>         > >       def request_is_facebook_ajax?
>         > >        
params["fb_sig_is_mockajax"]=="1" ||
>         > > params["fb_sig_is_ajax"]=="1"
>         > >       end
>         > > 
>         > > 
>         > 
>         > 
>         > So calling request_comes_from_facebook isn''t really a
>         > security feature, and doesn''t change the behavior
w.r.t. the
>         > test case that vince outlined, since they could just as
>         > easily fake params["fb_sig"]
>         > 
>         > 
>         > > > 
>         > > > All the malicious user has to do is send a malformed
>         > > > HTTP request similar to:
>         > > > 
>         > > >
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>         > 
>         > 
>         > I think the before filter should be:
>         >  
>         >       def set_adapter
>         >         Facebooker.load_adapter(facebook_params)
>         > if(params[:fb_sig_api_key])
>         >       end
>         > 
>         > 
>         > since facebook_params calls verify_signature & ensures the
>         > request comes from facebook. 
>         > 
>         > 
>         > Alternatively, we could be verify the params signature in
>         > request_comes_from_facebook.  That may be a better solution
>         > since the request_comes_from_facebook method is a little
>         > misleading (as david has just demonstrated).
>         > 
>         > 
>         > Also, we throw an exception on a bad signature, so we should
>         > have a catch somewhere in the process.
>         > 
>         > 
>         > - kevin
>         > 
>         > 
>         > 
>         > 
>         > 
>         > 
>         > 
>         > On Feb 25, 2009, at 1:10 PM, David Clements wrote:
>         > 
>         > > Cool thanks for taking a look.  Looks like I can just add
>         > > a condition to that call
>         > > 
>         > > if request_comes_from_facebook?
>         > > 
>         > > I''ll try to get to it later today.
>         > > 
>         > > Dave
>         > > 
>         > > On 2/25/09, vincent chu <vincentchu at gmail.com>
wrote:
>         > >         Hi David ---
>         > >         
>         > >         I took a look at your fix. Though I''m
somewhat
>         > >         unfamiliar with exactly what you want to do, I
>         > >         think it would be prudent to validate that the
>         > >         incoming params hash actually originates from
>         > >         facebook before using the parameters to reset the
>         > >         adapter. This way, you never touch the adapter
>         > >         until you''re sure that it''s
Facebook sending the
>         > >         request, and not some malicious actor.
>         > >         
>         > >         Cheers,
>         > >         
>         > >         Vince
>         > >         ----
>         > >         
>         > >                 
>         > >                 > 
>         > >                 > 
>         > >                 > On 2/24/09, vincent chu
>         > >                 > <vincentchu at gmail.com>
wrote:
>         > >                 >         Hi all ---
>         > >                 >         
>         > >                 >         In the course of developing
our
>         > >                 >         Facebook connect app, we
>         > >                 >         realized that there was a
>         > >                 >         security hole in Facebooker
that
>         > >                 >         allows any malicious user to
>         > >                 >         change the state of the
>         > >                 >         Facebooker module and crash
any
>         > >                 >         controller/view that uses
>         > >                 >         Facebooker to capture a
Facebook
>         > >                 >         session. For Facebook
connect
>         > >                 >         apps, this could potentially
be
>         > >                 >         in any view that uses the
>         > >                 >        
"set_facebook_session"
>         > >                 >         before_filter.
>         > >                 >         
>         > >                 >         All the malicious user has
to do
>         > >                 >         is send a malformed HTTP
request
>         > >                 >         similar to:
>         > >                 >         
>         > >                 >        
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>         > >                 >         
>         > >                 >         The problem comes in the
>         > >                 >        
''set_adapter'' method of
>         > >                 >        
''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will attempt to load an adapter from the params hash if
fb_sig_api_key is in the request (ignoring the configuration found in the
facebooker.yml file). In this case, Facebooker would dutifully set the api_key
to "you_are_pwned" and any subsequent call to Facebooker would try and
use "you_are_pwned" as the api_key, causing it to crash the site.
>         > >                 
>         > >                 
>         > >         
>         > >         
>         > > 
>         > 
>         > 
>         > _______________________________________________
>         > Facebooker-talk mailing list
>         > Facebooker-talk at rubyforge.org
>         > http://rubyforge.org/mailman/listinfo/facebooker-talk
>         > 
>         
>         --
>         Mike Mangino
>         http://www.elevatedrails.com
>         
>         
>         
>         
>         
>         
> 
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk

The new code doesn''t allow the loading of a bad config.  It returns the
default config if it can''t find the passed in one.   The bug was a
simple
one of ruby returning the last value.

We can change it to use Post params only, doesn''t really solve any
security
concerns though.

The initial implementation was to support Bebo it was augmented to allow
multiple Facebook applications.  I think it is a useful feature to have.   I
believe some are using it.

Dave


On 2/25/09, Matthew Beale <mixonic at synitech.com>
wrote:
>
> B/c of the way facebooker stores loaded configs the attack will crash
> all requests until a good config is set again (yes?).
>
> A better question is why one would need to change configs via GET
> params.  Multiple facebook apps?  Does that really need to be supported?
>
>
> --
> Matthew Beale :: 607 227 0871
> Resume & Portfolio @ http://madhatted.com
>
>
> On Wed, 2009-02-25 at 12:13 -0700, David Clements wrote:
> > I don''t think I am clear anymore what the risk is of loading
an
> > adapter from some malicious attack?   Before adapters there was simply
> > one config.  All the adapter does is allow for multiple configs.
> >
> > What are we trying to protect against?
> >
> >
> > Dave
> >
> > On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
> > <mmangino at elevatedrails.com> wrote:
> >         Do we need to know which adapter to use before we can verify
> >         the params? Don''t other implementation use different
params?
> >
> >
> >         Maybe we could require each adapter to verify that the request
> >         is valid. Would that fix this issue?
> >
> >
> >         Mike
> >
> >
> >
> >
> >         On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
> >
> >
> >         >
> >         > I''m not convinced we have a good solution yet. 
here''s the
> >         > relevant code:
> >         >
> >         >
> >         >
> >         > >       def request_comes_from_facebook?
> >         > >         request_is_for_a_facebook_canvas? ||
> >         > > request_is_facebook_ajax? || request_is_fb_ping?
> >         > >       end
> >         > >
> >         > >
> >         > >       def request_is_fb_ping?
> >         > >         !params[''fb_sig''].blank?
> >         > >       end
> >         > >
> >         > >       def request_is_for_a_facebook_canvas?
> >         > >        
!params[''fb_sig_in_canvas''].blank?
> >         > >       end
> >         > >
> >         > >       def request_is_facebook_ajax?
> >         > >        
params["fb_sig_is_mockajax"]=="1" ||
> >         > > params["fb_sig_is_ajax"]=="1"
> >         > >       end
> >         > >
> >         > >
> >         >
> >         >
> >         > So calling request_comes_from_facebook isn''t
really a
> >         > security feature, and doesn''t change the
behavior w.r.t. the
> >         > test case that vince outlined, since they could just as
> >         > easily fake params["fb_sig"]
> >         >
> >         >
> >         > > >
> >         > > > All the malicious user has to do is send a
malformed
> >         > > > HTTP request similar to:
> >         > > >
> >         > > >
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
> >         >
> >         >
> >         > I think the before filter should be:
> >         >
> >         >       def set_adapter
> >         >         Facebooker.load_adapter(facebook_params)
> >         > if(params[:fb_sig_api_key])
> >         >       end
> >         >
> >         >
> >         > since facebook_params calls verify_signature &
ensures the
> >         > request comes from facebook.
> >         >
> >         >
> >         > Alternatively, we could be verify the params signature in
> >         > request_comes_from_facebook.  That may be a better
solution
> >         > since the request_comes_from_facebook method is a little
> >         > misleading (as david has just demonstrated).
> >         >
> >         >
> >         > Also, we throw an exception on a bad signature, so we
should
> >         > have a catch somewhere in the process.
> >         >
> >         >
> >         > - kevin
> >         >
> >         >
> >         >
> >         >
> >         >
> >         >
> >         >
> >         > On Feb 25, 2009, at 1:10 PM, David Clements wrote:
> >         >
> >         > > Cool thanks for taking a look.  Looks like I can
just add
> >         > > a condition to that call
> >         > >
> >         > > if request_comes_from_facebook?
> >         > >
> >         > > I''ll try to get to it later today.
> >         > >
> >         > > Dave
> >         > >
> >         > > On 2/25/09, vincent chu <vincentchu at
gmail.com> wrote:
> >         > >         Hi David ---
> >         > >
> >         > >         I took a look at your fix. Though
I''m somewhat
> >         > >         unfamiliar with exactly what you want to do,
I
> >         > >         think it would be prudent to validate that
the
> >         > >         incoming params hash actually originates
from
> >         > >         facebook before using the parameters to
reset the
> >         > >         adapter. This way, you never touch the
adapter
> >         > >         until you''re sure that
it''s Facebook sending the
> >         > >         request, and not some malicious actor.
> >         > >
> >         > >         Cheers,
> >         > >
> >         > >         Vince
> >         > >         ----
> >         > >
> >         > >
> >         > >                 >
> >         > >                 >
> >         > >                 > On 2/24/09, vincent chu
> >         > >                 > <vincentchu at gmail.com>
wrote:
> >         > >                 >         Hi all ---
> >         > >                 >
> >         > >                 >         In the course of
developing our
> >         > >                 >         Facebook connect app,
we
> >         > >                 >         realized that there was
a
> >         > >                 >         security hole in
Facebooker that
> >         > >                 >         allows any malicious
user to
> >         > >                 >         change the state of the
> >         > >                 >         Facebooker module and
crash any
> >         > >                 >         controller/view that
uses
> >         > >                 >         Facebooker to capture a
Facebook
> >         > >                 >         session. For Facebook
connect
> >         > >                 >         apps, this could
potentially be
> >         > >                 >         in any view that uses
the
> >         > >                 >        
"set_facebook_session"
> >         > >                 >         before_filter.
> >         > >                 >
> >         > >                 >         All the malicious user
has to do
> >         > >                 >         is send a malformed
HTTP request
> >         > >                 >         similar to:
> >         > >                 >
> >         > >                 >
> http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
> >         > >                 >
> >         > >                 >         The problem comes in
the
> >         > >                 >        
''set_adapter'' method of
> >         > >                 >
> ''facebooker/lib/facebooker/rails/controller.rb'' where
Facebooker will
> attempt to load an adapter from the params hash if fb_sig_api_key is in the
> request (ignoring the configuration found in the facebooker.yml file). In
> this case, Facebooker would dutifully set the api_key to
"you_are_pwned" and
> any subsequent call to Facebooker would try and use
"you_are_pwned" as the
> api_key, causing it to crash the site.
> >         > >
> >         > >
> >         > >
> >         > >
> >         > >
> >         >
> >         >
> >         > _______________________________________________
> >         > Facebooker-talk mailing list
> >         > Facebooker-talk at rubyforge.org
> >         > http://rubyforge.org/mailman/listinfo/facebooker-talk
> >         >
> >
> >         --
> >         Mike Mangino
> >         http://www.elevatedrails.com
> >
> >
> >
> >
> >
> >
> >
> > _______________________________________________
> > Facebooker-talk mailing list
> > Facebooker-talk at rubyforge.org
> > http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/49f0f6c4/attachment.html>

Support for both multiple Facebook apps and for Bebo apps is certainly  
a feature in use in production, and it''s a great perk of facebooker.
As Dave said, is the issue not fixed when a default config is now  
returned if the passed key isn''t found?

Cheers.

On Feb 25, 2009, at 11:25 AM, David Clements wrote:
> The new code doesn''t allow the loading of a bad config.  It
returns
> the default config if it can''t find the passed in one.   The bug
was
> a simple one of ruby returning the last value.
>
> We can change it to use Post params only, doesn''t really solve any
> security concerns though.
>
> The initial implementation was to support Bebo it was augmented to  
> allow multiple Facebook applications.  I think it is a useful  
> feature to have.   I believe some are using it.
>
> Dave
>
>
> On 2/25/09, Matthew Beale <mixonic at synitech.com> wrote:
> B/c of the way facebooker stores loaded configs the attack will crash
> all requests until a good config is set again (yes?).
>
> A better question is why one would need to change configs via GET
> params.  Multiple facebook apps?  Does that really need to be  
> supported?
>
>
> --
> Matthew Beale :: 607 227 0871
> Resume & Portfolio @ http://madhatted.com
>
>
> On Wed, 2009-02-25 at 12:13 -0700, David Clements wrote:
> > I don''t think I am clear anymore what the risk is of loading
an
> > adapter from some malicious attack?   Before adapters there was  
> simply
> > one config.  All the adapter does is allow for multiple configs.
> >
> > What are we trying to protect against?
> >
> >
> > Dave
> >
> > On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
> > <mmangino at elevatedrails.com> wrote:
> >         Do we need to know which adapter to use before we can verify
> >         the params? Don''t other implementation use different
params?
> >
> >
> >         Maybe we could require each adapter to verify that the  
> request
> >         is valid. Would that fix this issue?
> >
> >
> >         Mike
> >
> >
> >
> >
> >         On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
> >
> >
> >         >
> >         > I''m not convinced we have a good solution yet. 
here''s the
> >         > relevant code:
> >         >
> >         >
> >         >
> >         > >       def request_comes_from_facebook?
> >         > >         request_is_for_a_facebook_canvas? ||
> >         > > request_is_facebook_ajax? || request_is_fb_ping?
> >         > >       end
> >         > >
> >         > >
> >         > >       def request_is_fb_ping?
> >         > >         !params[''fb_sig''].blank?
> >         > >       end
> >         > >
> >         > >       def request_is_for_a_facebook_canvas?
> >         > >        
!params[''fb_sig_in_canvas''].blank?
> >         > >       end
> >         > >
> >         > >       def request_is_facebook_ajax?
> >         > >        
params["fb_sig_is_mockajax"]=="1" ||
> >         > > params["fb_sig_is_ajax"]=="1"
> >         > >       end
> >         > >
> >         > >
> >         >
> >         >
> >         > So calling request_comes_from_facebook isn''t
really a
> >         > security feature, and doesn''t change the
behavior w.r.t.
> the
> >         > test case that vince outlined, since they could just as
> >         > easily fake params["fb_sig"]
> >         >
> >         >
> >         > > >
> >         > > > All the malicious user has to do is send a
malformed
> >         > > > HTTP request similar to:
> >         > > >
> >         > > >
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
> >         >
> >         >
> >         > I think the before filter should be:
> >         >
> >         >       def set_adapter
> >         >         Facebooker.load_adapter(facebook_params)
> >         > if(params[:fb_sig_api_key])
> >         >       end
> >         >
> >         >
> >         > since facebook_params calls verify_signature &
ensures the
> >         > request comes from facebook.
> >         >
> >         >
> >         > Alternatively, we could be verify the params signature in
> >         > request_comes_from_facebook.  That may be a better  
> solution
> >         > since the request_comes_from_facebook method is a little
> >         > misleading (as david has just demonstrated).
> >         >
> >         >
> >         > Also, we throw an exception on a bad signature, so we  
> should
> >         > have a catch somewhere in the process.
> >         >
> >         >
> >         > - kevin
> >         >
> >         >
> >         >
> >         >
> >         >
> >         >
> >         >
> >         > On Feb 25, 2009, at 1:10 PM, David Clements wrote:
> >         >
> >         > > Cool thanks for taking a look.  Looks like I can
just
> add
> >         > > a condition to that call
> >         > >
> >         > > if request_comes_from_facebook?
> >         > >
> >         > > I''ll try to get to it later today.
> >         > >
> >         > > Dave
> >         > >
> >         > > On 2/25/09, vincent chu <vincentchu at
gmail.com> wrote:
> >         > >         Hi David ---
> >         > >
> >         > >         I took a look at your fix. Though
I''m somewhat
> >         > >         unfamiliar with exactly what you want to do,
I
> >         > >         think it would be prudent to validate that
the
> >         > >         incoming params hash actually originates
from
> >         > >         facebook before using the parameters to
reset
> the
> >         > >         adapter. This way, you never touch the
adapter
> >         > >         until you''re sure that
it''s Facebook sending the
> >         > >         request, and not some malicious actor.
> >         > >
> >         > >         Cheers,
> >         > >
> >         > >         Vince
> >         > >         ----
> >         > >
> >         > >
> >         > >                 >
> >         > >                 >
> >         > >                 > On 2/24/09, vincent chu
> >         > >                 > <vincentchu at gmail.com>
wrote:
> >         > >                 >         Hi all ---
> >         > >                 >
> >         > >                 >         In the course of
developing
> our
> >         > >                 >         Facebook connect app,
we
> >         > >                 >         realized that there was
a
> >         > >                 >         security hole in
Facebooker
> that
> >         > >                 >         allows any malicious
user to
> >         > >                 >         change the state of the
> >         > >                 >         Facebooker module and
crash
> any
> >         > >                 >         controller/view that
uses
> >         > >                 >         Facebooker to capture a
> Facebook
> >         > >                 >         session. For Facebook
connect
> >         > >                 >         apps, this could
potentially
> be
> >         > >                 >         in any view that uses
the
> >         > >                 >        
"set_facebook_session"
> >         > >                 >         before_filter.
> >         > >                 >
> >         > >                 >         All the malicious user
has
> to do
> >         > >                 >         is send a malformed
HTTP
> request
> >         > >                 >         similar to:
> >         > >                 >
> >         > >                 >        
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
> >         > >                 >
> >         > >                 >         The problem comes in
the
> >         > >                 >        
''set_adapter'' method of
> >         > >                 >        
''facebooker/lib/facebooker/
> rails/controller.rb'' where Facebooker will attempt to load an  
> adapter from the params hash if fb_sig_api_key is in the request  
> (ignoring the configuration found in the facebooker.yml file). In  
> this case, Facebooker would dutifully set the api_key to  
> "you_are_pwned" and any subsequent call to Facebooker would try
and
> use "you_are_pwned" as the api_key, causing it to crash the site.
> >         > >
> >         > >
> >         > >
> >         > >
> >         > >
> >         >
> >         >
> >         > _______________________________________________
> >         > Facebooker-talk mailing list
> >         > Facebooker-talk at rubyforge.org
> >         > http://rubyforge.org/mailman/listinfo/facebooker-talk
> >         >
> >
> >         --
> >         Mike Mangino
> >         http://www.elevatedrails.com
> >
> >
> >
> >
> >
> >
> >
> > _______________________________________________
> > Facebooker-talk mailing list
> > Facebooker-talk at rubyforge.org
> > http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/3ec398ae/attachment-0001.html>

I''m still not convinced.   Here''s the basic logic:

def load_adapter(params)

   api_key = params[:fb_sig_api_key]      #paraphrasing, but we know  
fb_sig_api_key was in the params

  facebooker_config.each do |key,value|
         next unless value == api_key

	. . .  #sets up new adapter_config

         return adapter_class.new(adapter_config)
   end
   return self.default_adapter(params)
end

That looks to me like it returns the default adapter only if there is  
no value in facebooker_config matching the passed in api_key.  The  
cookies set by facebook connect all have names of "foo_#{api_key}", so
the api key isn''t exactly a security feature.

- kevin

On Feb 25, 2009, at 2:25 PM, David Clements wrote:
>  It returns the default config if it can''t find the passed in one.
>
> Dave

I think i''m with you now -

If they go through the trouble of sending in your api key, that just  
loads the appropriate adapter.

If they send a bogus api_key, it returns the default.

  right?

  - kevin


On Feb 25, 2009, at 2:25 PM, David Clements wrote:
> The new code doesn''t allow the loading of a bad config.  It
returns
> the default config if it can''t find the passed in one.   The bug
was
> a simple one of ruby returning the last value.
>
> We can change it to use Post params only, doesn''t really solve any
> security concerns though.
>
> The initial implementation was to support Bebo it was augmented to  
> allow multiple Facebook applications.  I think it is a useful  
> feature to have.   I believe some are using it.
>
> Dave
>
>
> On 2/25/09, Matthew Beale <mixonic at synitech.com> wrote:
> B/c of the way facebooker stores loaded configs the attack will crash
> all requests until a good config is set again (yes?).
>
> A better question is why one would need to change configs via GET
> params.  Multiple facebook apps?  Does that really need to be  
> supported?
>
>
> --
> Matthew Beale :: 607 227 0871
> Resume & Portfolio @ http://madhatted.com
>
>
> On Wed, 2009-02-25 at 12:13 -0700, David Clements wrote:
> > I don''t think I am clear anymore what the risk is of loading
an
> > adapter from some malicious attack?   Before adapters there was  
> simply
> > one config.  All the adapter does is allow for multiple configs.
> >
> > What are we trying to protect against?
> >
> >
> > Dave
> >
> > On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
> > <mmangino at elevatedrails.com> wrote:
> >         Do we need to know which adapter to use before we can verify
> >         the params? Don''t other implementation use different
params?
> >
> >
> >         Maybe we could require each adapter to verify that the  
> request
> >         is valid. Would that fix this issue?
> >
> >
> >         Mike
> >
> >
> >
> >
> >         On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
> >
> >
> >         >
> >         > I''m not convinced we have a good solution yet. 
here''s the
> >         > relevant code:
> >         >
> >         >
> >         >
> >         > >       def request_comes_from_facebook?
> >         > >         request_is_for_a_facebook_canvas? ||
> >         > > request_is_facebook_ajax? || request_is_fb_ping?
> >         > >       end
> >         > >
> >         > >
> >         > >       def request_is_fb_ping?
> >         > >         !params[''fb_sig''].blank?
> >         > >       end
> >         > >
> >         > >       def request_is_for_a_facebook_canvas?
> >         > >        
!params[''fb_sig_in_canvas''].blank?
> >         > >       end
> >         > >
> >         > >       def request_is_facebook_ajax?
> >         > >        
params["fb_sig_is_mockajax"]=="1" ||
> >         > > params["fb_sig_is_ajax"]=="1"
> >         > >       end
> >         > >
> >         > >
> >         >
> >         >
> >         > So calling request_comes_from_facebook isn''t
really a
> >         > security feature, and doesn''t change the
behavior w.r.t.
> the
> >         > test case that vince outlined, since they could just as
> >         > easily fake params["fb_sig"]
> >         >
> >         >
> >         > > >
> >         > > > All the malicious user has to do is send a
malformed
> >         > > > HTTP request similar to:
> >         > > >
> >         > > >
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
> >         >
> >         >
> >         > I think the before filter should be:
> >         >
> >         >       def set_adapter
> >         >         Facebooker.load_adapter(facebook_params)
> >         > if(params[:fb_sig_api_key])
> >         >       end
> >         >
> >         >
> >         > since facebook_params calls verify_signature &
ensures the
> >         > request comes from facebook.
> >         >
> >         >
> >         > Alternatively, we could be verify the params signature in
> >         > request_comes_from_facebook.  That may be a better  
> solution
> >         > since the request_comes_from_facebook method is a little
> >         > misleading (as david has just demonstrated).
> >         >
> >         >
> >         > Also, we throw an exception on a bad signature, so we  
> should
> >         > have a catch somewhere in the process.
> >         >
> >         >
> >         > - kevin
> >         >
> >         >
> >         >
> >         >
> >         >
> >         >
> >         >
> >         > On Feb 25, 2009, at 1:10 PM, David Clements wrote:
> >         >
> >         > > Cool thanks for taking a look.  Looks like I can
just
> add
> >         > > a condition to that call
> >         > >
> >         > > if request_comes_from_facebook?
> >         > >
> >         > > I''ll try to get to it later today.
> >         > >
> >         > > Dave
> >         > >
> >         > > On 2/25/09, vincent chu <vincentchu at
gmail.com> wrote:
> >         > >         Hi David ---
> >         > >
> >         > >         I took a look at your fix. Though
I''m somewhat
> >         > >         unfamiliar with exactly what you want to do,
I
> >         > >         think it would be prudent to validate that
the
> >         > >         incoming params hash actually originates
from
> >         > >         facebook before using the parameters to
reset
> the
> >         > >         adapter. This way, you never touch the
adapter
> >         > >         until you''re sure that
it''s Facebook sending the
> >         > >         request, and not some malicious actor.
> >         > >
> >         > >         Cheers,
> >         > >
> >         > >         Vince
> >         > >         ----
> >         > >
> >         > >
> >         > >                 >
> >         > >                 >
> >         > >                 > On 2/24/09, vincent chu
> >         > >                 > <vincentchu at gmail.com>
wrote:
> >         > >                 >         Hi all ---
> >         > >                 >
> >         > >                 >         In the course of
developing
> our
> >         > >                 >         Facebook connect app,
we
> >         > >                 >         realized that there was
a
> >         > >                 >         security hole in
Facebooker
> that
> >         > >                 >         allows any malicious
user to
> >         > >                 >         change the state of the
> >         > >                 >         Facebooker module and
crash
> any
> >         > >                 >         controller/view that
uses
> >         > >                 >         Facebooker to capture a
> Facebook
> >         > >                 >         session. For Facebook
connect
> >         > >                 >         apps, this could
potentially
> be
> >         > >                 >         in any view that uses
the
> >         > >                 >        
"set_facebook_session"
> >         > >                 >         before_filter.
> >         > >                 >
> >         > >                 >         All the malicious user
has
> to do
> >         > >                 >         is send a malformed
HTTP
> request
> >         > >                 >         similar to:
> >         > >                 >
> >         > >                 >        
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
> >         > >                 >
> >         > >                 >         The problem comes in
the
> >         > >                 >        
''set_adapter'' method of
> >         > >                 >        
''facebooker/lib/facebooker/
> rails/controller.rb'' where Facebooker will attempt to load an  
> adapter from the params hash if fb_sig_api_key is in the request  
> (ignoring the configuration found in the facebooker.yml file). In  
> this case, Facebooker would dutifully set the api_key to  
> "you_are_pwned" and any subsequent call to Facebooker would try
and
> use "you_are_pwned" as the api_key, causing it to crash the site.
> >         > >
> >         > >
> >         > >
> >         > >
> >         > >
> >         >
> >         >
> >         > _______________________________________________
> >         > Facebooker-talk mailing list
> >         > Facebooker-talk at rubyforge.org
> >         > http://rubyforge.org/mailman/listinfo/facebooker-talk
> >         >
> >
> >         --
> >         Mike Mangino
> >         http://www.elevatedrails.com
> >
> >
> >
> >
> >
> >
> >
> > _______________________________________________
> > Facebooker-talk mailing list
> > Facebooker-talk at rubyforge.org
> > http://rubyforge.org/mailman/listinfo/facebooker-talk
>
>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/6aad698c/attachment-0001.html>

On Feb 25, 2009, at 2:42 PM, Chris Johnson wrote:
> Support for both multiple Facebook apps and for Bebo apps is  
> certainly a feature in use in production, and it''s a great perk of
> facebooker.
> As Dave said, is the issue not fixed when a default config is now  
> returned if the passed key isn''t found?
Would this bug only affect the current request? If the next request  
loads the new adapter, it probably isn''t a big deal.

Sorry about being late to the table. This came in while I was on  
vacation without internet access.

Mike

>
> Cheers.
>
> On Feb 25, 2009, at 11:25 AM, David Clements wrote:
>
>> The new code doesn''t allow the loading of a bad config.  It
returns
>> the default config if it can''t find the passed in one.   The
bug
>> was a simple one of ruby returning the last value.
>>
>> We can change it to use Post params only, doesn''t really solve
any
>> security concerns though.
>>
>> The initial implementation was to support Bebo it was augmented to  
>> allow multiple Facebook applications.  I think it is a useful  
>> feature to have.   I believe some are using it.
>>
>> Dave
>>
>>
>> On 2/25/09, Matthew Beale <mixonic at synitech.com> wrote:
>> B/c of the way facebooker stores loaded configs the attack will crash
>> all requests until a good config is set again (yes?).
>>
>> A better question is why one would need to change configs via GET
>> params.  Multiple facebook apps?  Does that really need to be  
>> supported?
>>
>>
>> --
>> Matthew Beale :: 607 227 0871
>> Resume & Portfolio @ http://madhatted.com
>>
>>
>> On Wed, 2009-02-25 at 12:13 -0700, David Clements wrote:
>> > I don''t think I am clear anymore what the risk is of
loading an
>> > adapter from some malicious attack?   Before adapters there was  
>> simply
>> > one config.  All the adapter does is allow for multiple configs.
>> >
>> > What are we trying to protect against?
>> >
>> >
>> > Dave
>> >
>> > On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
>> > <mmangino at elevatedrails.com> wrote:
>> >         Do we need to know which adapter to use before we can  
>> verify
>> >         the params? Don''t other implementation use
different
>> params?
>> >
>> >
>> >         Maybe we could require each adapter to verify that the  
>> request
>> >         is valid. Would that fix this issue?
>> >
>> >
>> >         Mike
>> >
>> >
>> >
>> >
>> >         On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
>> >
>> >
>> >         >
>> >         > I''m not convinced we have a good solution
yet.  here''s
>> the
>> >         > relevant code:
>> >         >
>> >         >
>> >         >
>> >         > >       def request_comes_from_facebook?
>> >         > >         request_is_for_a_facebook_canvas? ||
>> >         > > request_is_facebook_ajax? || request_is_fb_ping?
>> >         > >       end
>> >         > >
>> >         > >
>> >         > >       def request_is_fb_ping?
>> >         > >        
!params[''fb_sig''].blank?
>> >         > >       end
>> >         > >
>> >         > >       def request_is_for_a_facebook_canvas?
>> >         > >        
!params[''fb_sig_in_canvas''].blank?
>> >         > >       end
>> >         > >
>> >         > >       def request_is_facebook_ajax?
>> >         > >        
params["fb_sig_is_mockajax"]=="1" ||
>> >         > >
params["fb_sig_is_ajax"]=="1"
>> >         > >       end
>> >         > >
>> >         > >
>> >         >
>> >         >
>> >         > So calling request_comes_from_facebook isn''t
really a
>> >         > security feature, and doesn''t change the
behavior
>> w.r.t. the
>> >         > test case that vince outlined, since they could just
as
>> >         > easily fake params["fb_sig"]
>> >         >
>> >         >
>> >         > > >
>> >         > > > All the malicious user has to do is send a
malformed
>> >         > > > HTTP request similar to:
>> >         > > >
>> >         > > >
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>> >         >
>> >         >
>> >         > I think the before filter should be:
>> >         >
>> >         >       def set_adapter
>> >         >         Facebooker.load_adapter(facebook_params)
>> >         > if(params[:fb_sig_api_key])
>> >         >       end
>> >         >
>> >         >
>> >         > since facebook_params calls verify_signature &
ensures
>> the
>> >         > request comes from facebook.
>> >         >
>> >         >
>> >         > Alternatively, we could be verify the params
signature in
>> >         > request_comes_from_facebook.  That may be a better  
>> solution
>> >         > since the request_comes_from_facebook method is a
little
>> >         > misleading (as david has just demonstrated).
>> >         >
>> >         >
>> >         > Also, we throw an exception on a bad signature, so we
>> should
>> >         > have a catch somewhere in the process.
>> >         >
>> >         >
>> >         > - kevin
>> >         >
>> >         >
>> >         >
>> >         >
>> >         >
>> >         >
>> >         >
>> >         > On Feb 25, 2009, at 1:10 PM, David Clements wrote:
>> >         >
>> >         > > Cool thanks for taking a look.  Looks like I can
just
>> add
>> >         > > a condition to that call
>> >         > >
>> >         > > if request_comes_from_facebook?
>> >         > >
>> >         > > I''ll try to get to it later today.
>> >         > >
>> >         > > Dave
>> >         > >
>> >         > > On 2/25/09, vincent chu <vincentchu at
gmail.com> wrote:
>> >         > >         Hi David ---
>> >         > >
>> >         > >         I took a look at your fix. Though
I''m somewhat
>> >         > >         unfamiliar with exactly what you want to
do, I
>> >         > >         think it would be prudent to validate
that the
>> >         > >         incoming params hash actually originates
from
>> >         > >         facebook before using the parameters to
reset
>> the
>> >         > >         adapter. This way, you never touch the
adapter
>> >         > >         until you''re sure that
it''s Facebook sending
>> the
>> >         > >         request, and not some malicious actor.
>> >         > >
>> >         > >         Cheers,
>> >         > >
>> >         > >         Vince
>> >         > >         ----
>> >         > >
>> >         > >
>> >         > >                 >
>> >         > >                 >
>> >         > >                 > On 2/24/09, vincent chu
>> >         > >                 > <vincentchu at
gmail.com> wrote:
>> >         > >                 >         Hi all ---
>> >         > >                 >
>> >         > >                 >         In the course of
developing
>> our
>> >         > >                 >         Facebook connect
app, we
>> >         > >                 >         realized that there
was a
>> >         > >                 >         security hole in
Facebooker
>> that
>> >         > >                 >         allows any
malicious user to
>> >         > >                 >         change the state of
the
>> >         > >                 >         Facebooker module
and crash
>> any
>> >         > >                 >         controller/view
that uses
>> >         > >                 >         Facebooker to
capture a
>> Facebook
>> >         > >                 >         session. For
Facebook connect
>> >         > >                 >         apps, this could  
>> potentially be
>> >         > >                 >         in any view that
uses the
>> >         > >                 >        
"set_facebook_session"
>> >         > >                 >         before_filter.
>> >         > >                 >
>> >         > >                 >         All the malicious
user has
>> to do
>> >         > >                 >         is send a malformed
HTTP
>> request
>> >         > >                 >         similar to:
>> >         > >                 >
>> >         > >                 >        
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>> >         > >                 >
>> >         > >                 >         The problem comes
in the
>> >         > >                 >        
''set_adapter'' method of
>> >         > >                 >        
''facebooker/lib/facebooker/
>> rails/controller.rb'' where Facebooker will attempt to load an
>> adapter from the params hash if fb_sig_api_key is in the request  
>> (ignoring the configuration found in the facebooker.yml file). In  
>> this case, Facebooker would dutifully set the api_key to  
>> "you_are_pwned" and any subsequent call to Facebooker would
try and
>> use "you_are_pwned" as the api_key, causing it to crash the
site.
>> >         > >
>> >         > >
>> >         > >
>> >         > >
>> >         > >
>> >         >
>> >         >
>> >         > _______________________________________________
>> >         > Facebooker-talk mailing list
>> >         > Facebooker-talk at rubyforge.org
>> >         > http://rubyforge.org/mailman/listinfo/facebooker-talk
>> >         >
>> >
>> >         --
>> >         Mike Mangino
>> >         http://www.elevatedrails.com
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > _______________________________________________
>> > Facebooker-talk mailing list
>> > Facebooker-talk at rubyforge.org
>> > http://rubyforge.org/mailman/listinfo/facebooker-talk
>>
>>
>>
>> _______________________________________________
>> Facebooker-talk mailing list
>> Facebooker-talk at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
--
Mike Mangino
http://www.elevatedrails.com



-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/a5d52446/attachment-0001.html>

I''m ready to sign off on dave''s updated patch.

- kevin

On Feb 25, 2009, at 3:12 PM, Mike Mangino wrote:
>
> On Feb 25, 2009, at 2:42 PM, Chris Johnson wrote:
>
>> Support for both multiple Facebook apps and for Bebo apps is  
>> certainly a feature in use in production, and it''s a great
perk of
>> facebooker.
>> As Dave said, is the issue not fixed when a default config is now  
>> returned if the passed key isn''t found?
>
> Would this bug only affect the current request? If the next request  
> loads the new adapter, it probably isn''t a big deal.
>
> Sorry about being late to the table. This came in while I was on  
> vacation without internet access.
>
> Mike
>
>
>>
>> Cheers.
>>
>> On Feb 25, 2009, at 11:25 AM, David Clements wrote:
>>
>>> The new code doesn''t allow the loading of a bad config. 
It
>>> returns the default config if it can''t find the passed in
one.
>>> The bug was a simple one of ruby returning the last value.
>>>
>>> We can change it to use Post params only, doesn''t really
solve any
>>> security concerns though.
>>>
>>> The initial implementation was to support Bebo it was augmented to
>>> allow multiple Facebook applications.  I think it is a useful  
>>> feature to have.   I believe some are using it.
>>>
>>> Dave
>>>
>>>
>>> On 2/25/09, Matthew Beale <mixonic at synitech.com> wrote:
>>> B/c of the way facebooker stores loaded configs the attack will  
>>> crash
>>> all requests until a good config is set again (yes?).
>>>
>>> A better question is why one would need to change configs via GET
>>> params.  Multiple facebook apps?  Does that really need to be  
>>> supported?
>>>
>>>
>>> --
>>> Matthew Beale :: 607 227 0871
>>> Resume & Portfolio @ http://madhatted.com
>>>
>>>
>>> On Wed, 2009-02-25 at 12:13 -0700, David Clements wrote:
>>> > I don''t think I am clear anymore what the risk is of
loading an
>>> > adapter from some malicious attack?   Before adapters there
was
>>> simply
>>> > one config.  All the adapter does is allow for multiple
configs.
>>> >
>>> > What are we trying to protect against?
>>> >
>>> >
>>> > Dave
>>> >
>>> > On Wed, Feb 25, 2009 at 12:07 PM, Mike Mangino
>>> > <mmangino at elevatedrails.com> wrote:
>>> >         Do we need to know which adapter to use before we can
>>> verify
>>> >         the params? Don''t other implementation use
different
>>> params?
>>> >
>>> >
>>> >         Maybe we could require each adapter to verify that the
>>> request
>>> >         is valid. Would that fix this issue?
>>> >
>>> >
>>> >         Mike
>>> >
>>> >
>>> >
>>> >
>>> >         On Feb 25, 2009, at 1:32 PM, kevin lochner wrote:
>>> >
>>> >
>>> >         >
>>> >         > I''m not convinced we have a good
solution yet.  here''s
>>> the
>>> >         > relevant code:
>>> >         >
>>> >         >
>>> >         >
>>> >         > >       def request_comes_from_facebook?
>>> >         > >         request_is_for_a_facebook_canvas? ||
>>> >         > > request_is_facebook_ajax? ||
request_is_fb_ping?
>>> >         > >       end
>>> >         > >
>>> >         > >
>>> >         > >       def request_is_fb_ping?
>>> >         > >        
!params[''fb_sig''].blank?
>>> >         > >       end
>>> >         > >
>>> >         > >       def request_is_for_a_facebook_canvas?
>>> >         > >        
!params[''fb_sig_in_canvas''].blank?
>>> >         > >       end
>>> >         > >
>>> >         > >       def request_is_facebook_ajax?
>>> >         > >        
params["fb_sig_is_mockajax"]=="1" ||
>>> >         > >
params["fb_sig_is_ajax"]=="1"
>>> >         > >       end
>>> >         > >
>>> >         > >
>>> >         >
>>> >         >
>>> >         > So calling request_comes_from_facebook
isn''t really a
>>> >         > security feature, and doesn''t change the
behavior
>>> w.r.t. the
>>> >         > test case that vince outlined, since they could
just as
>>> >         > easily fake params["fb_sig"]
>>> >         >
>>> >         >
>>> >         > > >
>>> >         > > > All the malicious user has to do is
send a malformed
>>> >         > > > HTTP request similar to:
>>> >         > > >
>>> >         > > >
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>> >         >
>>> >         >
>>> >         > I think the before filter should be:
>>> >         >
>>> >         >       def set_adapter
>>> >         >         Facebooker.load_adapter(facebook_params)
>>> >         > if(params[:fb_sig_api_key])
>>> >         >       end
>>> >         >
>>> >         >
>>> >         > since facebook_params calls verify_signature
& ensures
>>> the
>>> >         > request comes from facebook.
>>> >         >
>>> >         >
>>> >         > Alternatively, we could be verify the params
signature
>>> in
>>> >         > request_comes_from_facebook.  That may be a
better
>>> solution
>>> >         > since the request_comes_from_facebook method is a
little
>>> >         > misleading (as david has just demonstrated).
>>> >         >
>>> >         >
>>> >         > Also, we throw an exception on a bad signature,
so we
>>> should
>>> >         > have a catch somewhere in the process.
>>> >         >
>>> >         >
>>> >         > - kevin
>>> >         >
>>> >         >
>>> >         >
>>> >         >
>>> >         >
>>> >         >
>>> >         >
>>> >         > On Feb 25, 2009, at 1:10 PM, David Clements
wrote:
>>> >         >
>>> >         > > Cool thanks for taking a look.  Looks like I
can
>>> just add
>>> >         > > a condition to that call
>>> >         > >
>>> >         > > if request_comes_from_facebook?
>>> >         > >
>>> >         > > I''ll try to get to it later today.
>>> >         > >
>>> >         > > Dave
>>> >         > >
>>> >         > > On 2/25/09, vincent chu <vincentchu at
gmail.com> wrote:
>>> >         > >         Hi David ---
>>> >         > >
>>> >         > >         I took a look at your fix. Though
I''m somewhat
>>> >         > >         unfamiliar with exactly what you
want to do, I
>>> >         > >         think it would be prudent to
validate that the
>>> >         > >         incoming params hash actually
originates from
>>> >         > >         facebook before using the parameters
to
>>> reset the
>>> >         > >         adapter. This way, you never touch
the adapter
>>> >         > >         until you''re sure that
it''s Facebook sending
>>> the
>>> >         > >         request, and not some malicious
actor.
>>> >         > >
>>> >         > >         Cheers,
>>> >         > >
>>> >         > >         Vince
>>> >         > >         ----
>>> >         > >
>>> >         > >
>>> >         > >                 >
>>> >         > >                 >
>>> >         > >                 > On 2/24/09, vincent chu
>>> >         > >                 > <vincentchu at
gmail.com> wrote:
>>> >         > >                 >         Hi all ---
>>> >         > >                 >
>>> >         > >                 >         In the course
of
>>> developing our
>>> >         > >                 >         Facebook
connect app, we
>>> >         > >                 >         realized that
there was a
>>> >         > >                 >         security hole
in
>>> Facebooker that
>>> >         > >                 >         allows any
malicious user to
>>> >         > >                 >         change the
state of the
>>> >         > >                 >         Facebooker
module and
>>> crash any
>>> >         > >                 >         controller/view
that uses
>>> >         > >                 >         Facebooker to
capture a
>>> Facebook
>>> >         > >                 >         session. For
Facebook
>>> connect
>>> >         > >                 >         apps, this
could
>>> potentially be
>>> >         > >                 >         in any view
that uses the
>>> >         > >                 >        
"set_facebook_session"
>>> >         > >                 >         before_filter.
>>> >         > >                 >
>>> >         > >                 >         All the
malicious user has
>>> to do
>>> >         > >                 >         is send a
malformed HTTP
>>> request
>>> >         > >                 >         similar to:
>>> >         > >                 >
>>> >         > >                 >        
http://my.rails.app.com/some_controller/?fb_sig_api_key=you_are_pwned
>>> >         > >                 >
>>> >         > >                 >         The problem
comes in the
>>> >         > >                 >        
''set_adapter'' method of
>>> >         > >                 >        
''facebooker/lib/facebooker/
>>> rails/controller.rb'' where Facebooker will attempt to load
an
>>> adapter from the params hash if fb_sig_api_key is in the request  
>>> (ignoring the configuration found in the facebooker.yml file). In  
>>> this case, Facebooker would dutifully set the api_key to  
>>> "you_are_pwned" and any subsequent call to Facebooker
would try
>>> and use "you_are_pwned" as the api_key, causing it to
crash the
>>> site.
>>> >         > >
>>> >         > >
>>> >         > >
>>> >         > >
>>> >         > >
>>> >         >
>>> >         >
>>> >         > _______________________________________________
>>> >         > Facebooker-talk mailing list
>>> >         > Facebooker-talk at rubyforge.org
>>> >         >
http://rubyforge.org/mailman/listinfo/facebooker-talk
>>> >         >
>>> >
>>> >         --
>>> >         Mike Mangino
>>> >         http://www.elevatedrails.com
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > _______________________________________________
>>> > Facebooker-talk mailing list
>>> > Facebooker-talk at rubyforge.org
>>> > http://rubyforge.org/mailman/listinfo/facebooker-talk
>>>
>>>
>>>
>>> _______________________________________________
>>> Facebooker-talk mailing list
>>> Facebooker-talk at rubyforge.org
>>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>>
>> _______________________________________________
>> Facebooker-talk mailing list
>> Facebooker-talk at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/facebooker-talk
>
> --
> Mike Mangino
> http://www.elevatedrails.com
>
>
>
> _______________________________________________
> Facebooker-talk mailing list
> Facebooker-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/facebooker-talk
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/facebooker-talk/attachments/20090225/5c58eddd/attachment-0001.html>