Skip to content
Permalink
Browse files

Don't expose SSL secrets over Ice/D-Bus.

I think there's a good excuse for this: we don't expose a user's password
hash over Ice, which is actually a pain in the backside for migrating
servers via Ice. So in the interests of consistency, I think it's probably
better to not expose the SSL private key or it's passphrase over Ice either,
particularly since SSL keys are a bit of a "cat out the bag" thing.

Note that at present, the SSL passphrase isn't exposed via meta either - the
"key" field of meta.GetDefaultConf() contains a plaintext copy of the key,
but I left the check in anyway in case we change that for any reason.

When the user specifically requests things that ought to be secret over Ice,
we raise an exception rather than have it silently fail, to hopefully reduce
the amount of head-scratching some poor script writer has to do.

Finally, more or less do the same thing over D-Bus.
  • Loading branch information...
fwaggle authored and mkrautz committed May 1, 2016
1 parent 3434ff8 commit 028812012451c55f334383fa92bf2534787b43d1
Showing with 21 additions and 3 deletions.
  1. +10 −1 src/murmur/DBus.cpp
  2. +3 −1 src/murmur/Murmur.ice
  3. +8 −1 src/murmur/MurmurIce.cpp
@@ -856,7 +856,10 @@ void MetaDBus::getConf(int server_id, const QString &key, const QDBusMessage &ms
if (! ServerDB::serverExists(server_id)) {
MurmurDBus::qdbc.send(msg.createErrorReply("net.sourceforge.mumble.Error.server", "Invalid server id"));
} else {
value = ServerDB::getConf(server_id, key).toString();
if (key == "key" || key == "passphrase")
MurmurDBus::qdbc.send(msg.createErrorReply("net.sourceforge.mumble.Error.writeonly", "Requested read of write-only field."));
else
value = ServerDB::getConf(server_id, key).toString();
}
}

@@ -876,6 +879,9 @@ void MetaDBus::getAllConf(int server_id, const QDBusMessage &msg, ConfigMap &val
MurmurDBus::qdbc.send(msg.createErrorReply("net.sourceforge.mumble.Error.server", "Invalid server id"));
} else {
values = ServerDB::getAllConf(server_id);

values.remove("key");
values.remove("passphrase");
}
}

@@ -893,6 +899,9 @@ void MetaDBus::getLog(int server_id, int min_offset, int max_offset, const QDBus

void MetaDBus::getDefaultConf(ConfigMap &values) {
values = Meta::mp.qmConfig;

values.remove("key");
values.remove("passphrase");
}

void MetaDBus::setSuperUserPassword(int server_id, const QString &pw, const QDBusMessage &msg) {
@@ -264,6 +264,8 @@ module Murmur
exception InvalidSecretException extends MurmurException {};
/** This is thrown when the channel operation would excede the channel nesting limit */
exception NestingLimitException extends MurmurException {};
/** This is thrown when you ask the server to disclose something that should be secret. */
exception WriteOnlyException extends MurmurException {};

/** Callback interface for servers. You can supply an implementation of this to receive notification
* messages from the server.
@@ -485,7 +487,7 @@ module Murmur
* @param key Configuration key.
* @return Configuration value. If this is empty, see {@link Meta.getDefaultConf}
*/
idempotent string getConf(string key) throws InvalidSecretException;
idempotent string getConf(string key) throws InvalidSecretException, WriteOnlyException;

/** Retrieve all configuration items.
* @return All configured values. If a value isn't set here, the value from {@link Meta.getDefaultConf} is used.
@@ -914,7 +914,10 @@ static void impl_Server_id(const ::Murmur::AMD_Server_idPtr cb, int server_id) {
#define ACCESS_Server_getConf_READ
static void impl_Server_getConf(const ::Murmur::AMD_Server_getConfPtr cb, int server_id, const ::std::string& key) {
NEED_SERVER_EXISTS;
cb->ice_response(u8(ServerDB::getConf(server_id, u8(key)).toString()));
if (key == "key" || key == "passphrase")
cb->ice_exception(WriteOnlyException());
else
cb->ice_response(u8(ServerDB::getConf(server_id, u8(key)).toString()));
}

#define ACCESS_Server_getAllConf_READ
@@ -926,6 +929,8 @@ static void impl_Server_getAllConf(const ::Murmur::AMD_Server_getAllConfPtr cb,
QMap<QString, QString> values = ServerDB::getAllConf(server_id);
QMap<QString, QString>::const_iterator i;
for (i=values.constBegin();i != values.constEnd(); ++i) {
if (i.key() == "key" || i.key() == "passphrase")
continue;
cm[u8(i.key())] = u8(i.value());
}
cb->ice_response(cm);
@@ -1644,6 +1649,8 @@ static void impl_Meta_getDefaultConf(const ::Murmur::AMD_Meta_getDefaultConfPtr
::Murmur::ConfigMap cm;
QMap<QString, QString>::const_iterator i;
for (i=meta->mp.qmConfig.constBegin();i != meta->mp.qmConfig.constEnd(); ++i) {
if (i.key() == "key" || i.key() == "passphrase")
continue;
cm[u8(i.key())] = u8(i.value());
}
cb->ice_response(cm);

0 comments on commit 0288120

Please sign in to comment.
You can’t perform that action at this time.