Publish denied on bridge connection when per_listener_settings is true

[wollud1969]

My configuration is

pid_file /var/run/mosquitto.pid
log_dest file /var/log/mosquitto/mosquitto.log

persistence true
persistence_location /var/lib/mosquitto/

per_listener_settings true


listener 1883 0.0.0.0
allow_anonymous true


listener 8883 0.0.0.0
password_file /etc/mosquitto/pwfile
allow_anonymous false
cafile /etc/mosquitto/ssl/ca/server-ca.crt
certfile /etc/mosquitto/ssl/internal-mqtt-broker.crt
keyfile /etc/mosquitto/ssl/internal-mqtt-broker.pem
tls_version tlsv1

connection to-external
address 172.16.1.10:8883
remote_password xxx
remote_username homegear
local_password xxx
local_username argus
bridge_insecure true
bridge_tls_version tlsv1
bridge_cafile /etc/mosquitto/ssl/ca/server-ca.crt
topic # both

Whenever a message from the other side of the bridge is received I see

1528996259: Denied PUBLISH from local.homegear.to-external (d0, q0, r0, m0, 'IoT/Test', ... (5 bytes))

in the logfile.

As soon as I remove the per_listener_settings entry everything is fine and the message is accepted.

Removing the local_password and local_username from the configuration doesn't change the behavior.

Is there anything wrong with my configuration - I don't think so - or is this a bug?

Cheers,
Wolfgang

[wollud1969]

My suspect is the context handling again. When a message is received on the bridge connection
my debug output macd2 appears.

int mosquitto_acl_check_default(struct mosquitto_db *db, struct mosquitto *context, const char *topic, int access)
{
[...]
	if(db->config->per_listener_settings){
		printf("macd1\n");
		if(!context->listener) {
			printf("macd2\n");
			return MOSQ_ERR_ACL_DENIED;
		}
		security_opts = &context->listener->security_options;
	}else{

This also happens when persistence is set to false.

[wollud1969]

Maybe this helps, although I don't completely understand the consequences:

In mosquitto_acl_check_default there is an explicit check whether a context is associated with a bridge. This check appears after the check for the listener mentioned above:

int mosquitto_acl_check_default(struct mosquitto_db *db, struct mosquitto *context, const char *topic, int access)
{
	char *local_acl;
	struct mosquitto__acl *acl_root;
	bool result;
	int i;
	int len, tlen, clen, ulen;
	char *s;
	struct mosquitto__security_options *security_opts = NULL;

	if(!db || !context || !topic) return MOSQ_ERR_INVAL;

	if(db->config->per_listener_settings){
		printf("macd1 %p\n", context);
		if(!context->listener) {
			printf("macd2\n");
			return MOSQ_ERR_ACL_DENIED;
		}
		security_opts = &context->listener->security_options;
	}else{
		security_opts = &db->config->security_options;
	}
	if(!security_opts->acl_list && !security_opts->acl_patterns){
			return MOSQ_ERR_PLUGIN_DEFER;
	}

	if(context->bridge) return MOSQ_ERR_SUCCESS;
	if(access == MOSQ_ACL_SUBSCRIBE) return MOSQ_ERR_SUCCESS; /* FIXME - implement ACL subscription strings. */
	if(!context->acl_list && !security_opts->acl_patterns) return MOSQ_ERR_ACL_DENIED;

However, in bridge__new context__init is called to create a new context, but - different than in net__socket_accept - the listener member of the context is not filled (sure, it's no listener but it's the active part of the connection), so it stays null, which triggers the listener check with my debug output macd2.

Moving the bridge-check in mosquitto_acl_check_default in front of the listener-check actually would solve the problem.

int mosquitto_acl_check_default(struct mosquitto_db *db, struct mosquitto *context, const char *topic, int access)
{
[...]
	if(!db || !context || !topic) return MOSQ_ERR_INVAL;

	if(context->bridge) return MOSQ_ERR_SUCCESS;

	if(db->config->per_listener_settings){
		printf("macd1 %p\n", context);
		if(!context->listener) {
			printf("macd2\n");
			return MOSQ_ERR_ACL_DENIED;
		}
		security_opts = &context->listener->security_options;
	}else{

However, I don't know whether this would introduce a security whole.

Could anyone who is deeper in the code please comment? Thank you very much!

[wollud1969]

I've submitted a pull request to fix this issue.

[ralight]

Some clarification:

Bridges aren't associated directly with a listener, so they can't have per listener settings applied to them. They haven't had ACL checks in the past, so they shouldn't now. The question of whether there should be more checks on bridge topics is a good one, but not for this issue.

I've merged the PR, so am going to close this issue. If there is anything still outstanding please reopen and provide details.

[ralight]

Related pull request is #864.