Thaumcraft Fix

Thaumcraft Fix

23.3k Downloads

Unsecured Packet PacketSyncProgressToServer

JoshieGemFinder opened this issue ยท 9 comments

commented

One of the base Thaumcraft packets, "PacketSyncProgressToServer" is supposed to be a simple packet that notifies the server that a client is trying to progress through a research node. Unfortunately, if a client sends a packet with the checks parameter set to false, it is possible for them to progress through the research without actually doing any of the work, or requiring any of the theories/observations to complete it (as long as the parent research nodes are completed).
This is kind of a big hole in the code since you don't have to do anything to actually progress through the research tree (in fact, the end effect could be compared having a Cheater's Thaumonomicon for free in survival).

commented

Looks like it might be all good then. I did check the bug with a build of this mod and the issue seemed to still exist, but it's possible that I didn't test it with research nodes past FIRSTSTEPS due to not being able to open the Thaumonomicon.

commented

Also I should note that inside GuiResearchBrowser there's a legitimate PacketSyncProgressToServer created where checks is false; however, this only happens when first is true (and you're unlocking a research node for the first time).

commented

This should already be fixed here - I didn't draw attention to it because I didn't know if this was public yet. I made it ignore the checks field and always treat it as true.

If you're interested, please check my work there - I had to do some special handling depending on the research stage because Thaumcraft was not able to deal with starting new research while checks was true. If we're confident that Thaumcraft always requires parent research and that works properly, that logic can probably be simplified to only override checks if the stage being completed is >= 0.

commented

I have just tested and the fix mod does in fact fix it for ones that have requisites (however certain researches that don't have requisites - such as PRIMPEARL - remain accessible to a malicious packet)

commented

For that first option, you'd probably have to inject patches since the client would then see it as being started (i.e. you wouldn't see the blinking on it). You might have to create some extra packets and inject them into the system as well (to mimic the behaviours of that first unlock click, such as giving the player XP). You might also have to do some extra hacking to make sure checks to <research>@0 (and maybe ThaumcraftCapabilities.knowsResearch() in general) still function properly (though I'm not sure of anything that rely on them in such a way this would majorly break things).

I don't have a perfect silver-bullet solution, but I do have a few checks that should at least partially deal with it (regardless of which strategy you take):

  • Research beginning with ! should be blacklisted from client packets
  • Research without a category should be blacklisted
  • Unregistered research should be blacklisted (though the way you access the list of all research is through categories, so this doubles with the second point)

I'll likely edit the list if I think of anything else that should be applied no matter what.

commented

Yep, that comes from the exception put in for starting a new research entry. I think all of the scan researches will be like this, which I didn't think of before and is a problem. Looks like that exception is going to have to be removed, and the client should probably be unconditionally banned from starting new researches. I have some ideas on ways to make things work if I do that:

  1. Whenever a research is awarded, find out which researches have that as a parent. For each one, if all requisites are now satisfied, automatically grant the first stage key ending in @0. This is the research "before" the actual first page of the research. I don't know if doing this will make the client think the research was already started, though.
  2. Prevent the client from giving itself the research if the research does not have any parents. This would probably break any addons that add a starting node for their addon that doesn't have any requirements, but I'm not sure if any addons fall under that category currently.

Do you have any ideas? Personally, I'm leaning towards option 1, with patches to make the client not think the research was started if that is an issue.

commented

A third option could be to create a server-side container for the Guis that access the packet and have them be the ultimate deciding factor on whether to grant the research or not

commented

Actually you can probably disregard those last two comments.
For the second option, it's just occurred to me that if an addon did list the category base with no parents, then it simply wouldn't show up (like how WARP and FLUX don't appear until some external criteron is met), meaning that addon must manually assign the research to players somewhere, meaning that preventing research nodes with no parents from being unlocked early is totally valid.

commented

I agreed with your observation about requiring research to have parents to unlock it from the client, so that is in the latest version. Please let me know if you find any other holes in it.