search

Together-Java / TJ-Bot

TJ-Bot is a Discord Bot used on the Together Java server. It is maintained by the community, anyone can contribute.
https://togetherjava.org
GNU General Public License v3.0
100 stars 89 forks source link

NPE Blacklisted attachments without extensions #673

Closed Zabuzard closed 1 year ago

Zabuzard commented 2 years ago

Exception

We just encounted the following exception:

ERROR net.dv8tion.jda.api.JDA - One of the EventListeners had an uncaught exception
java.lang.NullPointerException: Cannot invoke "String.toLowerCase(java.util.Locale)" because the return value of "net.dv8tion.jda.api.entities.Message$Attachment.getFileExtension()" is null
      at org.togetherjava.tjbot.commands.moderation.attachment.BlacklistedAttachmentListener.lambda$doesMessageContainBlacklistedContent$4(BlacklistedAttachmentListener.java:104) ~[classes/

      at java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90) ~[?:?]
      at java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1602) ~[?:?]
      at java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129) ~[?:?]
      at java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527) ~[?:?]
      at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513) ~[?:?]
      at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) ~[?:?]
      at java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230) ~[?:?]
      at java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196) ~[?:?]
      at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
      at java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:632) ~[?:?]
      at org.togetherjava.tjbot.commands.moderation.attachment.BlacklistedAttachmentListener.doesMessageContainBlacklistedContent(BlacklistedAttachmentListener.java:103) ~[classes/:?]
      at org.togetherjava.tjbot.commands.moderation.attachment.BlacklistedAttachmentListener.onMessageReceived(BlacklistedAttachmentListener.java:44) ~[classes/:?]
      at org.togetherjava.tjbot.commands.system.BotCore.lambda$onMessageReceived$9(BotCore.java:202) ~[classes/:?]
      at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
      at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) ~[?:?]
      at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179) ~[?:?]
      at java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1850) ~[?:?]
      at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) ~[?:?]
      at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) ~[?:?]
      at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
      at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
      at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
      at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) ~[?:?]
      at org.togetherjava.tjbot.commands.system.BotCore.onMessageReceived(BotCore.java:202) ~[classes/:?]
      at net.dv8tion.jda.api.hooks.ListenerAdapter.onEvent(ListenerAdapter.java:403) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.api.hooks.InterfacedEventManager.handle(InterfacedEventManager.java:96) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.hooks.EventManagerProxy.handleInternally(EventManagerProxy.java:88) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.hooks.EventManagerProxy.handle(EventManagerProxy.java:70) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.JDAImpl.handleEvent(JDAImpl.java:173) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.handle.MessageCreateHandler.handleInternally(MessageCreateHandler.java:136) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.handle.SocketHandler.handle(SocketHandler.java:39) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.requests.WebSocketClient.onDispatch(WebSocketClient.java:961) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.requests.WebSocketClient.onEvent(WebSocketClient.java:847) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.requests.WebSocketClient.handleEvent(WebSocketClient.java:825) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at net.dv8tion.jda.internal.requests.WebSocketClient.onBinaryMessage(WebSocketClient.java:1000) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
      at com.neovisionaries.ws.client.ListenerManager.callOnBinaryMessage(ListenerManager.java:385) ~[nv-websocket-client-2.14.jar:?]
      at com.neovisionaries.ws.client.ReadingThread.callOnBinaryMessage(ReadingThread.java:276) ~[nv-websocket-client-2.14.jar:?]
      at com.neovisionaries.ws.client.ReadingThread.handleBinaryFrame(ReadingThread.java:996) ~[nv-websocket-client-2.14.jar:?]
      at com.neovisionaries.ws.client.ReadingThread.handleFrame(ReadingThread.java:755) ~[nv-websocket-client-2.14.jar:?]
      at com.neovisionaries.ws.client.ReadingThread.main(ReadingThread.java:108) ~[nv-websocket-client-2.14.jar:?]
      at com.neovisionaries.ws.client.ReadingThread.runMain(ReadingThread.java:64) ~[nv-websocket-client-2.14.jar:?]
      at com.neovisionaries.ws.client.WebSocketThread.run(WebSocketThread.java:45) ~[nv-websocket-client-2.14.jar:?]

Source

The issue comes from BlacklistedAttachmentListener.java#L107:

code

Apparently its possible that attachments have no extension, for which our code then fails.

Desired behavior

Files without attachments should be blocked as well.

Nxllpointer commented 2 years ago

Why should files without extension be blocked? Most files without extension are just text anyways. Windows does not run files without extension afaik and linux is file extension independant anyways

Zabuzard commented 2 years ago

Why should files without extension be blocked? Most files without extension are just text anyways. Windows does not run files without extension afaik and linux is file extension independant anyways

