refactor Networking implementation

[SpaceWalkerRS]

This PR reworks the implementations of the Networking API in order to fix some issues/bugs with it.

Issues

Data that is sent during the LOGIN events quietly fails to get through.

Context

Minecraft has custom payload packets that allow modded instances to send data between the client and server without Vanilla instances freaking out about unknown packets. Vanilla instances simply ignore any custom payloads they do not recognize. In addition to the data, custom payloads also have a channel. This allows mods to 'tune in' to just their own data, and ignore the data that was targeted at a different mod.

It is good practice for modded instances to only send data over a channel if the other side is listening for data on that channel. Usually this involves initiating a handshake to query whether the other side is listening. If there is no response, the channel is considered closed, and no data should be sent over that channel. If the handshake is reciprocated, the channel is considered open, and data can be sent over that channel.

OSL handles this by sending channel registration data between the client and server upon login. Once this data is received, both sides know what channels the other is listening to. Mods that use OSL for their networking purposes do not need to check whether the channels they use are open, since OSL does that for them. If the channel is closed, the data is quietly ignored. This is normally perfectly fine, since the channel is only closed when the other side is unmodded and the data should not be sent anyway. However, because the channel registration data is sent upon login, there is a brief window where the connection is in the 'play' phase, and thus, custom payload packets can be sent, but the game has not yet received the channel registration data, and thus all channels are considered closed. This causes all data that is sent during the LOGIN events to be quietly ignored.

Solution

The obvious solution is to send the channel registration data before the connection enters the 'play' phase, and thus have the channel registration completed by the time the LOGIN events are fired. Unfortunately this is not feasible, because it is not possible to send custom payloads during the 'handshake' or 'login' phase. The channel registration data can only be sent during the 'play' phase. It is also not feasible to delay the exchange of login packets until after the channel registration is completed, since that would lead to the login packets being indefinitely delayed for a connection to a Vanilla instance.

However, it turns out it is possible to modify the handshake packet without breaking Vanilla instances at all. Handshake packets contain a few bits of data that the server does absolutely nothing with. Thus, we can replace that data with something else without confusing Vanilla servers, that let modded servers know the client is also modded. This in turn prompts the server to send the channel registration data, which it does immediately after the connection enters the 'play' phase, just before the login packet is sent. The client reciprocates and upon receiving the login packet, can safely fire the LOGIN event since the channel registration happened just before.

The only loose end is the server side. It either needs to delay the firing of the LOGIN event until it receives the channel registration data, or otherwise any data sent by the server needs to be throttled until the channel registration data is received. Both of these can be safely done since the server already knows during the 'login' phase whether the client is modded at all, and if it isn't, that the server doesn't have to wait for the channel registration data.

Implementation

The address field in the HandshakeC2SPacket is unused, thus we replace its value with "OrnitheMC". When the modded server detects that value, it marks the Connection as modded. Immediately after the connection enters the 'play' phase, the server sends the channel registration data, just before the login packet is sent. The client receives the packets in that order, and can safely fire the LOGIN event.

[zeichenreihe]

I think the sting we send should at least give some information what exactly sets it, so replacing "OrnitheMC" with something like "net.ornithemc.osl.clientHasOsl=true" or similar would be better.

From what I can tell, the address and port field is set by the following, in vanilla 1.12.2:

• In Minecraft.startGame(String, String, WorldSettings) where SocketAddress.toString(), which just calls the Object.toString() method, is passed for address, and 0 for port.
• The parameters of ConnectScreen.connect(String, int), which can come from the following:
  • ConnectScreen.<init>(Screen, Minecraft, String, int), passing the third and forth parameter.
  • This is called from Minecraft.init(), passing this.serverAddress and this.serverPort. These fields are set in Minecraft.<init>(RunArgs), from RunArgs.Server.ip and RunArgs.Server.port. These fields are set directly from the parsed command line arguments, --server SERVER and --port PORT.
  • ConnectScreen.<init>(Screen, Minecraft, ServerListEntry), from ServerAddress.getAddress() and ServerAddress.getPort().
• In MultiplayerServerListPinger.add(ServerListEntry), which calls ServerAddress.getAddress() and ServerAddress.getPort().
• In RealmsConnect.connect(String, int), passing it's own parameters. This method isn't called from anywhere.

Note that ServerAddress.getAddress() returns java.net.IDN.toASCII(String), and if that throws an IllegalArgumentException, it returns "". ServerAddress.getPort() on the other hand is just a simple getter.

This gives the following sources for address and port:

• The command line arguments.
• The parameters of RealsmConnect.connect(String, int).
• An Object.toString() call, and 0. Note that .toString() is the default implementation, and in my java 8 it's getClass().getName() + "@" + Integer.toHexString(hashCode()), which is (in all cases) not equal to "OrnitheMC".
• The result of java.net.IDN.toASCII(String), and whatever port is.

Lets assume we want to trigger this manually, in a not modded case. These options exist for "breaking" it:

• For some reason realms calls it with the string "OrnitheMC". I don't know how these realms classes work, and I also don't expect anybody to use this anymore.
• I think that java.net.IDN.toASCII(String) may return "OrnitheMC". I did not check how this works internally, but I think "OrnitheMC" can be used as a host name, and can then also be resolved via custom DNS
• You can pass "OrnitheMC" via the command line arguments. For the packet to be send, it requires working DNS resolution for the host OrnitheMC. This will break the server's assumption of a modded client.

I suggest to change the string to be something longer and more descriptive. I also suggest starting the string with an more "illegal" char, such as \0, as people will not as easily input them. This char may be chosen in a way that prevents creating a connection to such a host.

The port field can in theory also be used to decrease the probability of a collision, by sending a magic number over there. Note that that field is written with writeShort(short) and read with readUnsignedShort().

Now let's check what mojang does when trying to connect in the ConnectScreen.connect(String, int):

• They call java.net.InetAddress.getByName(String) on the address. After some method calls, this calls this validation method, which prevents any "attack" if there's a \0 in the string,

  private static void validate(String host) throws UnknownHostException {
      if (host.indexOf(0) != -1) {
          throw new UnknownHostException("NUL character not allowed in hostname");
      }
  }

• For preventing the use of any of the ways containing a call to java.net.IDN.toASCII(String), I started testing with "\0OrnitheMC" to check what that toASCII method allows and where it throws IllegalArgumentExceptions, leading to "". As you can see below, the string "\0OrnitheMC" doesn't get changed. The string "xn--ö\nOrnitheMC" fails, as well as the very long (segment >64) string.

  
  jshell> java.net.IDN.toASCII("\0OrnitheMC")
  $1 ==> "\000OrnitheMC"

jshell> java.net.IDN.toASCII("xn--ö\0OrnitheMC") | Exception java.lang.IllegalArgumentException: The input starts with the ACE Prefix | at IDN.toASCIIInternal (IDN.java:327) | at IDN.toASCII (IDN.java:122) | at IDN.toASCII (IDN.java:151) | at (#2:1)

jshell> java.net.IDN.toASCII("111111111122222222223333333333444444444455555555556666666666\0OrnitheMC") | Exception java.lang.IllegalArgumentException: The label in the input is too long | at IDN.toASCIIInternal (IDN.java:336) | at IDN.toASCII (IDN.java:122) | at IDN.toASCII (IDN.java:151) | at (#3:1)

jshell>

(Note: this jshell was ran using java 20, but this shouldn't change things)
This means that using a name such as `"xn--ö\0OrnitheMC"` ensures that there is no way, except the realms one, for an "attacker" to bring it into state on a vanilla 1.12.2 client.

[SpaceWalkerRS]

what does the xn--ö part in the string do/mean?

[zeichenreihe]

The xn-- part ensures that this code cannot do the punycode encoding of non ASCII chars. The ö is such a non ASCII char.

// java.net.IDN.toASCIIInternal(), gets called in .toASCII()
    //
    // toASCII operation; should only apply to a single label
    //
    private static String toASCIIInternal(String label, int flag)
    {
        // step 1
        // Check if the string contains code points outside the ASCII range 0..0x7c.
        boolean isASCII  = isAllASCII(label);

        // ...

        if (!isASCII) {
            // step 4
            // If all code points are inside 0..0x7f, skip to step 8
            if (!isAllASCII(dest.toString())) {
                // step 5
                // verify the sequence does not begin with ACE prefix
                if(!startsWithACEPrefix(dest)){

                    // step 6
                    // encode the sequence with punycode
                    try {
                        dest = Punycode.encode(dest, null);
                    } catch (java.text.ParseException e) {
                        throw new IllegalArgumentException(e);
                    }

                    dest = toASCIILower(dest);

                    // step 7
                    // prepend the ACE prefix
                    dest.insert(0, ACE_PREFIX);
                } else {
                    throw new IllegalArgumentException("The input starts with the ACE Prefix");
                }

            }
        }

         // ...

In case this code throws this IllegalArgumentException, the minecraft code, in ServerAddress.getAddress() catches that exception and returns the empty string. This prevents people from getting our string to new HandshakeC2SPacket(), which would make a client modded when it isnt.

If I remember correctly, a domain having any non ASCII chars in them, is converted to punycode: The xn-- is used in that punycode encoding.

[SpaceWalkerRS]

The xn-- part ensures that this code cannot do the punycode encoding of non ASCII chars. The ö is such a non ASCII char.

ahh that makes sense - that should work out great then

[SpaceWalkerRS]

The ~1.12 implementation is now functional. Some last feedback would be appreciated before I start updating the other implementations.

[SpaceWalkerRS]

Here's a different idea: we fire the LOGIN event like normal, and initiate the channel registration upon login as well, but then fire a PLAY_READY event once channel registration is complete. Data can still not be sent during the LOGIN event, but mods should then use the PLAY_READY event instead. This is a lot cleaner to implement as we don't have to modify the handshake packet to make it work.