Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement over-engineered diceroll #479

Open
wants to merge 12 commits into
base: fabric
Choose a base branch
from

Conversation

wagyourtail
Copy link
Contributor

@wagyourtail wagyourtail commented Dec 19, 2022

uses a modified diffie-hellman key exchange to prevent cheating

@wagyourtail wagyourtail changed the title implement over-engineered coinflip implement over-engineered diceroll Dec 20, 2022
@xpple
Copy link
Collaborator

xpple commented Dec 26, 2022

Sorry if this is a strange request but could you perhaps extract the C2C packet changes to a separate PR?

@wagyourtail
Copy link
Contributor Author

I can't completely remove them from this one or it'll break this one. but I can have them as a seperate branch that this one is based uppon

@xpple
Copy link
Collaborator

xpple commented Dec 28, 2022

Sure, thanks!

@Starmania
Copy link
Contributor

Would it be useful to use 2048 bit keys? A wikipedia article

@wagyourtail
Copy link
Contributor Author

Would it be useful to use 2048 bit keys? A wikipedia article

yes, but they don't fit in a minecraft chat message, xd and for our purposes I don't think that'll be an issue

Copy link
Collaborator

@xpple xpple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand much of the technical stuff so these are mostly style related changes.

import net.minecraft.network.PacketByteBuf;

public interface C2CPacket {
void write(PacketByteBuf buf);

void apply(CCPacketListener listener);
void apply(CCPacketListener listener) throws CommandSyntaxException;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is out of the scope of command syntax exceptions. Perhaps we should create our own exception class or just return instead of throwing an exception.

Comment on lines +97 to +101
public static void register() {
CCPacketHandler.register(DiceRollInitC2CPacket.class, DiceRollInitC2CPacket::new);
CCPacketHandler.register(DiceRollAcceptedC2CPacket.class, DiceRollAcceptedC2CPacket::new);
CCPacketHandler.register(DiceRollResultC2CPacket.class, DiceRollResultC2CPacket::new);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's necessary to register them here. I don't know why CCPacketHandler#register wasn't private in the first place.

@@ -4,7 +4,10 @@
import com.mojang.brigadier.exceptions.DynamicCommandExceptionType;
import com.mojang.brigadier.exceptions.SimpleCommandExceptionType;
import com.mojang.logging.LogUtils;
import io.netty.buffer.Unpooled;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import.

Suggested change
import io.netty.buffer.Unpooled;

import java.security.SecureRandom;
import java.util.*;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import.

Suggested change
import java.util.concurrent.atomic.AtomicInteger;

Comment on lines +66 to +73
dispatcher.register(literal("cdiceroll")
.executes(ctx -> localDiceroll(ctx.getSource(), 6))
.then(argument("sides", integer(2, 100))
.executes(ctx -> localDiceroll(ctx.getSource(), getInteger(ctx, "sides")))
.then(argument("player", gameProfile())
.executes(ctx -> diceroll(ctx.getSource(), getCProfileArgument(ctx, "player"), getInteger(ctx, "sides")))))
.then(argument("player", gameProfile())
.executes(ctx -> diceroll(ctx.getSource(), getCProfileArgument(ctx, "player"), 6))));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation.

Comment on lines +113 to +122
"commands.diceroll.diceroll": "dice roll (%s side)",
"commands.diceroll.coinflip": "coin flip",
"commands.diceroll.sent": "Sent %s request to %s",
"commands.diceroll.received": "Received %s request from %s",
"commands.diceroll.value": "%s result from %s: %s",
"commands.diceroll.heads": "Heads",
"commands.diceroll.tails": "Tails",
"commands.diceroll.cheater": "I think %s is cheating, they got a different value.",
"commands.diceroll.timeout": "Timeout waiting for %s's response to %s request",
"commands.diceroll.waitingForResponse": "Waiting for %s's response already",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be cdiceroll. Also I think some of these shouldn't be a subkey of commands, but rather of ccpacket.

try {
md = MessageDigest.getInstance("SHA-1");
}
catch(NoSuchAlgorithmException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
catch(NoSuchAlgorithmException e) {
catch (NoSuchAlgorithmException e) {

Comment on lines +220 to +221
System.out.println("expected: " + byteArrayToHexString(opponentHash.get(packet.sender)));
System.out.println("actual: " + byteArrayToHexString(toSHA1(B.toByteArray())));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be removed.


public class DiceRollCommand {
private static final Logger LOGGER = LogUtils.getLogger();
private static final SimpleCommandExceptionType PLAYER_NOT_FOUND_EXCEPTION = new SimpleCommandExceptionType(Text.translatable("commands.cwe.playerNotFound"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's an idea to add a few common exceptions for c2c related stuff to a certain class, I don't like the commands.cwe translation key being used here.

@xpple xpple added the enhancement New feature or request label Jul 3, 2023
@RealRTTV
Copy link
Contributor

might i ask: how does this work, can't i just generate these values on my client over and over until i get the one i want, then send it?

@Semisol
Copy link

Semisol commented Oct 8, 2024

A simpler solution for fairness could be implemented:

  • Initiator generates s_i, sends H(s_i) (commit)
  • Receiver saves H(s_i), generates s_r, sends H(s_r) (commit)
  • Initiator saves H(s_r), sends s_i (reveal)
  • Receiver verifies H(s_i received) = H(s_i) saved, calculates xor s_i ^ s_r = s, sends s_r, shows dice roll result (reveal)
  • Initiator verifies H(s_r received) = H(s_r) saved, calculates xor s_i ^ s_r = s, shows dice roll result

This is secure, as:

  • the committed value cannot be changed after the fact, and no values are revealed before commit is complete, so you cannot bias your value to get an expected xor output
  • xoring two values that are not correlated can only increase entropy (correlated being generated from the same source as the other value, or generated with knowledge of the other value to intentionally bias the xor)
  • there is an attack where receiver can withhold revealing, but this should result in a big warning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants