Team-Fruit / Emojicord

Discord Emojis in Minecraft
MIT License
16 stars 10 forks source link

why? #25

Closed GoldenCookieGIT closed 2 years ago

GoldenCookieGIT commented 3 years ago

https://github.com/Team-Fruit/Emojicord/blob/master/shared/src/main/java/net/teamfruit/emojicord/Analytics.java#L54

AliceDTRH commented 2 years ago

Wait, are you sending the Minecraft session token as part of your analytics? If so, that's a serious privacy/account security issue!

AliceDTRH commented 2 years ago

I believe this is indeed the case because of the following:

  1. https://github.com/Team-Fruit/Emojicord/blob/8a627f8e58bd95bb997f2d4566c2735f43c84f63/shared/src/main/java/net/teamfruit/emojicord/Analytics.java?rgh-link-date=2021-10-23T21%3A45%3A16Z#L50
  2. https://github.com/Team-Fruit/Emojicord/blob/8a627f8e58bd95bb997f2d4566c2735f43c84f63/shared/src/main/java/net/teamfruit/emojicord/Analytics.java?rgh-link-date=2021-10-23T21%3A45%3A16Z#L54
  3. https://github.com/Team-Fruit/Emojicord/blob/8a627f8e58bd95bb997f2d4566c2735f43c84f63/shared/src/main/java/net/teamfruit/emojicord/Analytics.java?rgh-link-date=2021-10-23T21%3A45%3A16Z#L99
  4. https://github.com/Team-Fruit/Emojicord/blob/8a627f8e58bd95bb997f2d4566c2735f43c84f63/shared/src/main/java/net/teamfruit/emojicord/Analytics.java?rgh-link-date=2021-10-23T21%3A45%3A16Z#L100
  5. https://github.com/Team-Fruit/Emojicord/blob/8a627f8e58bd95bb997f2d4566c2735f43c84f63/shared/src/main/java/net/teamfruit/emojicord/Analytics.java?rgh-link-date=2021-10-23T21%3A45%3A16Z#L105
  6. https://github.com/Team-Fruit/Emojicord/blob/8a627f8e58bd95bb997f2d4566c2735f43c84f63/shared/src/main/java/net/teamfruit/emojicord/Analytics.java?rgh-link-date=2021-10-23T21%3A45%3A16Z#L107

If this code path is followed, it sends the contents from Minecraft.getSession().getToken(). In addition to this, it seems to also send the player ID, Username, another token from the mod itself, the Minecraft Version and the Forge version.

AliceDTRH commented 2 years ago

I believe this means the owner of the Analytics server may be able to hjiack player sessions.

sjcl commented 2 years ago

We apologize for the issues with the analysis function. I am not the author of these codes and features, but since the author seems to have abandoned this project and us, I have stopped sending analytics as the best I can do now. This should work for the current release.

However, I can assure everyone that no tokens or other analytical data have ever been collected. I participated in this project as the web department and manage the domain to which the analysis data has been sent so far. The analysis data was sent to a front-end page, and there is no analysis server there or anything that redirects to it. I did not have time to make an analysis server. The front-end pages are hosted on Netlify and are linked to GitHub, making them difficult to cheat. As for possible DNS record edits, please check the DNS history tool. I understand that it is not perfect, but I have no further way to prove it.

I am aware that even if the analytics stops sending, there is still the possibility of issues. I would like to support this project from now on, but it may take some time as I am not even sure how to build this project. Again, my apologies.

AliceDTRH commented 2 years ago

I haven't had the time to study your Netlify page in detail yet, but I wanted to respond to your message first. I want to preface this that I am not trying to attack you or place blame, I just want to make sure everyone understand the situation and can deal with it accordingly.

Regarding the claim that no tokens or other analytical data has been collected: It's possible to deploy / edit your site without changing to the git repository on Netlify (https://docs.netlify.com/cli/get-started/#deploy-directories) as well as there likely being request logs. The fact stands that a large amount of sensitive user data has been leaked to you without users permission and may be leaked at any time with a single change on git. We do not know if this information has been leaked further or stored in any way.

My advice for the future: I suggest removing the current mod files from CurseForge/Github until you've made sure all code sending personal information other than the information as disclosed on Curseforge have been removed and then re-uploading new versions without the offending code.

In it's current form this mod is malicious, even if you didn't intend for it to be and IMHO should be classified as malware even in it's "dormant" form.

Kamesuta commented 2 years ago

Sorry for the late reply.

This analysis feature was implemented to find out how much redistribution is occurring outside of CurseForge. Session tokens were used to ensure that the statistics could not be tampered with. And session tokens were used to verify that the account was a unique user, not an invalid user, using Mojang's session server. However, two years later I look back and realize that this was not the correct method. Using the same mechanism as joining multiplayer, I could have sent the token to Mojang's session server, received a code, and sent the code to the analysis server to verify without having to send the token directly to the analysis server.

Its also not a good idea to do the analysis unauthorizedly without notice and without the option to disable it. I am sorry for that.

I terminated my analysis server about 6 months after the initial release because analysis is no longer needed. Of course I did not save the session tokens.

I have removed the analysis code and made a release, but the CurseForge project has been temporarily closed and I do not know when I will be able to release it. Or the project may be permanently deleted.

AliceDTRH commented 2 years ago

Using the same mechanism as joining multiplayer, I could have sent the token to Mojang's session server, received a code, and sent the code to the analysis server to verify without having to send the token directly to the analysis server.

Sending the UUID of the player is probably good enough for verifying the account is a real Mojang account. Pretty sure there are API's for verifying if it's a premium account. While this can technically be spoofed, nobody is going to bother with that and if they do it's just one bad piece of data in your analytics, that's hardly a huge problem in comparison with accidentally mismanaging the data.

Its also not a good idea to do the analysis unauthorizedly without notice and without the option to disable it. I am sorry for that.

Your apology is greatly appreciated as is the removal of the offending code, I'm glad this mess is behind us! 😅

GoldenCookieGIT commented 2 years ago

Took a year but glad it got figured out, closing issue as it seems to have been resolved.