MinecraftForge / FML

(Archive Only: Merged into Forge proper) The Forge Mod Loader - an opensource replacement mod loader for minecraft
Other
434 stars 201 forks source link

Missing option to disable signature checking, #184

Closed nvx closed 11 years ago

nvx commented 11 years ago

There is no option to disable signature checking on mod loading. There are some cases where a monkeypatch is required to fix serious crash bugs to prevent denial of service attacks (or even accidental triggering!).

Some mods implement the signature check failed callback as an excuse to hard crash the game, which is not acceptable behaviour.

Personally I would prefer to see signature validation removed entirely, however an option to disable triggering of the callbacks would be an acceptable compromise.

LunNova commented 11 years ago

edit: disregard this, commented on the wrong issue.

cadyyan commented 11 years ago

Isn't this something that's built into the mod not Forge? Forge would have to reverse engineer the mods code to find where this is happening and then try to stop it. This seems like its a modders decision.

LunNova commented 11 years ago

https://github.com/MinecraftForge/FML/blob/master/common/cpw/mods/fml/common/event/FMLFingerprintViolationEvent.java

Of course, mods are free to do their own signature validation, so allowing this event to be easily disabled would probably just lead to modders choosing not to use this, unless a SecurityManager was used to ensure that this was the only possible way of validating signatures.

cpw commented 11 years ago

The violation code is there so a mod knows something is wrong. Disabling it defeats its purpose for existence. A monkey patch should not be required by end users.

nvx commented 11 years ago

In an ideal world, you're right.

We don't live in an ideal world.

One example: http://pastie.org/6202476

cpw commented 11 years ago

Rubbish and you know it. I seem too be able to run all the same mods as you all, on servers no less, and see none of these purported issues. Basically if you need to monkey patch you are failing to understand the problems and their solutions.

nvx commented 11 years ago

Did you read the simple step-by-step instructions on how to crash your server? I encourage you to try it if you don't believe me.

Or are you saying you tried them, and it didn't work?

If you tried it, and it worked, then what crack are you smoking? Clearly there's an issue, and clearly I've managed to stumble across it because hey, I found the bug didn't I? And clearly waiting 6 months for an official fix from eloraam isn't an acceptable solution when a monkey patch would fix it in minutes.

If you'd like I can demonstrate on your own server if you can't reproduce it yourself.

Please do enlighten me as to what's "rubbish" about this?

ArcanoxDragon commented 11 years ago

@nvx you're acting very immature and unprofessional here. I can tell you the most simple fix for this problem: don't allow grates to be crafted. I'm 99% sure the only problem is ejecting non-world-placeable liquids from grates. If the problem still exists, block Pumps too. This "issue" is the least logical solution and it's turning into an insult war.

nvx commented 11 years ago

Yeah, that's not actually the issue (I actually used water during my own testing, however that's a good idea of something else to test too...). A grate was just used as a handy sink. Another tank would also suffice, and a sink for the liquids may not even be needed to trigger the bug.

Should have bet on that 1%.

Blocking pumps would of course "fix" the issue, but as has been already thoroughly explored simply removing functionality is in fact NOT a fix for an issue. I mean heck, why not just remove RedPower2 entirely, that'd fix it. Or you know, Forge. Lets just play Vanilla! Oh hang on, I think there's a bug there too, lets just play Solitare instead.

Forge integrates many fixes for bugs from Vanilla minecraft. It seems rather hypocritical to say "oh, just put up with it" or "don't use/remove that feature" while the other hand fixes issues in Mojangs code. What makes mods code holier than Mojangs?

LunNova commented 11 years ago

I have also ran into and patched an RP2 dupe bug. Removing buggy functionality from a mod which is rarely updated is not the solution. I have considered attempting to make an open-source clone of RP2's most important functionality, but do not have the time.

I will be moving to replacing classes in the classloader, as it seems that for some reason this counts as acceptable (see RPTweaks), while replacing it in the jar/zip does not.

Cloudhunter commented 11 years ago

Signature checking being removed wouldn't solve anything, honestly. If people want to crash your client under certain conditions, they can do it regardless of signatures. Signature checking is a tool, it is up to the people implementing the checking to use it responsibly. If they don't, take it up with them - it shouldn't be FML's job to babysit what other people do.

I for one just think for the mods causing the issue, the server side shouldn't be checked, since there could possibly be legit reasons.

nvx commented 11 years ago

Which, in light of the fact that it's trivial to bypass said "security" anyway... Why such "security" exists in the first place.

Clearly anyone determined enough to do nefarious things can do it anyway, so why penalise the server admin that needs to monkeypatch something for whatever reason? (Heck, another valid monkeypatchable thing could be to remove RP2 volcanos... or fix them... I personally like them, even though the glitchy generation irks me a bit, but I know many others do not).

And of course before you suggest disabling the wgen forge-side, consider it would also disable the RP2 ores from spawning, etc.

Please reiterate why you're deliberately making it as hard as possible to patch mods?

ArcanoxDragon commented 11 years ago

@nvx I state again: you're being very unprofessional here by making statements like "heck vanilla is buggy, i'll just play solitaire." I was providing a solution that would be 100% sure to fix the problem until a fix was released. You can interpret that however you want but don't come here posting sarcastic snap-backs at me. Like @Cloudhunter said, this isn't FML's issue.

