Integrated Dynamics

Integrated Dynamics

63M Downloads

Server crash exploit

TomyLobo opened this issue ยท 7 comments

commented

Issue type:

  • ๐Ÿ› Bug

Short description:

Disclosed in a mail to @rubensworks.
I will disclose details here once a fix is released.

Steps to reproduce the problem:

  1. Build this:
    image
  2. Throw your equivalent of card #333 into a display panel
  3. Throw everthing else into a variable store
    • Expected: Anything but a crash
    • Actual: crash
  4. Restart server
    • Expected: Anything but a crash
    • Actual: crash

Expected behaviour:


Versions:

  • This mod: IntegratedDynamics-1.12.2-1.1.0.jar
  • Minecraft: 1.12.2
  • Forge: 14.23.5.2847

Log file:

crash-2019-10-26_17.14.23-server.txt

commented

@TomyLobo Could you include all information in this issue?

commented

Done

commented

The exploit can probably be simplified by replacing #332 by List literal containing a single Integer directly.

Looks like you're doing an unchecked cast from a IValueTypeListProxy<?, ?> to a IValueTypeListProxy<ValueTypeString, ValueTypeString.ValueString>.
IntelliJ even warns about this :)
My fix streams that IValueTypeListProxy<?, ?> directly and filters out everything that isn't a ValueTypeString.ValueString before casting.

What would be really neat is if you could make some kind of toString-like method in IValue and use it for this.
This could be a no-op on strings and could return some more or less useful value for everything else.

Btw, while I was at it, I took the liberty of converting this to use Collectors.joining, instead of collecting a list and then joining that.
Slightly more efficient (at least in terms of garbage created) and also more elegant :)

commented

@TomyLobo Could you include the crashlog as well?

commented

If you're into micro-optimization, this should be a tiny bit faster :)

diff --git a/src/main/java/org/cyclops/integrateddynamics/core/evaluate/operator/Operators.java b/src/main/java/org/cyclops/integrateddynamics/core/evaluate/operator/Operators.java
index 06e65eae1..69451b998 100644
--- a/src/main/java/org/cyclops/integrateddynamics/core/evaluate/operator/Operators.java
+++ b/src/main/java/org/cyclops/integrateddynamics/core/evaluate/operator/Operators.java
@@ -729,17 +729,21 @@ public final class Operators {

                 // Join in O(n), while type-checking each element, as the list may have been of ANY type.
                 StringBuilder sb = new StringBuilder();
-                for (IValue value : elements.getRawValue()) {
+                Iterator<? extends IValue> iterator = elements.getRawValue().iterator();
+                while (true) {
+                    IValue value = iterator.next();
                     if (value.getType() != ValueTypes.STRING) {
                         L10NHelpers.UnlocalizedString error = new L10NHelpers.UnlocalizedString(
                                 L10NValues.VALUETYPE_ERROR_INVALIDLISTVALUETYPE,
                                 value.getType(), ValueTypes.STRING);
                         throw new EvaluationException(error.localize());
                     }
-                    if (sb.length() > 0) {
-                        sb.append(delimiter.getRawValue());
-                    }
                     sb.append(((ValueTypeString.ValueString) value).getRawValue());
+
+                    if (!iterator.hasNext()) {
+                        break;
+                    }
+                    sb.append(delimiter.getRawValue());
                 }

                 return ValueTypeString.ValueString.of(sb.toString());
commented

I don't think the performance benefits outweigh the cost in code readability here :-)

But happy to reconsider if you find there's a significant difference in performance.

(Plenty of opportunities in the rest of the code for more significant optimizations IMO)