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

regression because of CVE-2017-7650: clientid with / character #462

Closed
wiebeytec opened this Issue Jun 14, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@wiebeytec

wiebeytec commented Jun 14, 2017

The / character is not allowed in the client ID since version 1.4.12. This breaks mosquitto_pub and mosquitto_sub, in generate_client_id, because they themselves use a /.

Even if that is changed, all versions installed by distros will break on this.

The / was included in the blacklist because it "may represent an additional risk". Perhaps it should be undone?

@hmvp

This comment has been minimized.

Show comment
Hide comment
@hmvp

hmvp Jun 15, 2017

Also plugins that do proper checks are effected by this!
We have to move to custom builds of mosquitto because we cannot disallow '/', '+' and '#' characters.

It would be nice to be able to opt out this unwanted security...

hmvp commented Jun 15, 2017

Also plugins that do proper checks are effected by this!
We have to move to custom builds of mosquitto because we cannot disallow '/', '+' and '#' characters.

It would be nice to be able to opt out this unwanted security...

@vavrecan

This comment has been minimized.

Show comment
Hide comment
@vavrecan

vavrecan Jun 16, 2017

What about cleaning up client_id after auth plugin check instead of enforcing policy before?

vavrecan commented Jun 16, 2017

What about cleaning up client_id after auth plugin check instead of enforcing policy before?

@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Jun 19, 2017

Contributor

It was a mistake not to change the autogenerated IDs for mosquitto_pub/sub. This is fixed in the fixes branch.

I think it will be required to allow / in usernames given their fairly widespread use there, but the spec does not expect / to be allowed in client ids.

Plugins that do proper checks are affected by this, yes, but in a survey of plugins that I could find, more were vulnerable than secure, so this was taken as the least worst option.

How about a plugin_supports CVE-2017-7650 option to indicate that the plugin already handles it?

Contributor

ralight commented Jun 19, 2017

It was a mistake not to change the autogenerated IDs for mosquitto_pub/sub. This is fixed in the fixes branch.

I think it will be required to allow / in usernames given their fairly widespread use there, but the spec does not expect / to be allowed in client ids.

Plugins that do proper checks are affected by this, yes, but in a survey of plugins that I could find, more were vulnerable than secure, so this was taken as the least worst option.

How about a plugin_supports CVE-2017-7650 option to indicate that the plugin already handles it?

@wiebeytec

This comment has been minimized.

Show comment
Hide comment
@wiebeytec

wiebeytec Jun 20, 2017

How about a plugin_supports CVE-2017-7650 option to indicate that the plugin already handles it?

That would work for me, although in my case a bit of a misnomer, because the auth plugin we wrote is not susceptible to the problem, i.e. user names or client IDs never appear in topics.

What about auth_plugin_cve_2017_7650_safe true?

wiebeytec commented Jun 20, 2017

How about a plugin_supports CVE-2017-7650 option to indicate that the plugin already handles it?

That would work for me, although in my case a bit of a misnomer, because the auth plugin we wrote is not susceptible to the problem, i.e. user names or client IDs never appear in topics.

What about auth_plugin_cve_2017_7650_safe true?

@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Jun 24, 2017

Contributor

auth_plugin_cve_2017_7650_safe true - I was hoping to have a single config option for this sort of situation so if something else comes up then it's just another value to add rather than a new option.

Any thoughts on a better name?

Contributor

ralight commented Jun 24, 2017

auth_plugin_cve_2017_7650_safe true - I was hoping to have a single config option for this sort of situation so if something else comes up then it's just another value to add rather than a new option.

Any thoughts on a better name?

ralight added a commit that referenced this issue Jun 27, 2017

[462] Add auth_plugin_deny_special_chars option.
Auth plugins can be configured to disable the check for +# in
usernames/client ids with the auth_plugin_deny_special_chars option.

Thanks to wiebeytec.

Bug: #462

ralight added a commit that referenced this issue Jun 27, 2017

[462] Relax CVE-2017-7650 checks.
Checks for '/' are no longer made, this character is a much lower risk
and is widely used in usernames.

Bug: #462
@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Jun 27, 2017

Contributor

This should be fixed in the upcoming 1.4.13 release, so I'm closing this.

Contributor

ralight commented Jun 27, 2017

This should be fixed in the upcoming 1.4.13 release, so I'm closing this.

@ralight ralight closed this Jun 27, 2017

@ralight ralight added this to the fixes-next milestone Jun 27, 2017

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