Skip to content
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

Several protocol violations or bugs in NanoMQ #1782

Open
songxpu opened this issue May 14, 2024 · 47 comments
Open

Several protocol violations or bugs in NanoMQ #1782

songxpu opened this issue May 14, 2024 · 47 comments
Assignees

Comments

@songxpu
Copy link

songxpu commented May 14, 2024

Describe the bug
Hi, I found something on the NanoMQ that is contrary to the protocol specification description (protocol violation or logic bug).
For tracking purposes, I will report all results under this Issue.

Environment Details

  • NanoMQ version 0.21.8
  • Operating system and version: Ubuntu 20.04

Client SDK
If possible include the mqtt sdk you used to connect to nanomq
Minimal C test cases are perferred.

Additional context
Add any other context about the problem here.

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv3.1.1:

If the Client supplies a zero-byte ClientId with CleanSession set to 0, the Server MUST respond to the CONNECT Packet with a CONNACK return code 0x02 (Identifier rejected) and then close the Network Connection [MQTT-3.1.3-8].

Replay such packet:

echo 105300044d515454048cd2c000000014626275737356364252756938646c70694379633300106c6e50763233507757527865744b4375001d5574436d5142567a716a4a523046394f566d535a74695258644a584f30 | xxd -p -r | nc 127.0.0.1 1883

image

The unintended behavior is that NanoMQ returns a CONNACK message where the return code is SUCCESS.

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv3.1.1:

If the User Name Flag is set to 0, the Password Flag MUST be set to 0 [MQTT-3.1.2-22].

Replay:

echo 105300064d5149736470036e52f1000b77756f79306c734876744500166d7936563836586f4d543773546e684334713671706b000172001d416f705a556739564c51736a75726657675938434d7738686d73365271 | xxd -p -r | nc 127.0.0.1 1883

The expected behavior is to reject such erroneous packets, but NanoMQ received and responded correctly.

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The Response Topic MUST NOT contain wildcard characters [MQTT-3.3.2-14]

However, if we send such packet like:

echo 103900044d5154540502003c2627ffffffff22000a2600046b657931000676616c7565312600046b657932000676616c75653200066e616e6f6d713c6800023254d5fa5302c24453370800012b3223c7c026001b546c4d336634713267796a516468723051576963636f66326649650004536f487026001b546c4d336634713267796a516468723051576963636f66326649650004536f48706c4357794c386e31773353666c58 | xxd -p -r | nc 172.17.0.6 1883

I debugged and found that NanoMQ returns PUBREC messages unintentionally and there is no check in the code for the contents of the response topic.

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The Server MUST validate that the reserved flag in the CONNECT packet is set to 0 [MQTT-3.1.2-3]. If the reserved flag is not 0 it is a Malformed Packet.

But we unexpectedly received CONNACK, replay such packet:

echo 105500044d51545405abc0a03016001c42336262383631465a54334151796762466768376d6b49497771754b17011901260001310002396c27c3567cb3000c374930333264347462396e32000a476f3031623970793744 | xxd -p -r | nc 172.17.0.6 1883

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

[MQTT-3.1.2-13]  If the Will Flag is set to 0, then Will Retain MUST be set to 0.

Replay packet
The packet is a Connect (Will Flag is set to 0, but Will Retain is set to 1).

echo 103300044d5154540560ae9d1826000e62377a3353495932364f4d49415600054275353942000654364b4e70390006446737575757 | xxd -p -r | nc 127.0.0.1 1883

image

NanoMQ sends the corresponding CONNACK with the success code to the client unexpectedly.

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The Server MUST NOT send more than one CONNACK in a Network Connection [MQTT-3.2.0-2].

But strangely, NanoMQ returned two CONNACKs for such requests, and the expected behavior should be to only return the first CONNACK.

echo 101300044d5154540502003c0321001400033132332003009000 | xxd -p -r | nc 127.0.0.1 1883

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The Content Type MUST be a UTF-8 Encoded String as defined in [section 1.5.4] [MQTT-3.3.2-19].

If we send the follow CONNECT packet (the content type contains "0xff")

echo 10e90100044d51545405362b33381140a8288315001d795374694f3074736a5462326e6d31736d4b77414b3432696755693841190122aec3260001500003546c3227ced40e9800064b635556324b810102f2402d8c03001f394b7a6c4b696d417031306f5465ff6d7a76347055326c666b57596943374d08001b475568736e537034495a4f31766568356e754f566f6a346370685009001bbd0b33e3e4bede81711998c0e7a5bbe5bc9771b4876cd489545a622600036477770016413531354d767036737a4f41665074417a4f574c50300016684f74336b52656e46334e324679714b465568785942000162 | xxd -p -r | nc 127.0.0.1 1883

Unexpectedly, the server returned a successful response message and saved such properties.

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

[MQTT-3.3.2-9] A Client MUST NOT send a PUBLISH packet with a Topic Alias greater than the Topic Alias Maximum value returned by the Server in the CONNACK packet.
[MQTT-3.3.2-12] A Server MUST accept all Topic Alias values greater than 0 and less than or equal to the Topic Alias Maximum value that it returned in the CONNACK packet.

In testing, it was found that if NanoMQ returns a CONNACK message (where the Topic Alias MAX value is 46512) and the client continues to send a PUBLISH message with a Topic Alias value of 65391, NanoMQ will still parse it correctly and respond, but the expectation is that it should treat such a message as an error.

echo  107c00044d5154540580a5753917001901210cb822b5b026000d6836536138326c4d6c68346970001d6c6e4256514f55624845384f74666e61626a343179487550696a64546b00167a6448453378656156463437614a74334d7630713347001e4f6e3150536d4d5865684a356575305832304f5259514b7138645550333333b001000d30346354435735643164534636895e3d023a4bc32e03000b305a555a68583755416438080011306e4d764f7377563530726b555650316209001065bab3a87c7b6c2015dbbd18851c56b923ff6f56376b66756275583445594e415a756737436959677867315233314d37715278446566734c515457654374433258786b50766c62795735303668306d654c6e527a417a306870674848524a516d42395a593438366642445939385464714a484a77  | xxd -p -r | nc 172.17.0.6 1883

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The DUP flag MUST be set to 0 for all QoS 0 messages [MQTT-3.3.1-2].

But it seems that NanoMQ did not check this rule and consider it valid.

mosquitto_sub -t "YCGo8KZY3a005HC0iI1LTUtsOrK" -h 172.17.0.6

echo 106600044d515454050058293f1700190021786922469426001a426d644777554a6c51427155496471466676313434357751624600167839316c415475656c7552673069564d436469373044001a43546c6b4a3170376654784e66386d5457436f6476495a57326c388401001b5943476f384b5a593361304f3548434f6a49314c545574734f724b53010103001346586b4847476d746b4f6756746b5a744241690900125805c310df29a346319f083402afbbba308b0b9bc9b2502600027359001a624d54663445586a4d505059384f577443534d573432674e6a76475464744f6b554575ff355241646635627830ce00 | xxd -p -r | nc 172.17.0.6 1883

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

[MQTT-3.3.4-6] A PUBLISH packet sent from a Client to a Server MUST NOT contain a Subscription Identifier.

After testing, NanoMQ violates this rule.

echo 106100044d5154540540b3b037119afb60e317001901215c5326000f766e72366d4541644d78553c44327800173049574d36324268715a6179524b5a62536749534a31360013317675535434755733374e64397846585a38570008676235716d5836363554000841684b3146454962f71a16010109000c79a506aff5eef39ed5210cd60bba849b523155596942337761334c376e765936573739413862666a46414e4e3172647544345773415778724a6667386d3258653363 | xxd -p -r | nc 172.17.0.6 1883

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

[MQTT-3.6.1-1]

Bits 3,2,1 and 0 of the Fixed Header in the PUBREL packet are reserved and MUST be set to 0,0,1 and 0 respectively. The Server MUST treat any other value as malformed and close the Network Connection.
However, the situation is different:

# The PUBREL (the reserve field in fixed header is 0, 0, 0, 0)
echo 107900044d5154540574625e0a1700190121de6022e03400126c5141593872614674494b73536a7956444d250101027f4801a708001b4a43394d424c416c5033654d704d457a626a5177527259663777330008465a416b4f4f4367001638344d77536b3430774f526451496463617259773337000636725930564c603ac6b000361f0017736d73496a39436331524a6d7541307644425a373968792600136e656f3666454367794c4a457948686e64696f000479326175 | xxd -p -r | nc 172.17.0.6 1883 

The expected behavior was considered illegal and the network connection was closed, but the corresponding request was unexpectedly received.
image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

It is a Protocol Error to include Request Problem Information more than once, or to have a value other than 0 or 1.

If we send a Connect (the value of Resequest Problem Information is set to 2), FlashMQ returns the CONNACK with successful code unexpectedly.

echo 105600044d5154540580404724118580d11d16000478697441170219012600075869563769414100083237746a6f4c3972001169587932653251324d6a3845697435517100126e63424f613833736f464e4e4d5944366442 | xxd -p -r | nc 172.17.0.6 1883

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

It is a Protocol Error to include Authentication Data if there is no Authentication Method.

Replay:

echo 107500044d515454054069d62216001a3836444175657452417668423854684a735a7474744935793341274e45fab6001974496e5645793058343872467a735361747763776c6849787a001d575a796d784c4e55317a335a7175744936434e673748576f34307a4f42000c4b4c32484b46385933343065 | xxd -p -r | nc 172.17.0.6 1883

image

NanoMQ unexpectedly returns CONNACK with the success code.

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

21 (0x15) Byte, Identifier of the Authentication Method.

Followed by a UTF-8 Encoded String containing the name of the authentication method used for extended authentication

However, NanoMQ does not verify that the value of this attribute is UTF-8 encoded, and then responds with a CONNACK.

image
image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

Similarly,

38 (0x26) Byte, Identifier of the User Property.
Followed by a UTF-8 String Pair.

However, NanoMQ does not verify that the value of this attribute is UTF-8 encoded, and then responds with a CONNACK.

echo 10d40100044d515454056ea3136415001a3061784d52367454306b617071576b30547a784f513858666e5516000c544e435937427545474d447a1901218a2b2248e526001bfffe4a6b474a33767549363567397269724e4c626256793843764c000b6f644b7a4a373341475675277f21bf19000e6d4a45327079426b47524e34527a1701002600030eeb66000d4f504a676a5578f1e3750a436a0012555563376b6251524c754d725a586f6e6a6800096e794c6b4a44784a77001c424d6771576b4c6f4c396c484c59736f7a67325675595854637a3442 | xxd -p -r | nc 172.17.0.6 1883

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

It is Protocol Error to include the Request Response Information more than once, or to have a value other than 0 or 1.

But we send a CONNECT (the Request Response Information is set to 50), NanoMQ did not reject such packet.

echo 103700044d5154540580081105193222a5140016787a4b6458694f633255337050765479776a7843646c000d594d3337384e5636594d45424d | xxd -p -r | nc 172.17.0.6 1883

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

It is a Protocol Error to include the Message Expiry Interval more than once.

Replay:

echo 108a0100044d515454059469d30519012181a700154e4838466b6a54617152696e326c797052316b50664103001c4935556b465530434d3778366d736371444e5a777a6b35654637386508000950624a747043434d4a0900132cc6f6912ac734b5e6a8b2acdc301886dd273600095a6f6a6632473268710007526a744d756576000b64364d61664257353733593c8d0100036e6b45dd3b510252ab83340252ab833403001854556873333351354b6f42643261696c4258364c6a547138080005724f4a4e6a09001e06ec9601ab2f750b9df8c182d84259566a61556c59d1a95fd38688652caa23409657777a6d484d344e5342766631556c5248653130516865757a665768594b787957636c5a7143394b713848737070504942687469 | xxd -p -r | nc 127.0.0.1 1883

I also found this is labeled as TODO in source code.

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv3.1.1:

[MQTT-2.3.1-1]
SUBSCRIBE, UNSUBSCRIBE, and PUBLISH (in cases where QoS > 0) Control Packets MUST contain a non-zero 16-bit Packet Identifier.

Replay such packet:

echo 105d00044d515454040c8e3c0018546a37786f3258556551443047644e385950706d51613251001c54374578706f4e686d4679485874384b4c483872554247764935385500196f65486351376f6a694d48596355524d3341533052686b7164a2110000000d357a386333384e6f416a54305a | xxd -p -r | nc 172.17.0.6 1883

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The Subscription Identifier can have the value of 1 to 268,435,455.

But if we send a packet containing too large values of the Subscription Identifier, the expectation is to reject such packets, but NanoMQ processes them normally.

echo 107900044d5154540574625e0a1700190121de6022e03400126c5141593872614674494b73536a7956444d250101027f4801a708001b4a43394d424c416c5033654d704d457a626a5177527259663777330008465a416b4f4f4367001638344d77536b3430774f526451496463617259773337000636725930564c829d018577060be8d5aafd0e0005623679527302001b47797a4242475278435279725258516a325669464e496a337444440c0001332c0001572a001d503345616967345677696b63696a746c70586968325841325158616f4d05001548704d36474a546e5a6c3566636475514c6d365256290003793734050011734257454b707755616d6d66694249583216001176314652585a6a6b4e4e716f4b5266736c26 | xxd -p -r | nc 172.17.0.6 1883

image

image

@songxpu
Copy link
Author

songxpu commented May 14, 2024

According to the specification of MQTTv5.0:

The Client or Server sending the PUBACK packet MUST use one of the PUBACK Reason Codes [MQTT-3.4.2-1].

However, if we transmit a PUBACK packet containing an invalid reason code that is not specified in Table 3-4, such as 0x01, the expected behavior would be to treat this request as erroneous, yet NanoMQ seemingly handles it without issue. Upon examining the code within the function nni_mqtt_pubres_decode(), it appears that no validation is performed to ensure the reason_code's validity.

echo 10800100044d51545405c20001411701212e3b22464426001a5a554c7742485a7a676a6e6579664f77447230496a374a46304c001a38436f4c643657616b6a6a4e74306c436b6f78536b593143356c001b6d6c33456b677234313272796d6d707a5a7664534e6c735965594200086732514e52776a38000b4743684f424f6c7a7967624004b65d0100320f0005746f706963000100746f706963  | xxd -p -r | nc 127.0.0.1 1883

image

@JaylinYu
Copy link
Member

If the Client supplies a zero-byte ClientId

You really have a deep look of MQTT protocol

@JaylinYu
Copy link
Member

If the Client supplies a zero-byte ClientId

You really have a deep look of the MQTT protocol.
Here we indeed make small tweak of implementation, for users convenience. nanomq assign new clientid to this client, just like MQTT5.
And we think such modification is better for users, regarding usability.

@JaylinYu
Copy link
Member

JaylinYu commented May 14, 2024

According to the specification of MQTTv5.0:

The Response Topic MUST NOT contain wildcard characters [MQTT-3.3.2-14]

However, if we send such packet like:

echo 103900044d5154540502003c2627ffffffff22000a2600046b657931000676616c7565312600046b657932000676616c75653200066e616e6f6d713c6800023254d5fa5302c24453370800012b3223c7c026001b546c4d336634713267796a516468723051576963636f66326649650004536f487026001b546c4d336634713267796a516468723051576963636f66326649650004536f48706c4357794c386e31773353666c58 | xxd -p -r | nc 172.17.0.6 1883

I debugged and found that NanoMQ returns PUBREC messages unintentionally and there is no check in the code for the contents of the response topic.

image

image

Return PUBREC is expected since you send QOS2, but ignoring wildcard is definitely a flaw

@JaylinYu
Copy link
Member

Response Topic MUST NOT contain

Indeed, but we leave the topic verify for client SDK for now, will fix this later

@JaylinYu
Copy link
Member

According to the specification of MQTTv5.0:

The Server MUST NOT send more than one CONNACK in a Network Connection [MQTT-3.2.0-2].

But strangely, NanoMQ returned two CONNACKs for such requests, and the expected behavior should be to only return the first CONNACK.

echo 101300044d5154540502003c0321001400033132332003009000 | xxd -p -r | nc 127.0.0.1 1883

image

Client sends CONNACK which is considered as a malformed packet.
Good catch again!

@JaylinYu
Copy link
Member

According to the specification of MQTTv5.0:

The Content Type MUST be a UTF-8 Encoded String as defined in [section 1.5.4] [MQTT-3.3.2-19].

If we send the follow CONNECT packet (the content type contains "0xff")

echo 10e90100044d51545405362b33381140a8288315001d795374694f3074736a5462326e6d31736d4b77414b3432696755693841190122aec3260001500003546c3227ced40e9800064b635556324b810102f2402d8c03001f394b7a6c4b696d417031306f5465ff6d7a76347055326c666b57596943374d08001b475568736e537034495a4f31766568356e754f566f6a346370685009001bbd0b33e3e4bede81711998c0e7a5bbe5bc9771b4876cd489545a622600036477770016413531354d767036737a4f41665074417a4f574c50300016684f74336b52656e46334e324679714b465568785942000162 | xxd -p -r | nc 127.0.0.1 1883

