Skip to content
Permalink
Browse files Browse the repository at this point in the history
Strict meta channel recognition in server
This addresses a security vulnerability affecting user-added extensions
that implement access control for channels. These extensions typically
work by checking incoming messages whose channel is `/meta/subscribe`
and then performing some authentication routine before allowing the
message through.

However, the Server parses channels in a way that means any channel
namespaced under `/meta/subscribe` will also work as a subscription
request. For example if the client sends a message to the channel
`/meta/subscribe/x`, that will bypass most authentication extensions but
will still be interpreted by the server as a subscription request, and
the client will be subscribed to the requested channel. The client has
thus bypassed the user's access control policy.

Here we prevent this by using a strict equality check; only messages
whose channel is exactly `/meta/subscribe` will be interpreted as
subscription requests. The same pattern is applied to all other meta
channels.
  • Loading branch information
jcoglan committed Apr 27, 2020
1 parent d9b8d97 commit 65d297d
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 8 deletions.
16 changes: 12 additions & 4 deletions lib/faye/protocol/server.rb
Expand Up @@ -6,8 +6,6 @@ class Server
include Logging
include Extensible

META_METHODS = %w[handshake connect disconnect subscribe unsubscribe]

attr_reader :engine

def initialize(options = {})
Expand Down Expand Up @@ -107,9 +105,9 @@ def handle(message, local = false, &callback)
end

def handle_meta(message, local, &callback)
method = Channel.parse(message['channel'])[1]
method = method_for(message)

unless META_METHODS.include?(method)
unless method
response = make_response(message)
response['error'] = Faye::Error.channel_forbidden(message['channel'])
response['successful'] = false
Expand All @@ -123,6 +121,16 @@ def handle_meta(message, local, &callback)
end
end

def method_for(message)
case message['channel']
when Channel::HANDSHAKE then :handshake
when Channel::CONNECT then :connect
when Channel::SUBSCRIBE then :subscribe
when Channel::UNSUBSCRIBE then :unsubscribe
when Channel::DISCONNECT then :disconnect
end
end

def advize(response, connection_type)
return unless [Channel::HANDSHAKE, Channel::CONNECT].include?(response['channel'])

Expand Down
36 changes: 36 additions & 0 deletions spec/javascript/server/extensions_spec.js
Expand Up @@ -36,6 +36,42 @@ jstest.describe("Server extensions", function() { with(this) {
}})
}})

describe("with subscription auth installed", function() { with(this) {
before(function() { with(this) {
var extension = {
incoming: function(message, callback) {
if (message.channel === "/meta/subscribe" && !message.auth) {
message.error = "Invalid auth"
}
callback(message)
}
}
server.addExtension(extension)
}})

it("does not subscribe using the intended channel", function() { with(this) {
var message = {
channel: "/meta/subscribe",
clientId: "fakeclientid",
subscription: "/foo"
}
stub(engine, "clientExists").yields([true])
expect(engine, "subscribe").exactly(0)
server.process(message, false, function() {})
}})

it("does not subscribe using an extended channel", function() { with(this) {
var message = {
channel: "/meta/subscribe/x",
clientId: "fakeclientid",
subscription: "/foo"
}
stub(engine, "clientExists").yields([true])
expect(engine, "subscribe").exactly(0)
server.process(message, false, function() {})
}})
}})

describe("with an outgoing extension installed", function() { with(this) {
before(function() { with(this) {
var extension = {
Expand Down
36 changes: 36 additions & 0 deletions spec/ruby/server/extensions_spec.rb
Expand Up @@ -40,6 +40,42 @@ def incoming(message, callback)
end
end

describe "with subscription auth installed" do
before do
extension = Class.new do
def incoming(message, callback)
if message["channel"] == "/meta/subscribe" and !message["auth"]
message["error"] = "Invalid auth"
end
callback.call(message)
end
end
server.add_extension(extension.new)
end

it "does not subscribe using the intended channel" do
message = {
"channel" => "/meta/subscribe",
"clientId" => "fakeclientid",
"subscription" => "/foo"
}
engine.stub(:client_exists).and_yield(true)
engine.should_not_receive(:subscribe)
server.process(message, false) {}
end

it "does not subscribe using an extended channel" do
message = {
"channel" => "/meta/subscribe/x",
"clientId" => "fakeclientid",
"subscription" => "/foo"
}
engine.stub(:client_exists).and_yield(true)
engine.should_not_receive(:subscribe)
server.process(message, false) {}
end
end

describe "with an outgoing extension installed" do
before do
extension = Class.new do
Expand Down
18 changes: 14 additions & 4 deletions src/protocol/server.js
Expand Up @@ -13,8 +13,6 @@ var Class = require('../util/class'),
Socket = require('./socket');

var Server = Class({ className: 'Server',
META_METHODS: ['handshake', 'connect', 'disconnect', 'subscribe', 'unsubscribe'],

initialize: function(options) {
this._options = options || {};
var engineOpts = this._options.engine || {};
Expand Down Expand Up @@ -120,10 +118,10 @@ var Server = Class({ className: 'Server',
},

_handleMeta: function(message, local, callback, context) {
var method = Channel.parse(message.channel)[1],
var method = this._methodFor(message),
response;

if (array.indexOf(this.META_METHODS, method) < 0) {
if (method === null) {
response = this._makeResponse(message);
response.error = Error.channelForbidden(message.channel);
response.successful = false;
Expand All @@ -137,6 +135,18 @@ var Server = Class({ className: 'Server',
}, this);
},

_methodFor: function(message) {
var channel = message.channel;

if (channel === Channel.HANDSHAKE) return 'handshake';
if (channel === Channel.CONNECT) return 'connect';
if (channel === Channel.SUBSCRIBE) return 'subscribe';
if (channel === Channel.UNSUBSCRIBE) return 'unsubscribe';
if (channel === Channel.DISCONNECT) return 'disconnect';

return null;
},

_advize: function(response, connectionType) {
if (array.indexOf([Channel.HANDSHAKE, Channel.CONNECT], response.channel) < 0)
return;
Expand Down

0 comments on commit 65d297d

Please sign in to comment.