I do not care too much. I can see arguments for both sides. For example that such files could be a harmful exe and better safe than sorry.

But I also get that the main argument for this feature is to prevent harmful auto-executables.

@marko-radosavljevic whats your take on this?

Mom0aut commented 1 year ago

If not taken i could easy fix the issue? 😄 duplicate of #618 ?

Zabuzard commented 1 year ago

@Mom0aut yes, please. thank you :)

Mom0aut commented 1 year ago

I would suggest to prevent the uploading of files without extension.

Nxllpointer commented 1 year ago

Do you have a reason for that?

marko-radosavljevic commented 1 year ago

@marko-radosavljevic whats your take on this?

Sorry for the late reply @Zabuzard.

In my opinion, files without extensions should be blocked.

Do you have a reason for that?

So let's examine few aspects: actual threat, perceived threat by the community, moderation overhead and liability & responsibility of the community owners. They would be our risk, and compare it to benefits of allowing files without an extension.

Benefits

Our community does not want, or need, to share files without extensions. It's a very rare occurrence on the server.

Furthermore, files without extensions can't even be opened easily on most systems, so their usability is severely limited.

Risks

Threats

File without an extension can be malware or disturbing/illegal/pornographic media.

Since benefits are non-existent, I don't even have to prove any risks. Because of that, I will skip going over malware attack vectors on different systems, which is a big relief. :smile: It's a massively complex subject, and I don't have enough knowledge to speak with any authority.

We will assume that, generally, windows does not open files without extensions easily in gui. And linux opens files without extensions without any issues, but to run executables you have to explicitly give execute permissions.

Since we are ignoring malware, the threat would be downloading illegal material, or being exposed to NSFW content without your knowledge.

Community

We have a highly technical community, that has a big aversion towards downloading anything from random people on the internet. Rightfully so.

I personally witnessed this many times, helpers that won't download anything, or people questioning files in lobby.

Posting files, especially executables, files with no extension, etc., is perceived as extremely dodgy behaviour. It creates an unsafe atmosphere, and elicits a negative reaction from the community.

This affects everyone. Less experienced members feel unsafe, and security veterans take a principled position even if they how to open the file safely, and they are not in any real danger.

Moderation overhead

Moderators have to make sure members are safe.

To guarantee that, they have to download random files, upload to virustotal or something, and then view the content themselves. They also have to do that safely, which requires knowledge, time and effort.

For example, I mark files I checked with a standard moderator checkmark.

Responsibilities of community owners

Community owners need to make sure they respect the platform they are on. Which in our case means following and enforcing discord tos and community guidelines.

Which, of course, extends also to user uploaded illegal or malicious material. In the end, community owners are responsible for allowing and hosting those files. And the whole community can suffer consequences as the result of that.

Conclusion

Risks outweigh the benefits by a huge margin.

We should aim to remove all files. Word assignment files can have dangerous macros, project archives can be zip bombs, screenshare videos can be NSFW or copyrighted movie that has been illegally downloaded. Project jars can be both malware and illegal material, for eg. keylogger that seeds torrents in the background :p. And finally, files without an extension can be anything.

Instead of blacklist, it's probably much easier and smarter to go with a whitelist. Only things with significant benefits should be allowed, since risks are high by default. And optionally, we can remove that restriction for trusted members (top helpers+ or something similar), if the need arises.

For comparison, we want and need links, benefits are so huge that it's not even worth discussing it. For media embedded in discord, it's something we want, it's safe, and it's easy to moderate. Files are none of that.

marko-radosavljevic commented 1 year ago

Do we agree on converting to whitelist as a solution to this issue?

It's safer that way. I also fear we will continuously discover new problematic extensions, and it's a hassle to add them to the vps.

If so, which extensions will be whitelisted: .txt, .md, .java. Maybe other languages, but we should encourage gists/pastebin anyway?

And of course, extensions discord embeds:

I'm also fine with just merging the current solution. Especially because I'm late to the party, and PR is already done.

Mom0aut commented 1 year ago

I think it would be better to have some extra ticket for those changes from blacklist to whitelist 😄

marko-radosavljevic commented 1 year ago

Yeah, that's completely fair. ^^

It's outside the scope of this issue, that already has a working PR anyway. My only worry is that we probably won't bother to revisit this again, since it's not that big of a deal.

---- On Sun, 27 Nov 2022 00:29:34 +0100 Mom0aut @.***> wrote ---

I think it would be better to have some extra ticket for those changes from blacklist to whitelist 😄

— Reply to this email directly, https://github.com/Together-Java/TJ-Bot/issues/673#issuecomment-1328131141, or https://github.com/notifications/unsubscribe-auth/AHL6YWLQIIXSCL5YR5WNNLTWKKMN5ANCNFSM6AAAAAARYBYK5M. You are receiving this because you were mentioned.