Tigase security vulnerability: XMPP stanza smuggling via unescaped qutes (#15)

Closed

Activities

	wojciech.kapcia@tigase.net opened 3 years ago Tigase XMPP server suffers from a security vulnerability due to not escaping double quote characters when serializing parsed XML. This can be used to smuggle (or, if you prefer, inject) arbitrary attacker-controlled stanza in the XMPP server's output stream. A malicious client can abuse this vulnerability to send arbitrary XMPP stanzas to another client (including the control stanzas that are only meant to be sent by the server). Consider for example the following XML tag `<foo attr='aaa"bbb' />` which contains a single attribute enclosed in single quotes. When Tigase parses and then serializes the element, it will convert single quotes to double quotes, however it will not escape the double quote. Thus the output becomes `<foo attr="aaa"bbb" />` Which is invalid XML. The corresponding code for serializing attributes can be seen in https://github.com/tigase/tigase-xmltools/blob/b0c64df99f62b88bec7c152d52027369b6415ada/src/main/java/tigase/xml/Element.java#L845 To see how this issue can be used to smuggle arbitrary stanzas through the server, consider for example the following incoming stanza `<message ...> <body> <body a='a"/>' >text</body> <message a='a"/>' >text</message> <iq>...</iq> <message a='a">' /> <body a='a">' /> </body> </message>` When this stanza gets parsed by the server, the corresponding tree is `-message --body ---body with attribute name=a value=a"/> ----cdata=text ---message with attribute name=a value=a"/> ----cdata=text ---iq ---message with attribute name=a value=a"> ---body with attribute name=a value=a">` However, when such a stanza gets serialized and forwarded to the recipient client (or another XMPP server) it becomes `<message ...><body><body a="a"/>" >text</body><message a="a"/>" >text</message><iq>...</iq><message a="a">" /><body a="a">" /></body></message>` (single quoted attributes became double quoted) When the receiving client parses it, the corresponding tree is seen as `-message --body ---body with attribute name=a value=a ---cdata=" >text --message with attribute name=a value=a --cdata=" >text -iq -message with attribute name=a value=a --cdata=>" /> --body with attribute name=a value=a ---cdata=>" />` This works because, after the quote, we used '/>' instead of '>' and vice-versa to change the semantics of the closing tags. As can be seen in the tree above, the receiving client will consider the iq tag (that was originally a part of the message body tree) as a new stanza at the same "depth" in the XML tree as the message stanza. As mentioned above, this can be used to "smuggle" arbitrary stanzas through the XMPP server to the victim client. This can be used for message spoofing (making it appear a message was sent by a different sender), but also, depending on the XMPP extensions the client implements and what data is sent over XMPP, it can have impact beyond that (e.g. potentially redirecting the connection through a malicious XMPP server, code execution).
	Andrzej Wójcik (Tigase) commented 3 years ago Initial JMH benchmark to test original performance of the code responsible for parsing data. /Users/andrzej/Development/bin/graalvm-ce-java17-21.3.0/Contents/Home/bin/java -ea -Dproject.target=/Users/andrzej/Development/Tigase/tigase-xmltools/target/classes -Dproject.version=4.2.0-SNAPSHOT --illegal-access=permit -Didea.test.cyclic.buffer.size=1048576 -javaagent:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/lib/idea_rt.jar=53342:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/bin -Dfile.encoding=UTF-8 -classpath /Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/lib/idea_rt.jar:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/plugins/junit/lib/junit5-rt.jar:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/plugins/junit/lib/junit-rt.jar:/Users/andrzej/Development/Tigase/tigase-xmltools/target/test-classes:/Users/andrzej/Development/Tigase/tigase-xmltools/target/classes:/Users/andrzej/.m2/repository/org/openjdk/jmh/jmh-core/1.34/jmh-core-1.34.jar:/Users/andrzej/.m2/repository/net/sf/jopt-simple/jopt-simple/5.0.4/jopt-simple-5.0.4.jar:/Users/andrzej/.m2/repository/org/apache/commons/commons-math3/3.2/commons-math3-3.2.jar:/Users/andrzej/.m2/repository/org/openjdk/jmh/jmh-generator-annprocess/1.34/jmh-generator-annprocess-1.34.jar:/Users/andrzej/.m2/repository/junit/junit/4.13.2/junit-4.13.2.jar:/Users/andrzej/.m2/repository/org/hamcrest/hamcrest-core/1.3/hamcrest-core-1.3.jar:/Users/andrzej/.m2/repository/org/apache/felix/org.osgi.core/1.4.0/org.osgi.core-1.4.0.jar:/Users/andrzej/.m2/repository/javax/activation/activation/1.1.1/activation-1.1.1.jar com.intellij.rt.junit.JUnitStarter -ideVersion5 -junit4 tigase.xml.SimpleParserBenchmarkTest,launchBenchmark OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=permit; support was removed in 17.0 # JMH version: 1.34 # VM version: JDK 17.0.1, OpenJDK 64-Bit Server VM, 17.0.1+12-jvmci-21.3-b05 # VM invoker: /Users/andrzej/Development/bin/graalvm-ce-java17-21.3.0/Contents/Home/bin/java # VM options: -XX:ThreadPriorityPolicy=1 -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCIProduct -XX:-UnlockExperimentalVMOptions -ea -Dproject.target=/Users/andrzej/Development/Tigase/tigase-xmltools/target/classes -Dproject.version=4.2.0-SNAPSHOT --illegal-access=permit -Didea.test.cyclic.buffer.size=1048576 -javaagent:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/lib/idea_rt.jar=53342:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/bin -Dfile.encoding=UTF-8 # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable) # Warmup: 2 iterations, 1 s each # Measurement: 2 iterations, 1 s each # Timeout: 10 min per iteration # Threads: 2 threads, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: tigase.xml.SimpleParserBenchmarkTest.benchmarkParsing # Run progress: 0,00% complete, ETA 00:00:04 # Fork: 1 of 1 OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=permit; support was removed in 17.0 OpenJDK 64-Bit Server VM warning: -XX:ThreadPriorityPolicy=1 may require system level permission, e.g., being the root user. If the necessary permission is not possessed, changes to priority will be silently ignored. # Warmup Iteration 1: 28,387 us/op # Warmup Iteration 2: 20,497 us/op Iteration 1: 19,084 us/op Iteration 2: 17,714 us/op Result "tigase.xml.SimpleParserBenchmarkTest.benchmarkParsing": 18,399 us/op # Run complete. Total time: 00:00:09 REMEMBER: The numbers below are just data. To gain reusable insights, you need to follow up on why the numbers are the way they are. Use profilers (see -prof, -lprof), design factorial experiments, perform baseline and negative tests that provide experimental control, make sure the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from the domain experts. Do not assume the numbers tell you what you want them to tell. NOTE: Current JVM experimentally supports Compiler Blackholes, and they are in use. Please exercise extra caution when trusting the results, look into the generated code to check the benchmark still works, and factor in a small probability of new VM bugs. Additionally, while comparisons between different JVMs are already problematic, the performance difference caused by different Blackhole modes can be very significant. Please make sure you use the consistent Blackhole mode for comparisons. Benchmark Mode Cnt Score Error Units SimpleParserBenchmarkTest.benchmarkParsing avgt 2 18,399 us/op Process finished with exit code 0
	Andrzej Wójcik (Tigase) commented 3 years ago Result after applying changes: /Users/andrzej/Development/bin/graalvm-ce-java17-21.3.0/Contents/Home/bin/java -ea -Dproject.target=/Users/andrzej/Development/Tigase/tigase-xmltools/target/classes -Dproject.version=4.2.0-SNAPSHOT --illegal-access=permit -Didea.test.cyclic.buffer.size=1048576 -javaagent:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/lib/idea_rt.jar=55263:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/bin -Dfile.encoding=UTF-8 -classpath /Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/lib/idea_rt.jar:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/plugins/junit/lib/junit5-rt.jar:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/plugins/junit/lib/junit-rt.jar:/Users/andrzej/Development/Tigase/tigase-xmltools/target/test-classes:/Users/andrzej/Development/Tigase/tigase-xmltools/target/classes:/Users/andrzej/.m2/repository/org/openjdk/jmh/jmh-core/1.34/jmh-core-1.34.jar:/Users/andrzej/.m2/repository/net/sf/jopt-simple/jopt-simple/5.0.4/jopt-simple-5.0.4.jar:/Users/andrzej/.m2/repository/org/apache/commons/commons-math3/3.2/commons-math3-3.2.jar:/Users/andrzej/.m2/repository/org/openjdk/jmh/jmh-generator-annprocess/1.34/jmh-generator-annprocess-1.34.jar:/Users/andrzej/.m2/repository/junit/junit/4.13.2/junit-4.13.2.jar:/Users/andrzej/.m2/repository/org/hamcrest/hamcrest-core/1.3/hamcrest-core-1.3.jar:/Users/andrzej/.m2/repository/org/apache/felix/org.osgi.core/1.4.0/org.osgi.core-1.4.0.jar:/Users/andrzej/.m2/repository/javax/activation/activation/1.1.1/activation-1.1.1.jar com.intellij.rt.junit.JUnitStarter -ideVersion5 -junit4 tigase.xml.SimpleParserBenchmarkTest,launchBenchmark OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=permit; support was removed in 17.0 # JMH version: 1.34 # VM version: JDK 17.0.1, OpenJDK 64-Bit Server VM, 17.0.1+12-jvmci-21.3-b05 # VM invoker: /Users/andrzej/Development/bin/graalvm-ce-java17-21.3.0/Contents/Home/bin/java # VM options: -XX:ThreadPriorityPolicy=1 -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCIProduct -XX:-UnlockExperimentalVMOptions -ea -Dproject.target=/Users/andrzej/Development/Tigase/tigase-xmltools/target/classes -Dproject.version=4.2.0-SNAPSHOT --illegal-access=permit -Didea.test.cyclic.buffer.size=1048576 -javaagent:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/lib/idea_rt.jar=55263:/Users/andrzej/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/213.6777.52/IntelliJ IDEA.app/Contents/bin -Dfile.encoding=UTF-8 # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable) # Warmup: 2 iterations, 1 s each # Measurement: 2 iterations, 1 s each # Timeout: 10 min per iteration # Threads: 2 threads, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: tigase.xml.SimpleParserBenchmarkTest.benchmarkParsing # Run progress: 0,00% complete, ETA 00:00:04 # Fork: 1 of 1 OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=permit; support was removed in 17.0 OpenJDK 64-Bit Server VM warning: -XX:ThreadPriorityPolicy=1 may require system level permission, e.g., being the root user. If the necessary permission is not possessed, changes to priority will be silently ignored. # Warmup Iteration 1: 26,922 us/op # Warmup Iteration 2: 19,497 us/op Iteration 1: 19,051 us/op Iteration 2: 17,793 us/op Result "tigase.xml.SimpleParserBenchmarkTest.benchmarkParsing": 18,422 us/op
	wojciech.kapcia@tigase.net commented 3 years ago @kobit after discussing the issue with Andrzej and indeed there was and issue and we arrived at the conclusion that the best solution will be to modify our parser and, in case of encountering quote within attribute value escape it (directly within parser to avoid available unescape code due to performance penalty - and as seen below, the change affects performance in minimal way). Now to the handling of the topic - currently it's based on master branch, should be backport it to older branches and if so, how "deep"? (recently released 8.2 stable comes to mind, should we include it in 8.1? others)? How about announcement? Do we want to announce it (and if so - where?) or just include it in the release notes (I'd say it would be prudent to at least inform our clients, though they mostly use non-public deployments)?
	Artur Hefczyc commented 3 years ago Ok, before deciding I would like to make sure I understand the impact. Please confirm or clarify below: As I understand, to exploit the bug, a user has to be logged in, right? It is not possible to successfully attack without logging in? The fix is applied to our parser, which is xmltools only. So, in order to apply the fix it is enough to replace this library only? If this is the case, we could just prepare xmltools library for older versions of Tigase right? If you have any comments and/or suggestions, please let me know. Looking at our stats, the most used versions are: ver7.1.x - 1,300 servers ver8.1.2 - 1,084 servers ver5.1.0 (and all earlier) - 774 servers ver0.0.0 (custom builds) - 510 servers ver8.x-snapshots - approx 300 servers Therefore, it seems to me, if we can, we should prepare bug-fix builds for ver5.1.x, ver7.1.x, ver8.1.x, ver8.2.x. At the same time we should remove access to 8.2.0 build once the bugfix is released. For older versions, the bugfix version can be just as simple as repackaging the oldest minor version and replacing xmltools library. Just make sure it is compiled with a correct java version. We also need a list of affected customers and we need to prepare instructions on how to apply the fix for each version. Once we have the bugfix ready and instructions, I will contact our customers to let them know about the problem and how to apply the fix. Artur
	Andrzej Wójcik (Tigase) commented 3 years ago A few comments: As I understand, to exploit the bug, a user has to be logged in, right? It is not possible to successfully attack without logging in? It could be possible, if data would be sent from malicious XMPP server over S2S. Then it could theoretically be possible to "embed" a stanza within element attribute value. However, to do so, you would need to use `<` char within element attribute value, which should be rejected by out parser. That should be enough to stop someone from embedding a full stanza, but he could inject some attributes which would be invisible for processing by Tigase XMPP Server, but would appear on the client side (when stanza would be delivered). Due to that, I would say that impact on the users would be medium or low. The fix is applied to our parser, which is xmltools only. So, in order to apply the fix it is enough to replace this library only? If this is the case, we could just prepare xmltools library for older versions of Tigase right? For Tigase XMPP Server, that would be enough. For XMPP clients based on tigase-xmltools, we would need to make a few changes - XMPP client may need to process received attribute value (unescape it).
	wojciech.kapcia@tigase.net commented 3 years ago As Andrzej pointed out - it could be possible to deliver such stanza over s2s, but in the context of our clients s2s is disabled most of the time thus limiting impact to them. However, regarding injecting the stanza - our parser would parse such payload and create the Element object (with 5 children, so correct) and would simply forward such stanza to the destination. The issue would be our Element.toString() method converting single quotes to double quotes rendering the stanza/xml incorrect/susceptible to the attack (but this depends on the library used by the client, internally Tigase handles this correctly). On the other hand there could be a case where such stanza would have to be forwarded to the other cluster node (marshalled to string via same method) but most likely it would be rejected on the other side as incorrect cluster packet. Yes, updating tigase-xmltools would be enough but only for version 8.x. Previous versions had tigase-utils and tigase-xmltools inlined into tigase-server.jar (AFAIR OSGi requirement to have all the classes from the same package in the same binary, later on Java introduced same mechanism and in the meantime we cleaned up our packages structure). And there hasn't been all that much changes in it since 2017 (mostly "maintenance cleanup" or adjusting maven configuration). The question is, looking at the statistics, whether we want to provide builds of anything prior to tigase-7.0.x (which was released in 2015)? I would say that we would be safe to provide builds for 7.1.x and up. For the 8.2.0 we can simply remove attachments/binaries (there is no option to "unpublish" release on github). Therefore we should: prepare fixed xmltools for 4.1.1 (last stable), 4.0.1, 3.5.1 prepare releases/packages of tigase-server: 8.2.1 (and remove binaries of 8.2.0), 8.1.3. Prepare tigase-server.jar binaries for 7.1.x and 7.0.x? Regarding customer list, I assume this includes both those with ACS licence, support licence and also past clients (Alcatel, Epic for example?)
	wojciech.kapcia@tigase.net commented 3 years ago (I manually changed visibility of the comments in this issue to "CoreTeam" in case we would make it public to avoid making the discussion public as well.)
	Andrzej Wójcik (Tigase) commented 3 years ago For the 8.2.0 we can simply remove attachments/binaries (there is no option to "unpublish" release on github). I would suggest to actually release 8.2.1 as while you can replace binaries, there will be issues, as users will not know that they are using patched version (or not). Also, docker/k8s caches images, so it will not fetch a new images with the same tag if it already has one.
	wojciech.kapcia@tigase.net commented 3 years ago I would suggest to actually release 8.2.1 as while you can replace binaries, there will be issues, as users will not know that they are using patched version (or not). Also, docker/k8s caches images, so it will not fetch a new images with the same tag if it already has one. I wasn't suggesting replacing them inline, just removing it from 8.2.0 (while making 8.2.1) to avoid distributing (recently released) version with vulnerability. Same goes for docker images as per Artur's comment: "At the same time we should remove access to 8.2.0 build once the bugfix is released." - we would leave release page but remove binaries, hence "remove access".
	Artur Hefczyc commented 3 years ago OK, understand. Thank you for explanation and clarification. So, maybe we should prepare following public releases at minimum: Update the latest 8.x, that is prepare 8.2.1 Update the latest 7.x, that is 7.1.7 That would cover most of known existing installations. I do not know what exact version of Tigase is used by our customers. They might have some custom builds. So, we should probably contact all of them and let them know about the issue and offer our help with updating their version. But that would be a custom build/work on base-by-case basis.
	Artur Hefczyc commented 3 years ago I have prepared the following notice to our customers. Please take a look and let me know if all is correct and if you have suggestions to add/remove something: Hi, This is Tigase Team sending you a notification about recently discovered vulnerability in one of the libraries used in Tigase software, mainly Tigase XMPP Server. For Tigase XMPP Server deployments allowing for public user account registrations and/or allowing for XMPP federation with other XMPP servers impact is medium to high. For all other deployments with disabled XMPP federation and with no public account registration impact is low to medium. The vulnerability allows to inject malicious XMPP packet over established and authorized XMPP connection, either user connection (c2s) or federated connection (s2s). The vulnerablity cannot be exploited over unauthorized XMPP connection, that is by an intruder who has no login credentials. All versions of the Tigase XMPP Server prior to 8.2.1 and 7.1.7 are impacted. A fix is already created and available but patching existing installations depends on the version used. If do not run the latest 8.2.1 or later or 7.1.7 or later and you cannot upgrade to the latest version, please contact us for help with patching your system. with best regards, Tigase Team.
	wojciech.kapcia@tigase.net commented 3 years ago The mail looks correct. In addition to preparing 8.2.1 and 7.1.7 release we can also prepare dedicated build of xml-tools (for drop-in replacement if some customers would prefer that). I'm pondering how we should handle preparation and distribution of the packages. Should we commit the fix with an inconspicuous description and prepare all the packages without making the fuss, notify the customers and let everyone update the installations?
	Artur Hefczyc commented 3 years ago Yes, I think this is the right way to do it. And then, once we have contacted all our customers, and they updated their systems we can release a public announcement.
	Artur Hefczyc commented 3 years ago Please let me know when the big fix versions are available and then I will send a message to our customers.
	wojciech.kapcia@tigase.net commented 3 years ago Main releases: Tigase XMPP Server 7.1.7: https://github.com/tigase/tigase-server/releases/edit/untagged-28eea89b388877bf6447 Tigase XMPP Server 8.2.1: https://github.com/tigase/tigase-server/releases/edit/untagged-a99080f4550bca7b45e9 Individual releases Tigase XMLtools 3.5.1 Release: https://github.com/tigase/tigase-xmltools/releases/edit/untagged-46d823a6135095898a39 (for version 7.1.x) Tigase XMLtools 4.0.1 Release: https://github.com/tigase/tigase-xmltools/releases/edit/untagged-20f6364f2cc3e3f837ae (for version 8.0.x) Tigase XMLtools 4.1.1 Release: https://github.com/tigase/tigase-xmltools/releases/edit/untagged-07d14a87d952785dc9f8 (for version 8.1.x and 8.2.x) @kobit those are draft releases for now, I wasn't sure what to put in the "Changes" section (or whether to include it at all (we could amend them after the public anouncement later on)? Either I can publish them or you can do it just before sending the notification to the customers. I'll prepare docker images once those are publish due to dependencies.
	Artur Hefczyc commented 3 years ago Please put something general like "security bugfix in xmltools". This provides some basic information and suggestion for the public to update due to security issue without any specifics on the possible exploit.
	wojciech.kapcia@tigase.net commented 3 years ago Releases made, tigase-xmltools: https://github.com/tigase/tigase-xmltools/releases/tag/tigase-issue #3.5.1 https://github.com/tigase/tigase-xmltools/releases/tag/tigase-issue #4.0.1 https://github.com/tigase/tigase-xmltools/releases/tag/tigase-issue #4.1.1 tigase-server: https://github.com/tigase/tigase-server/releases/tag/tigase-server-7.1.7 https://github.com/tigase/tigase-server/releases/tag/tigase-server-8.2.1
	wojciech.kapcia@tigase.net commented 3 years ago 7.1.7 rebuild 8.2.1 docker images published: https://hub.docker.com/r/tigase/tigase-xmpp-server/tags?page=1&name=8.2.1
	Artur Hefczyc commented 3 years ago Thank you. Notifications were sent out to all companies I could find that use(d) Tigase. Artur

Type	Bug
Priority	Normal
Assignee	Artur Hefczyc
Version	tigase-server-8.3.0

helpdesk/helpdesk-priv#825 You are not authorized to access this issue
helpdesk/helpdesk-priv#809 You are not authorized to access this issue
Customers/catapush-s-r-l#29 You are not authorized to access this issue

Issue Votes (0)

Watchers (0)

Reference

tigase/_server/tigase-xmltools#15