nvx commented 11 years ago

I'd argue that it became FMLs issue the moment it added the ability to enforce signature checking by mods (see: FMLFingerprintViolationEvent).

This request is literally asking for a config option to disable firing of FMLFingerprintViolationEvent to mods. No more, no less.

How is that not FMLs issue?

cadyyan commented 11 years ago

The code was added for a reason. Why have it there if you're going to have an option to ignore/disable it. In the end that wouldn't solve the issue because a mod can still do the same thing.

nvx commented 11 years ago

Why have it if it can be bypassed by other means anyway? You're not making a strong argument for its continued existence...

I have presented a strong reason as to why there should be an option to disable firing FMLFingerprintViolationEvents. I have not yet seen a single reason in this thread as to why this was even added in the first place.

What are you trying to protect against with this "security"? Please cite an actual scenario where this "feature" enhances the users experience, makes life of a server admin better, or protects a user from some ghastly malicious threat - remembering that this "feature" can be bypassed via reflection and other means anyway if some malicious mod author wanted to do so anyway.

Bonus points if your scenario has actually occurred. My scenario has occurred, in fact, it exists right now. Please enlighten me.

cpw commented 11 years ago

No, you want to create a backdoor so you can run arbitrary hacked versions of mods. You haven't made any request to fix the bug with the mod, you simply believe you can justify your hackery under the banner of 'legitimate fix'. Sadly, if you persist in trolling FML for providing a modicum of security, I will have to take action to ban you, which is a shame, because you have contributed useful stuff in the past. This feature is not going away, monkey patching is not a supported function. Remove the mod if you don't like its consequences.. I will point out that this hackery is about to get incrementally harder. Multi language support is implemented locally. I hope modders use these facilities to double down on your hateful actions.

AKA-Syenite commented 11 years ago

@cpw http://pastie.org/6202476

cpw commented 11 years ago

@shukaro well aware of it. It can be fixed without violating signature checking. I'm well aware of the agenda here. I am not stupid. This feature is not going away. And its not going to be toggleable. Eloraam has not even been told of this issue, that I am aware. I will investigate why it causes world corruption but that is the extent of it..

LunNova commented 11 years ago

@nvx

If this is made easy to disable modders will just validate signatures on their own, so it won't help you.

I have a maybe more reasonable suggestion regarding disabling it - why not allow it to be disabled, but only on servers? As far as I can see, in most cases modders aim to prevent distribution of their mods in mod packs without permission when they validate signatures, so this matters more on the client.

On the server, any patches are much more likely to be legitimate fixes, so allowing it to be disabled on servers shouldn't be a problem.

Silwing commented 11 years ago

What the... The least one can do when encountering a serious issue is contact the mod developer and report the issue :S why not give the mod developers a chance to correct their mistakes before you start screaming about it being everyone else's problem to fix the issue....?

wizjany commented 11 years ago

@Silwing yes, it was already reported, ages ago Unfortunately, as long as the mod developer does not supply a fix, server owners have to supply their own.

LunNova commented 11 years ago

https://github.com/nallar/TickThreading/commit/8311eeecf7ad34d998a314a46ecc595c86a398bc#commitcomment-2638141

:D

nvx commented 11 years ago

@cpw You wrote rather boldly that I have not attempted to get the issue fixed. I have in fact. Months ago. When I first found out about the issue.

I'm still waiting for you to explain what purpose this feature serves at all, and then how that purpose would be defeated by providing an option to allow users to disable it if they so desired. Please stop distracting from the issue.

Myrathi commented 11 years ago

I don't see how he has to explain anything more than he has, already. If a mod author wants to secure his/her code then that's his/her choice and you, as an end-user, have little to no say in the matter as it's not your code.

That leaves you with three choices: encourage the author to fix their code, stop using the mod or find legitimate ways to fix the issues.

It really is that simple. It may make you unhappy but your dislike doesn't make it wrong.

nvx commented 11 years ago

@Myrathi The issue which spawned this is where a certain mod author decided to choose to "secure" other peoples code, which requires change of said mod authors code to remove said retardation.

Other cases include mods such as RedPower2, which gets minimal updates, so waiting for an official update isn't an option.

There are now two situations that I have presented where having this feature included would be a good thing. I have asked for one situation where adding a user configurable option to disable this behaviour would cause negative effects, and what possible "security" this feature adds in the first place considering that it can be bypassed by other methods of modification anyway.

Please do not bother replying unless you're presenting such a case for the continued existence of this feature as-is, without an option to disable it.

In summary:

cpw commented 11 years ago

@nvx you have demonstrated a desire to hack at mods. This feature makes that harder. Removing it would make your job easier. Ergo its not going anywhere. Currently mod hacking is easy mode. I want to make it a lot harder, just to annoy you. There are plenty of legitimate uses otherwise you wouldn't be so vociferous in your complaints. Feel free to fork FML if it disgusts you so much. I suspect your fork's adoption rates will be very low..

Now, this topic is CLOSED. Please, everyone, this is not going to change so let it drop, OK?

yuuka-miya commented 11 years ago

Well, @nvx shut up and use ASM. It's actually really easy to write a class replacer. EDIT: cpw, sure.