New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish denied on bridge connection when per_listener_settings is true #860

Closed
wollud1969 opened this Issue Jun 14, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@wollud1969
Contributor

wollud1969 commented Jun 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@wollud1969

wollud1969 Jun 14, 2018

Contributor

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.

Contributor

wollud1969 commented Jun 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@wollud1969

wollud1969 Jun 14, 2018

Contributor

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!

Contributor

wollud1969 commented Jun 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@wollud1969

wollud1969 Jun 21, 2018

Contributor

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

Contributor

wollud1969 commented Jun 21, 2018

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

@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Aug 8, 2018

Contributor

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.

Contributor

ralight commented Aug 8, 2018

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 ralight closed this Aug 8, 2018

@ralight ralight added this to the 1.5.1 milestone Aug 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment