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 EMQX #13045

Open
panhao410 opened this issue May 14, 2024 · 26 comments
Open

Several protocol violations or bugs in EMQX #13045

panhao410 opened this issue May 14, 2024 · 26 comments
Labels

Comments

@panhao410
Copy link

panhao410 commented May 14, 2024

Describe the bug
I found something on the EMQX 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

  • emqx v5.6
  • Operating system and version: Ubuntu 20.04

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

Additional context
Add any other context about the problem here.

@panhao410 panhao410 added the BUG label May 14, 2024
@panhao410
Copy link
Author

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 emqx received and responded correctly.
image

@panhao410
Copy link
Author

panhao410 commented May 14, 2024

According to the specification of MQTTv3.1.1:

If the User Name Flag is set to 1, a User Name MUST be present in the payload.

Replay:

echo 102700044d5154540482d05000193349503972474f63796a50496e32704e446b3551484a5252750000 | xxd -p -r | nc 127.0.0.1 1883

image

@panhao410
Copy link
Author

According to the specification of MQTTv5:

If the User Name Flag is set to 1, the User Name is the next field in the Payload. The User Name MUST be a UTF-8 Encoded String.

Replay:

echo 102300044d5154540582003c032100140010636c69656e74787878787878787878780001ff | xxd -p -r | nc 172.17.0.3 1883 

EMQX can return a 'Successful' Connect ACK message for this.
image

@panhao410
Copy link
Author

panhao410 commented May 15, 2024

According to the specification of MQTTv3.1.1:

The ClientId MUST be a UTF-8 encoded string as defined in Section 1.5.3.

Replay:

echo 100f00044d5154540402003c000331328f | xxd -p -r | nc 172.17.0.7 1883

image

@panhao410
Copy link
Author

According to the specification of MQTTv5:

The ClientId MUST be a UTF-8 encoded string.

Replay:

echo 104c00044d51545405806b56091700190027b34a02f6001c6a43764d696d71004453664875674e54764c6d5575706a4749544f70001831304e4a656446534554734330526356374f31377a6b4147 | xxd -p -r | nc 172.17.0.4 1883

image
image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

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 172.17.0.6 1883

image
image

@panhao410
Copy link
Author

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

@panhao410
Copy link
Author

panhao410 commented May 15, 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].

Replay:

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

image

@panhao410
Copy link
Author

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].

Replay:

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

image
image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

A PUBLISH packet sent from a Client to a Server MUST NOT contain a Subscription Identifier.

After testing, EMQX violates this rule.

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

image

@ieQu1
Copy link
Member

ieQu1 commented May 15, 2024

Hello,

Thanks, these are interesting finds. We'll implement stricter validation.

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

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:

echo 107900044d5154540574625e0a1700190121de6022e03400126c5141593872614674494b73536a7956444d250101027f4801a708001b4a43394d424c416c5033654d704d457a626a5177527259663777330008465a416b4f4f4367001638344d77536b3430774f526451496463617259773337000636725930564c603ac6b000361f0017736d73496a39436331524a6d7541307644425a373968792600136e656f3666454367794c4a457948686e64696f000479326175 | xxd -p -r | nc 172.17.0.5 1883 

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

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

If the User Name Flag is set to 1, the User Name is the next field in the Payload. The User Name MUST be a UTF-8 Encoded String.

Replay:

echo 101c00044d51545405c2003c03210014000000048f656d6f000470617373 | xxd -p -r | nc 172.17.0.2 1883

User names containing illegal UTF-8 hexadecimal characters, such as 0x8f, should theoretically be rejected by the broker.
image

@panhao410
Copy link
Author

panhao410 commented May 15, 2024

According to the specification of MQTTv5.0:

The Topic Name MUST be present as the first field in the PUBLISH packet Variable Header. It MUST be a UTF-8 Encoded String.

The Topic Filters MUST be a UTF-8 Encoded String.

The Topic Filters in an UNSUBSCRIBE packet MUST be UTF-8 Encoded Strings.

Replay:

# UNSUBSCRIBE
echo 104800044d5154540502198e1d215415260008744a613168307079000d77516d586452786d3835676132001e32746e6233766c6a304630344d317a6b3855744b45524a506a347579466aa28f01cb302d260016746e5a725a655353654471496a6a69334c6472686b4100127471745278344c7657457864724b70656e460006645535537a77000445624a490009364b59444d74495670001559777656754e706a53646d654b4f6b4c316e466c430012644b4a69677474757974696c544d70684152000172000339321a00114353696d34303034427951596846436373 | xxd -p -r | nc 172.17.0.4 1883

