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

broker 1.5 conf `use_identity_as_username true` TLS is broken #833

Closed
idcrook opened this Issue May 25, 2018 · 9 comments

Comments

Projects
None yet
6 participants
@idcrook

idcrook commented May 25, 2018

On a raspberry pi using raspbian package Version: 1.5-0mosquitto1, in the broker logfile, the logged username no longer matches the Common Name found in the client certificate.

output from: sudo less /var/log/mosquitto/mosquitto.log

1527190662: New client connected from 10.0.1.88 as 389IB4d3dWUllZpTLXUKcI (c1, k600, u'^E').
[...]
1527192380: New client connected from 10.0.1.51 as mosqpub/2404-rpi1 (c1, k60, u'^D').

the u'^E' and u'^D' in the log message above used to reflect the Common Name in the certificate, e.g. in output from 1.4.15 it is u'rpip3'

1526905503: New client connected from 10.0.1.88 as 389IB4d3dWUllZpTLXUKcI (c1, k600, u'rpip3').
@ptjm

This comment has been minimized.

Show comment
Hide comment
@ptjm

ptjm May 26, 2018

I think the problem isn't the logging but the actual user name is set incorrectly. At least, I changed my ACL file to have "User ^N" (with an actual control-N), and the client started working again. I'm seeing this on a NetBSD system.

ptjm commented May 26, 2018

I think the problem isn't the logging but the actual user name is set incorrectly. At least, I changed my ACL file to have "User ^N" (with an actual control-N), and the client started working again. I'm seeing this on a NetBSD system.

@idcrook idcrook changed the title from mosquitto broker 1.5 `use_identity_as_username true` in mosquitto.conf changed behavior in logging to mosquitto broker 1.5 `use_identity_as_username true` in mosquitto.conf changed behavior May 26, 2018

@ptjm

This comment has been minimized.

Show comment
Hide comment
@ptjm

ptjm May 26, 2018

I haven't prepared a pull request for this but the problem is in handle_connect.c. In version 1.4.15, the commonName was extracted like this:

i = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
name_entry = X509_NAME_get_entry(name, i);
context->username = _mosquitto_strdup((char *)ASN1_STRING_data(X509_NAME_ENTRY_get_data(name_entry)));

but in 1.5, it's like this:

i = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
name_entry = X509_NAME_get_entry(name, i);
context->username = mosquitto__strdup((char *)X509_NAME_ENTRY_get_data(name_entry));

These appear to be independent fixes to the same issue, but the fix in 1.4.15 (commit fff7416) is correct in this case. I've verified that 1.5 works correctly with a call to ASN1_STRING_data() inserted as in 1.4.15.

ptjm commented May 26, 2018

I haven't prepared a pull request for this but the problem is in handle_connect.c. In version 1.4.15, the commonName was extracted like this:

i = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
name_entry = X509_NAME_get_entry(name, i);
context->username = _mosquitto_strdup((char *)ASN1_STRING_data(X509_NAME_ENTRY_get_data(name_entry)));

but in 1.5, it's like this:

i = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
name_entry = X509_NAME_get_entry(name, i);
context->username = mosquitto__strdup((char *)X509_NAME_ENTRY_get_data(name_entry));

These appear to be independent fixes to the same issue, but the fix in 1.4.15 (commit fff7416) is correct in this case. I've verified that 1.5 works correctly with a call to ASN1_STRING_data() inserted as in 1.4.15.

@idcrook idcrook changed the title from mosquitto broker 1.5 `use_identity_as_username true` in mosquitto.conf changed behavior to broker 1.5 conf `use_identity_as_username true` TLS is broken Jun 3, 2018

@bricewge

This comment has been minimized.

Show comment
Hide comment
@bricewge

bricewge Jun 27, 2018

Contributor

I got the same issue.
1.4.14:

1530095225: New client connected from 192.168.10.153 as esp32_4B0094 (c1, k60, u'device.test').

1.5:

1530092237: New client connected from 192.168.10.153 as esp32_4B0094 (c1, k60, u'
                                                                                 ').

The culprit seems to be the commit 9c6a5f3#diff-72af1417df06c9efe29630bd226a62b3.
So reverting the change to src/handle_connect.c may broke Windows builds by regressing on #656.

Contributor

bricewge commented Jun 27, 2018

I got the same issue.
1.4.14:

1530095225: New client connected from 192.168.10.153 as esp32_4B0094 (c1, k60, u'device.test').

1.5:

1530092237: New client connected from 192.168.10.153 as esp32_4B0094 (c1, k60, u'
                                                                                 ').

The culprit seems to be the commit 9c6a5f3#diff-72af1417df06c9efe29630bd226a62b3.
So reverting the change to src/handle_connect.c may broke Windows builds by regressing on #656.

bricewge added a commit to bricewge/mosquitto that referenced this issue Jun 27, 2018

bricewge added a commit to bricewge/mosquitto that referenced this issue Jun 27, 2018

bricewge added a commit to bricewge/mosquitto that referenced this issue Jun 27, 2018

bricewge added a commit to bricewge/mosquitto that referenced this issue Jun 27, 2018

@ptjm

This comment has been minimized.

Show comment
Hide comment
@ptjm

ptjm Jul 13, 2018

@bricewge the problem with your change is that it won't work with openssl before version 1.1. The essential change from the previous commit is using X509_NAME_ENTRY_get_data() rather than accessing the structure members directly, so I think using the code from 1.4.15 is probably the way to go until older versions of openssl all go away.
On another note, there's a well-known security flaw with handling x509 certificates exhibited by this code. The common name in the certificate can have embedded nulls, so it's possible to spoof clients who don't expect that. e.g., if you can get a cert signed with common name someone@example.com\000@example2.com, mosquitto will treat that as if it were just someone@example.com. Ideally, there should be some code in there to compare ASN1_STRING_length() to the strlen of ASN1_STRING_data().

ptjm commented Jul 13, 2018

@bricewge the problem with your change is that it won't work with openssl before version 1.1. The essential change from the previous commit is using X509_NAME_ENTRY_get_data() rather than accessing the structure members directly, so I think using the code from 1.4.15 is probably the way to go until older versions of openssl all go away.
On another note, there's a well-known security flaw with handling x509 certificates exhibited by this code. The common name in the certificate can have embedded nulls, so it's possible to spoof clients who don't expect that. e.g., if you can get a cert signed with common name someone@example.com\000@example2.com, mosquitto will treat that as if it were just someone@example.com. Ideally, there should be some code in there to compare ASN1_STRING_length() to the strlen of ASN1_STRING_data().

bricewge added a commit to bricewge/mosquitto that referenced this issue Jul 17, 2018

bricewge added a commit to bricewge/mosquitto that referenced this issue Jul 17, 2018

@bricewge bricewge referenced this issue Jul 17, 2018

Closed

fix #833 #868

@toast-uz

This comment has been minimized.

Show comment
Hide comment
@toast-uz

toast-uz Jul 31, 2018

Contributor

Note there is a comment as bellow in ChangeLog.txt on mosquitto v1.5.

- Support for openssl versions 1.0.0 and 1.0.1 has been removed as these are
  no longer supported by openssl.
Contributor

toast-uz commented Jul 31, 2018

Note there is a comment as bellow in ChangeLog.txt on mosquitto v1.5.

- Support for openssl versions 1.0.0 and 1.0.1 has been removed as these are
  no longer supported by openssl.
@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Aug 1, 2018

Contributor

@toast-uz don't forget that openssl 1.0.2 is LTS, and very much still supported. What exactly is the "information" you feel is required here given your tagging?

Contributor

karlp commented Aug 1, 2018

@toast-uz don't forget that openssl 1.0.2 is LTS, and very much still supported. What exactly is the "information" you feel is required here given your tagging?

@toast-uz

This comment has been minimized.

Show comment
Hide comment
@toast-uz

toast-uz Aug 1, 2018

Contributor

@karlp sorry the discussion moved to the PR #868 .

Contributor

toast-uz commented Aug 1, 2018

@karlp sorry the discussion moved to the PR #868 .

bricewge added a commit to bricewge/mosquitto that referenced this issue Aug 8, 2018

fix #833
Signed-off-by: Brice Waegeneire <brice.wge@gmail.com>

ralight added a commit that referenced this issue Aug 8, 2018

fix #833
Signed-off-by: Brice Waegeneire <brice.wge@gmail.com>

ralight added a commit that referenced this issue Aug 8, 2018

Fix `use_identity_as_username true` not working.
Closes #833.

Thanks to David Crook and Brice Waegeneire.

Bug: #833

Signed-off-by: Roger A. Light <roger@atchoo.org>
@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Aug 8, 2018

Contributor

The PR #868 is now merged into the fixes branch, if you get the chance please take a look and close the issue if you are happy. Thanks for the report!

Contributor

ralight commented Aug 8, 2018

The PR #868 is now merged into the fixes branch, if you get the chance please take a look and close the issue if you are happy. Thanks for the report!

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

@bricewge

This comment has been minimized.

Show comment
Hide comment
@bricewge

bricewge Aug 8, 2018

Contributor

It's good with me, you can close the issue (I can't do it myself).

Contributor

bricewge commented Aug 8, 2018

It's good with me, you can close the issue (I can't do it myself).

@ralight ralight closed this Aug 8, 2018

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