Issues with escaping of strings

[Aaron]

I just found out the hard way that a "\" character is not escaped when
passed to a client. Injecting some message ending with a \ can cause a
javascript syntax error on the client.

If meteor is deployed on a site where a user can inject traffic into
it, a user might be able to crash all connected meteor clients
connected streams.

Can someone with access to the official code please change it to
replace all "\" with "\\" before serving to connected clients?

Thanks,

Aaron

[Kevin Peno]

This has been brought up before. The reasoning behind the lack of escaping is that meteor is designed to be client agnostic (which most people seem to agree on). So, because of this, you will need to escapse said strings yourself.

 

For more on the debate and the ultimate decision, please see this thread:

 

http://groups.google.com/group/meteorserver/browse_thread/thread/d984e18e80f5fa61/6dcebc3d5beffb1d

 

I had made a suggestion there to allow for "plugins" that determined the output (thus handling odd cases like string escaping), but no response was given.

Kevin Peno

[Aaron]

Ah, I should have searched but I just assumed it was a bug. I had no
idea meteor aimed to be client agnostic. I assumed JS + Flash would be
about it.
I don't know if I like the idea of plugin's though, they introduce
overhead in the form of plugin management and the only one I can think
of is escaping.

The server documentation states this:

Message template, content to send for each event: ~text~, ~id~,
~channel~ and ~timestamp~ will be replaced by the appropriate values.
Format: String, Default: <script>p(~id~,"~channel~","~text~");</script>
\r\n

What about just adding an ~escapedtext~ as a possible argument? All
format agnostic behaviour will continue to function as is and the only
thing that needs to change is the MessageTemplate for the javascript
case.

Patch below if someone likes this idea enough to push it into the main
repo.

Aaron

-----------------

diff --git a/meteor_daemon/Meteor/Config.pm b/meteor_daemon/Meteor/
Config.pm
index 6309b1d..3f2849e 100644
--- a/meteor_daemon/Meteor/Config.pm
+++ b/meteor_daemon/Meteor/Config.pm
@@ -85,7 +85,7 @@ package Meteor::Config;
MaxTime => 0,

'Message template, ~text~, ~id~, ~channel~ and ~timestamp~ will be
replaced by the appropriate values',
- MessageTemplate => '<script>p
(~id~,"~channel~","~text~");</script>\r\n',
+ MessageTemplate => '<script>p
(~id~,"~channel~","~escapedtext~");</script>\r\n',

'Interval at which PingMessage is sent to all persistent subscriber
connections. Must be at least 3 if set higher than zero. Set to zero
to disable.',
PingInterval => 5,
@@ -395,4 +395,4 @@ EOT
}

diff --git a/meteor_daemon/Meteor/Message.pm b/meteor_daemon/Meteor/
Message.pm
index eb9ac03..dee2c81 100644
--- a/meteor_daemon/Meteor/Message.pm
+++ b/meteor_daemon/Meteor/Message.pm
@@ -61,6 +61,8 @@ sub newWithID {
$self->{'timestamp'}=time;
$self->{'id'}=$id;
$self->{'text'}=$text;
+ $self->{'escapedtext'} = $text;
+ $self->{'escapedtext'} =~ s/\\/\\\\/g;

$::Statistics->{'unique_messages'}++;

@@ -75,6 +77,8 @@ sub setText {
my $text=shift || '';

$self->{'text'}=$text;
+ $self->{'escapedtext'} = $text;
+ $self->{'escapedtext'} =~ s/\\/\\\\/g;
}

sub channelName {
@@ -128,4 +132,4 @@ sub messageWithTemplate {

}


On Oct 19, 2:10 am, Kevin Peno <kevinp...@gmail.com> wrote:
> This has been brought up before. The reasoning behind the lack of escaping
> is that meteor is designed to be client agnostic (which most people seem to
> agree on). So, because of this, you will need to escapse said strings
> yourself.
>
> For more on the debate and the ultimate decision, please see this thread:
>

> http://groups.google.com/group/meteorserver/browse_thread/thread/d984...

[Andrew Betts]

Kevin - sorry I didn't pick up on your plugin request in the earlier
thread. I think implementing plugins could be interesting, and could
be done in a number of ways - it could be argued that right now the
ability to define differently named transports with different headers,
footers and message templates amounts to a plugin system, and in order
to achieve what is being discussed here, you only really need an
extension to this templating principle to allow for string
manipulation in the templates, rather than a more comprehensive plugin-
hook system.

So rather than simply dropping the data directly into template strings
defined in the config file, we could have Meteor check for the
existence of a filter class for that transport, and if one exists,
pass the output through the filter before it is inserted into the
template.

Such a solution would have a separation of the template content for a
transport from the string processing code that manipulates the data,
so for better encapsulation if may be better to simply have each
transport defined by a class that offers an interface for rendering
the necessary messages, for example (for a Meteor::Subscriber)

my $transport = Meteor::Transports->getTransport($self->{'mode'});
my $outputstring = $transport->renderMessage(id, channel, text);

And the template text for the message would be within the transport
class.

I don't really have the time to give to this right now, but if someone
wants to have a crack at it, feel free.

Aaron - Thanks for the suggestion but I disagree with ~escapedText~
because you're assuming that the escaping method that will be required
would be to simply prefix all \ characters with an extra \. Escaping
for some situations might require this, but it's not a generic
solution so it would simply lead to a requirement for ~escapedText2~
when we come across someone that wants a different set of characters
to be escaped in a different way.

Andrew

[Kevin Peno]

Hey Andrew,

 

That's basically all I was speaking about. Just some sort of filter/serializer check for the data itself based on an "Accept" header sent in through the request headers. So that meteor could remain neutral and serve multiple clients. The only thing I was suggetsing is that it was made in a plugable fashion so that people could drop in custom filters at any time.

 

-- Kevin

[Aaron]

Thanks for the reply Andrew,

Glad the project is still alive!

I figured my patch might be too much of a single-use-case to warrant
inclusion. :)

Second idea that is not-quite a plugin: Why not just support the
evaluation of a perl string specified in the config file?

- Aaron

On Oct 27, 1:51 am, Kevin Peno <kevinp...@gmail.com> wrote:
> Hey Andrew,
>
> That's basically all I was speaking about. Just some sort of
> filter/serializer check for the data itself based on an "Accept" header sent
> in through the request headers. So that meteor could remain neutral and
> serve multiple clients. The only thing I was suggetsing is that it was made
> in a plugable fashion so that people could drop in custom filters at any
> time.
>
> -- Kevin
>