# SUBSCRIBE
echo 101000044d5154540502003c0321001400008209000100000339321a00 | xxd -p -r | nc 172.17.0.4 1883

# PUBLISH
echo 101000044d5154540502003c0321001400003007000339321a0031 | xxd -p -r | nc 172.17.0.4 1883

The topic in PUBLISH, SUBSCRIBE, and UNSUBSCRIBE messages is "39321a" and it's not UTF-8 encoded. Note: Messages can be transmitted between SUBSCRIBE and PUBLISH.
image
image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

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

Replay:

echo 10c10100044d515454052ee8504d1193e14c4a1600154b47574f55355341677341784d654a39437071656921958c22bac526001635795733507a4e773069426875566e626c504e32574e000f43794c714538595549326663643672001d59507459787a4b4d736374486e305050336c384466335848486258304c1d010008000e56466f324b4c6f526c327058346c090007687186e0884020001a5847674b484f58756c6e5847414f6639667866705533646c6951000e78334a795477747a70486b4d3277 | xxd -p -r | nc 172.17.0.4 1883

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

It is Protocol Error to include the Request Response Information more than once.

Replay:

echo 107a00044d5154540500222d5e15001a64726131783935676844325331373545704c6b4b53656d4b6c4e16000d635844386e3575486f6e716d721701190019012284c5260009634e4e6b6735746b570015786f443969767169773778687879667a504e486c4f27c02b8ade000f7945426a754848644b695633663051
 | xxd -p -r | nc 172.17.0.4 1883

image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

Identifier of the User Property. Followed by a UTF-8 String Pair.

Replay:

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

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

It is a Protocol Error to include the Payload Format Indicator more than once.

Replay:

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

image

@panhao410
Copy link
Author

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 172.17.0.5 1883

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

image
image

@panhao410
Copy link
Author

panhao410 commented May 15, 2024

According to the specification of MQTTv3.1.1:

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

Replay:

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

image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

It is a Protocol Error if the Maximum QoS field has the value 3.

Replay:

echo 105b00044d51545405c0b9623c21315226001c486637676c5a49424741465a3447366f6450447a3445514f394e653700134a69354677384b58367a765166486976655664270bca5ac2000972753758477073645000013400045139416982a701ffac050b839f91190019657234636f41335077357236636b666c4664566f705962557010000d6c6a706a62554a7556696a55430e001a68437a79536a71654c6e6a5078594375683547444a32564d37462c001e5766467753304c6a387034383968725138354d41694e61735461334356531500036f767300000c6642303168354f61757639712e001c546a6448714b466e5a35694c7367665a7a3337455743515034384b44ee00
 | xxd -p -r | nc 172.17.0.5 1883

image
image

@panhao410
Copy link
Author

panhao410 commented May 15, 2024

According to the specification of MQTTv5.0:

It is a Protocol Error to include the Response Topic more than once.

Replay:

# Single message sequence
10ae0100044d51545405c6f8d7021701000f46356d30636e4b34633958413030332c020748306803000026000449454148001b366f694536686241734132583462704368426b6a77355832314d450018416b484869364752656558436c53725a36534a6f5a393962001733555a41655261594764374341486f4f4a78474d756c300014425243504679514b6e42534a4373453552434c570018496451446b6a736d44366a66796d513556557a7a6c4a6870
3d4a00025959c10f1a010002d31586140800013108000132260003417968000350754f4e616b6937656a796a696e77434a66775573544e377376644f4c38544f32434e6431365972304c4f6e
ce00

#Merged message sequence
echo 10ae0100044d51545405c6f8d7021701000f46356d30636e4b34633958413030332c020748306803000026000449454148001b366f694536686241734132583462704368426b6a77355832314d450018416b484869364752656558436c53725a36534a6f5a393962001733555a41655261594764374341486f4f4a78474d756c300014425243504679514b6e42534a4373453552434c570018496451446b6a736d44366a66796d513556557a7a6c4a68703d4a00025959c10f1a010002d31586140800013108000132260003417968000350754f4e616b6937656a796a696e77434a66775573544e377376644f4c38544f32434e6431365972304c4f6ece00 | xxd -p -r | nc 172.17.0.5 1883

EMQX will return a PUBREC for PUBLISH messages and will also return a PINGRESP for subsequent PINGREQ, indicating that the PUBLISH messages have been received normally.
image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

It is a Protocol Error to include the Content Type more than once.

Replay:

echo 10950100044d515454054e28d00a1120b290b627036e6116000261453d01000800074679685377495218ef612d5926000c72376a72376d747062733257001b735a595569453539795a487333616b4833494e77496b4762494157000658415873325a00166c47547172693658526b536f4e724666496d486f5573001c306d54567544763978426f526b59374c3672346a433142514c503838318d010014556b7768636d324d5371307667584f696943623351010103001a4e624553556651574a394b354f58367333397a6b6a636157373203001a4e624553556651574a394b354f58367333397a6b6a636157373308000d6856344a4e7873487062727a390bbeaee12f52675542547a5373475876796d644d68373161785a336d715430305a414b5346524c746e55ce00  | xxd -p -r | nc 172.17.0.5 1883

image
EMQX directly receives and processes such messages, which violates the regulations.
image

@panhao410
Copy link
Author

According to the specification of MQTTv5.0:

It is a Protocol Error to include the Topic Alias value more than once.

Replay:

echo 101000044d5154540502003c03210014000030150006746f706963310623000a23000b313233313233 | xxd -p -r | nc 172.17.0.5 1883

EMQX explicitly violates this rule because the subscribing end that subscribes to the topic1 topic can receive this message.
image

@ieQu1
Copy link
Member

ieQu1 commented May 15, 2024

Hello @panhao410 ,

First of all, thanks for your work and for providing very detailed reproduction instructions. I did some digging, and there is a configuration parameter mqtt.strict_mode that enables a lot of the checks found by your tests: https://docs.emqx.com/en/enterprise/v5.6.1/hocon/#V-mqtt-S-mqtt-strict_mode

By default, strict_mode is disabled (we should probably change the default in the future to true). The reason why it is disabled is to provide compatibility with the clients taking some liberties with the standard out of the box. I think enabling this flag will make a lot of these problems disappear.

@zmstone
Copy link
Member

zmstone commented May 17, 2024

Hi @panhao410

Thank you so much for your detailed report.
We truly appreciate your effort to help us improve EMQX.
I will try to address the issues you've raised in groups for clarity.

Issues

Username and password flags

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

We've deliberately allowed flexibility in EMQX to accommodate various configurations, such as using the client ID as a username, or extracting the username from the certificate's common name, etc.

If the User Name Flag is set to 1, a User Name MUST be present in the payload.

Similarly, EMQX is designed to not strictly require a username even when this flag is set, as it can be overridden depending on the configuration.

UTF-8 enforcement

EMQX can be configured with mqtt.strict_mode=true to enforce UTF-8 checks. By default, it is set to false to maintain compatibility. We are considering enabling this by default starting from version 6.

High Priority Fixes

We will fix issues that might lead to a "protocol error," such as:

  • Will message with retain=1
  • QoS > 2
  • Packet ID = 0

Low Priority Fixes

Similarly, we plan to address other less critical issues that could disrupt protocol compliance:

  • Bad PUBACK reason code
  • PUBLISH with a subscription identifier
  • Reserved bits in PUBREL
  • DUP flag QoS 0 message
  • Authentication data without authentication method
  • Request/response information flag more than once
  • Payload Format Indicator more than once
  • Message Expiry Interval more than once
  • Response Topic more than once
  • Content Type more than once
  • Topic alias more than once

My thoughts in general

As stated in MQTT v5 spec 4.3.13, the level of strictness in adhering to the MQTT specification and checking for errors in packets depends on practical considerations. Definitions of Malformed Packet and Protocol Errors are detailed in section 1.2 Terminology. The balance we seek in error checking is influenced by:

  • The size and capability of the Client or Server implementation
  • The trust level in the sender and network to deliver and send correct MQTT Control Packets
  • The potential consequences of processing an incorrect packet

From my perspective, while these issues may not seem to cause practical harm to either the client or server, your insights are invaluable. I am eager to hear more from you, especially if you can provide practical scenarios where these issues might affect operations.
Thank you once again for your contributions and looking forward to your continued engagement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants