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

A will published by a client on port with mount_point is later published without mount_point prefix #8

Closed
ralight opened this Issue Mar 15, 2016 · 1 comment

Comments

Projects
None yet
1 participant
@ralight
Contributor

ralight commented Mar 15, 2016

migrated from Bugzilla #487178
status UNCONFIRMED severity normal in component Mosquitto for 1.4
Reported in version 1.4 on platform PC
Assigned to: Roger Light

On 2016-02-03 18:43:08 -0500, Lance Riley wrote:

If a client connected to a broker port configured to prefix topics using configuration option "mount_point" registers a will topic, and subsequently fails, then the will topic is released by the broker without the mount_point prefix applied. This breaks the client isolation provided by the mount_point option.

To show the problem, edit mosquitto.conf to add "mount_point /ext" for a given non standard port (1884).

Prove normal operation by subscribing to the standard port that has no mount_point prefix defined and then publish via the port with the mount_point prefix:

echo "xxx" | mosquitto_pub -p 1884 -t /TEST -l --will-topic /TEST --will-payload will

Verbose subscribe client on standard port receives: "/ext/TEST xxx"

Now publish with no message from stdin and abort with ^C to trigger will publication:

mosquitto_pub -p 1884 -t /TEST -l --will-topic /TEST --will-payload will
^C

Verbose subscribe client on standard port receives: "/TEST will" which has not got the expected prefix.

Additionally a client subscribed to all topics on the non standard broker port with the mount_point prefix defined will not receive the will message as it is generated without the prefix and so filtered out for this port.

On 2016-02-11 08:41:38 -0500, Lance Riley wrote:

Just to clarify the point about client isolation, this issue allows a client to inject arbitrary topics and messages into the broker that are not constrained by the mount_point restrictions imposed on it by the port it is connected through.

In a secure installation where the root topic port were protected by TLS, etc. but also having an additional insecure port but with scope restricted by a mount_point prefix, a client connected to that insecure port would be able to inject topics without the prefix by first registering a will and then closing the connection to simulate a client failure without going through the normal closure sequence.

On 2016-02-11 16:29:37 -0500, Roger Light wrote:

Thanks very much for the report and the useful instructions.

This is now fixed in the fixes branch and will be part of 1.4.8 shortly.

On 2016-02-17 04:54:37 -0500, Lance Riley wrote:

Hi Roger.

Thanks for the fix. I've noticed however that under the original test conditions (mount_point /ext) with version 1.4.8 on Raspbian, I am now seeing the correct will mount point prefix, but the final character of the will topic is cropped.

mosquitto_pub -p 1884 -t /NORMAL -l --will-topic /WILL --will-payload will
^C

Publishes:

/ext/WIL will

I've changed the bug status back to unconfirmed - I hope that is the correct thing to do.

Thanks,
Lance.

On 2016-02-29 12:17:37 -0500, Lance Riley wrote:

Hi Roger.

The following change fixes the problem ...

Edit: mosquitto-1.4.8/src/read_handle_server.c

slen = strlen(context->listener->mount_point) + strlen(will_topic);
will_topic_mount = _mosquitto_malloc(slen+1);
if(!will_topic_mount){
rc = MOSQ_ERR_NOMEM;
goto handle_connect_error;
}
snprintf(will_topic_mount, slen, "%s%s", context->listener->mount_point, will_topic);

In the snprintf() change arg 2 to "slen+1" to match the malloc. Arg 2 is the max count including the /0 terminator.

From the man page:

"The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str."

Thanks,
Lance.

@ralight

This comment has been minimized.

Contributor

ralight commented Jun 13, 2016

This was fixed in 1.4.9.

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