Unexpectedly, the server returned a successful response message and saved such properties.

image

image

same issue as response_topic, nanomq doesnt verify it for now, leave it to client.
but yes, we shall add verification in the future

@JaylinYu
Copy link
Member

According to the specification of MQTTv5.0:

[MQTT-3.3.2-9] A Client MUST NOT send a PUBLISH packet with a Topic Alias greater than the Topic Alias Maximum value returned by the Server in the CONNACK packet.
[MQTT-3.3.2-12] A Server MUST accept all Topic Alias values greater than 0 and less than or equal to the Topic Alias Maximum value that it returned in the CONNACK packet.

In testing, it was found that if NanoMQ returns a CONNACK message (where the Topic Alias MAX value is 46512) and the client continues to send a PUBLISH message with a Topic Alias value of 65391, NanoMQ will still parse it correctly and respond, but the expectation is that it should treat such a message as an error.

echo  107c00044d5154540580a5753917001901210cb822b5b026000d6836536138326c4d6c68346970001d6c6e4256514f55624845384f74666e61626a343179487550696a64546b00167a6448453378656156463437614a74334d7630713347001e4f6e3150536d4d5865684a356575305832304f5259514b7138645550333333b001000d30346354435735643164534636895e3d023a4bc32e03000b305a555a68583755416438080011306e4d764f7377563530726b555650316209001065bab3a87c7b6c2015dbbd18851c56b923ff6f56376b66756275583445594e415a756737436959677867315233314d37715278446566734c515457654374433258786b50766c62795735303668306d654c6e527a417a306870674848524a516d42395a593438366642445939385464714a484a77  | xxd -p -r | nc 172.17.0.6 1883

image

image

Cool, I dont think we should limit the maximum topic alias number. so I simply make it max.
will push a modify PR

@JaylinYu
Copy link
Member

It is a Protocol Error to include Request Problem Information more than once, or to have a value other than 0 or 1.

well, nanomq doest support Authentication yet.

@JaylinYu
Copy link
Member

Hi bug hunter, most issues here are not critical, I will come by later to fix them

@JaylinYu
Copy link
Member

JaylinYu commented May 15, 2024

I prefer to leave reason code as it is, I believe community deserve better reasoncode definition, we gonna make our own.
Hi bug hunter @songxpu
Really curious about your fuzzing tool. How could u manage to produce such an accurate binary flow to trigger the bug? we once invested on PolyU for a fuzzing tool, I believe you can make a great fuzz engine

@songxpu
Copy link
Author

songxpu commented May 15, 2024

I prefer to leave reason code as it is, I believe community deserve better reasoncode definition, we gonna make...

Hi @JaylinYu. I do agree with all of your thoughts above, protocol specifications are not always absolute, and if there is a better solution it is worth pushing for it.

Also thanks for the affirmation of our fuzzing tool.
I'm actually working on academic research related to MQTT and will probably release it in the near future once it's completely finished.

@JaylinYu
Copy link
Member

Cool, I have wrapped fix in 0.21.10, happily to get feedback from you

@JaylinYu
Copy link
Member

JaylinYu commented May 18, 2024

Similarly,

38 (0x26) Byte, Identifier of the User Property.
Followed by a UTF-8 String Pair.

However, NanoMQ does not verify that the value of this attribute is UTF-8 encoded, and then responds with a CONNACK.

echo 10d40100044d515454056ea3136415001a3061784d52367454306b617071576b30547a784f513858666e5516000c544e435937427545474d447a1901218a2b2248e526001bfffe4a6b474a33767549363567397269724e4c626256793843764c000b6f644b7a4a373341475675277f21bf19000e6d4a45327079426b47524e34527a1701002600030eeb66000d4f504a676a5578f1e3750a436a0012555563376b6251524c754d725a586f6e6a6800096e794c6b4a44784a77001c424d6771576b4c6f4c396c484c59736f7a67325675595854637a3442 | xxd -p -r | nc 172.17.0.6 1883

image

image

Just realize that you are sending a UTF-8 str pair; the length field is located before str.
I will discard the new checker in new release

@songxpu
Copy link
Author

songxpu commented Jun 16, 2024

Hi, there are some other bugs related to the above that I missed unreported before, I'll update them here.
Since NanoMQ has been updated, I revalidated them in the latest commit (#ffd0e7b3f776feae6ebcdd7be5f4ba0be228a3c1).

@songxpu
Copy link
Author

songxpu commented Jun 16, 2024

MQTT v5:

[MQTT-4.8.2-2]
The ShareName MUST NOT contain the characters "/", "+" or "#", but MUST be followed by a "/" character. This "/" character MUST be followed by a Topic Filter.

Shred Subscriptions format:
$share/{ShareName}/{filter}

Replay:

echo 101000044d5154540502003c0321001400008215000100000f2473686172652f312b2f746f70696300 | xxd -p -r | nc 127.0.0.1 1883

The expected behavior should be to stop such a packet, but the broker receives it normally and can receive the publish message normally.
image

@songxpu
Copy link
Author

songxpu commented Jun 16, 2024

One interesting issue is that NanoMQ seems to be parsing the PUBLISH attribute field and instead of effectively stopping such a message when it encounters a malformed attribute value, it receives it and allows it to be forwarded to other subscribers, but this poses a risk to other subscribing clients.
Reproduce:

# Sub
./mosquitto_sub -t "2T" -p 1883 -V 5

# Pub
echo 103900044d5154540502003c2627ffffffff22000a2600046b657931000676616c7565312600046b657932000676616c75653200066e616e6f6d713a6800023254d5fa5302c24453370800012b3223c7c026001b546c4d336634713267796a516468723051576963636f66326649650004536f487026001b546c4d336634713267796a516468723051576963636f66326649650004536f48706c4357794c386e31773353666c58 | xxd -p -r | nc 127.0.0.1 1883

image

Also, we found that mosquitto_sub crashed.
image

@songxpu
Copy link
Author

songxpu commented Jun 16, 2024

Then, another is NanoMQ ACL Flaw.
Description:

When we turn on NanoMQ's Privilege Access Control to disable all users from subscribing to system topics that begin with $SYS, the acl.conf configuration file looks like this.

image

Now, theoretically any request from a subscribed client that subscribes to $SYS/ will be blocked, but we can get around it with a wildcard theme: the

# Subscriber:
mosquitto_sub -t "+/brokers/+" 
# Publisher:
mosquitto_pub -t "1/1/1" -m 123

You can see that the information about the $SYS topic was successfully obtained.

image

In fact, the MQTT specification refers to scenarios such as.

[MQTT-4.7.2-1]
The Server MUST NOT match Topic Filters starting with a wildcard character (# or +) with Topic Names beginning with a $ character.

@JaylinYu JaylinYu reopened this Jul 2, 2024
@JaylinYu
Copy link
Member

JaylinYu commented Jul 2, 2024

Some issues are not critical to fix. will revisit it soon

@songxpu
Copy link
Author

songxpu commented Jul 26, 2024

# MQTT v5
3.3.2.3.2 Payload Format Indicator

· 0 (0x00) Byte Indicates that the Payload is unspecified bytes, which is equivalent to not sending a Payload Format Indicator.
· 1 (0x01) Byte Indicates that the Payload is UTF-8 Encoded Character Data. The UTF-8 data in the Payload MUST be well-formed UTF-8 as defined by the Unicode specification [Unicode] and restated in RFC 3629 [RFC3629].

According to this description, when the Payload Format Indicator in the PUBLISH message is set to 1, the payload in the PUBLISH message must be UTF-8 encoded. However, it appears that NanoMQ does not validate this.

PoC:

echo 106e00044d515454056e866e0711c4f23d2e17010012764d78635176754e4b6163615164477247521d0300027a4e260009577852505166527043000a6e467067664149535563000f487a4134464c4d3352526c6231486c00104e364958486246414654713051374b79000559304c56433d3900087751586170725838415f060101090001b8423663546c627950717a306b363233664d3249426c36717a6a5a474546456a6f3845363038fe | xxd -r -p | nc 127.0.0.1 1883

image

@songxpu
Copy link
Author

songxpu commented Jul 26, 2024

Similarity,

# MQTT v5
3.1.3.2.3 Payload Format Indicator

Followed by the value of the Payload Format Indicator, either of:
· 0 (0x00) Byte Indicates that the Will Message is unspecified bytes, which is equivalent to not sending a Payload Format Indicator.
· 1 (0x01) Byte Indicates that the Will Message is UTF-8 Encoded Character Data. The UTF-8 data in the Payload MUST be well-formed UTF-8 as defined by the Unicode specification [Unicode] and restated in RFC 3629 [RFC3629]

According to this description, when the Payload Format Indicator in the CONNECT message is set to 1, the will message in the CONNECT message must be UTF-8 encoded. However, it appears that NanoMQ does not validate this.

POC:

106300044d51545405a6fd850c11987da8451900270b5434c60004534d763907010118e7f5bf8600104371794d4a3374706e773872744b364300126c397a7a71763353325647714f6a475951fe0016354278463574494f433735413474654d745869475467

image

@songxpu
Copy link
Author

songxpu commented Jul 26, 2024

# MQTTv5
3.3.2.3.2 Payload Format Indicator
Followed by the value of the Payload Format Indicator, either of: 0, 1.

But if we send a packet that the Payload Format Indicator is set to 32 to broker, NanoMQ will not reject it.

Packet:

10c30100044d515454056c77d82011629d51d217011901216c01221b58260008596f6c62443937310004725751730014454265377462773971454a7273576f77337037536a01010295ce5e3008000a36544c527069726b4c6d09000ecd70cf079fa2088bce36926d8e05189d1c754326001e6c4a4a736f4f56736f44334b7a6e7178724c5a4236693546425143504944001d613674384b6b366f6f32587a733179697643347a564c4a3172337a7a4700023144000137000e442b4a6c77366848686c686f4e4b
3560000444565251ab1821012002b77d4ce709001296ce5d71eef90c5561f8fc49bae5a29ea4140bbaa2b02c64707673596f737852394562636b4c4232544b7a4a4c30356571354f384e70715341437430506361375a7664456c41624b4a48534d56

image

@songxpu
Copy link
Author

songxpu commented Jul 26, 2024

MQTTv5:
If the Will Flag is set to 0, then the Will QoS MUST be set to 0 (0x00) [MQTT-3.1.2-11]

But if we sen such an invalid packet that the will flag is set to 0 and the will qos is set to 1, the broker will not reject it.
Packet:

104400044d515454054ad0e71411c0392d8d1701190021bc6222974927c1205f64000957664b367031777478001853695a6c6576506a717a6c356d7a46544741425a4a36567a

image

@songxpu
Copy link
Author

songxpu commented Jul 26, 2024

In MQTT 5.0, the requester can specify an expected Response Topic in the request message. After taking appropriate action based on the request message, the responder publishes a response message to the Response Topic carried in the request. If the requester has subscribed to that Response Topic, it will receive the response.

Ref 1: https://www.emqx.com/en/blog/mqtt5-request-response

In MQTT 5.0, I believe this property field should not be allowed to be empty, because it will play a role in message transmission in some scenarios.

Packet:

10a10100044d51545405a6d79b13170122e432260002766100024d5127c1f00b75000c3667713668573039766670314102dd44adf20300144b526a5343646e53427066393358395531756b470800002600094835774f764d6e4f37001466345a314b636936466648545078614461337063000f476234475670636c737132496830440005307648626400194d46344f6d6c72764951773877755a6a666f3331636c457278

image

@songxpu

This comment was marked as resolved.

@mosaicws
Copy link

mosaicws commented Jul 29, 2024

I noticed that when a client communicates with the broker using MQTT version 5.0, after the client sends a PUBLISH packet with QoS==1, the broker returns a PUBACK in MQTT version 3.1.1 format. I believe that such protocol version downgrading is not expected behavior and may impact some clients.

# MQTTv5
+----------------+-----------------+------------+------------------+
| Fixed Header   | Message ID      | Reason Code| Properties       |
+----------------+-----------------+------------+------------------+
| 0x40           | Message ID (2 bytes) | 0x00  | Properties (if any) |
+----------------+-----------------+------------+------------------+
#MQTTv3.1.1
+----------------+-----------------+
| Fixed Header   | Message ID      |
+----------------+-----------------+
| 0x40           | Message ID (2 bytes) |
+----------------+-----------------+

replay.py

import socket
import binascii

broker_address = '127.0.0.1'
broker_port = 1883

messages = [
    '10800100044d51545405c20001411701212e3b22464426001a5a554c7742485a7a676a6e6579664f77447230496a374a46304c001a38436f4c643657616b6a6a4e74306c436b6f78536b593143356c001b6d6c33456b677234313272796d6d707a5a7664534e6c735965594200086732514e52776a38000b4743684f424f6c7a796762',
    '320f0005746f706963000100746f706963'
]

def send_and_receive_message(sock, message):
    binary_message = binascii.unhexlify(message)
    sock.sendall(binary_message)
    response = sock.recv(1024)
    hex_response = binascii.hexlify(response).decode()
    return hex_response

def main():
    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    sock.connect((broker_address, broker_port))
    
    try:
        for message in messages:
            hex_response = send_and_receive_message(sock, message)
            print(f"Sent message: {message}")
            print(f"Received response: {hex_response}")
    finally:
        sock.close()

if __name__ == "__main__":
    main()

NanoMQ: image

image

I think it does and is likely the cause of the errors I listed here:
#1855

Hope they are able to fix these inconsistencies and bugs.

@songxpu
Copy link
Author

songxpu commented Jul 29, 2024

I noticed that when a client communicates with the broker using MQTT version 5.0, after the client sends a PUBLISH packet with QoS==1, the broker returns a PUBACK in MQTT version 3.1.1 format. I believe that such protocol version downgrading is not expected behavior and may impact some clients.

# MQTTv5
+----------------+-----------------+------------+------------------+
| Fixed Header   | Message ID      | Reason Code| Properties       |
+----------------+-----------------+------------+------------------+
| 0x40           | Message ID (2 bytes) | 0x00  | Properties (if any) |
+----------------+-----------------+------------+------------------+
#MQTTv3.1.1
+----------------+-----------------+
| Fixed Header   | Message ID      |
+----------------+-----------------+
| 0x40           | Message ID (2 bytes) |
+----------------+-----------------+

replay.py

import socket
import binascii

broker_address = '127.0.0.1'
broker_port = 1883

messages = [
    '10800100044d51545405c20001411701212e3b22464426001a5a554c7742485a7a676a6e6579664f77447230496a374a46304c001a38436f4c643657616b6a6a4e74306c436b6f78536b593143356c001b6d6c33456b677234313272796d6d707a5a7664534e6c735965594200086732514e52776a38000b4743684f424f6c7a796762',
    '320f0005746f706963000100746f706963'
]

def send_and_receive_message(sock, message):
    binary_message = binascii.unhexlify(message)
    sock.sendall(binary_message)
    response = sock.recv(1024)
    hex_response = binascii.hexlify(response).decode()
    return hex_response

def main():
    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    sock.connect((broker_address, broker_port))
    
    try:
        for message in messages:
            hex_response = send_and_receive_message(sock, message)
            print(f"Sent message: {message}")
            print(f"Received response: {hex_response}")
    finally:
        sock.close()

if __name__ == "__main__":
    main()

NanoMQ: image
image

I think it does and is likely the cause of the errors I listed here: #1855

Hope they are able to fix these inconsistencies and bugs.

Although the issue you mentioned is indeed related to inconsistencies and bugs, I made a mistake in this case.
It is a legitimate one, because MQTT states: "The third byte of the PUBACK variable header is the Reason Code. If the remaining length is 2, it indicates the use of Reason Code 0x00 (Success)." (Omission allowed)

@JaylinYu
Copy link
Member

The ShareName MUST NOT contain the characters "/", "+" or "#"

You have reported this before, however I received more security complaints after I pushed a fix...
So I just decided to quote it for now. you can check 3903 line of mqtt_codec.c

also, I assume some of your issue shall be addressed by SDK from the first place, such as response topic.
Anyway, it is just my assumption, will continue to solidate it when I got time.

JaylinYu added a commit to nanomq/NanoNNG that referenced this issue Jul 30, 2024
nanomq/nanomq#1782
will qos & will flag

Signed-off-by: jaylin <jaylin@emqx.io>
JaylinYu added a commit to nanomq/NanoNNG that referenced this issue Jul 30, 2024
nanomq/nanomq#1782
will qos & will flag

Signed-off-by: jaylin <jaylin@emqx.io>
JaylinYu added a commit that referenced this issue Jul 30, 2024
Signed-off-by: jaylin <jaylin@emqx.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants