BeardedManStudios / ForgeNetworkingRemastered

See various forks, also please join the Forge Community fork of Forge Alloy! -> https://github.com/ForgeAlloyCommunity/ForgeAlloy
https://twitter.com/FarrisFaulds
Apache License 2.0
1.49k stars 305 forks source link

[wiki] Basic Authentication possible fix #362

Closed eusebiu closed 4 years ago

eusebiu commented 4 years ago

Version Number and Operating System(s):

master; Windows

Expected behavior:

Get the correct data set from client.

Actual behavior:

Exception in response.GetByteArray()

Steps to reproduce:

  1. create a client authenticator to send the data to server; set it before Connect()

    public void AcceptChallenge(NetWorker networker, BMSByte challenge, Action<BMSByte> authServerAction, Action rejectServerAction)
    {
    var data = ObjectMapper.BMSByte(0, Encoding.UTF8.GetBytes($"test:test"));
    
    authServerAction(data);
    }
  2. create a server authenticator to read the data

    public void VerifyResponse(NetWorker networker, NetworkingPlayer player, BMSByte response, Action<NetworkingPlayer> authUserAction, Action<NetworkingPlayer> rejectUserAction)
    {
    ulong steamid = response.GetBasicType<ulong>();
    byte[] ticketBinary = response.GetByteArray(response.StartIndex()); // here exception
    
    if (AuthUser(steamid, ticketBinary))
    {
        authUserAction(player);
    }
    else
    {
        rejectUserAction(player);
    }
    }

Possible fix of VerifyResponse:

if (response == null || response.byteArr == null || 
    response.byteArr.Length == 0 || response.Size == 0)
{
    rejectUserAction(player);
    return;
}

ulong steamid = response.GetBasicType<ulong>();

byte[] ticketBinary = new byte[response.Size];
Buffer.BlockCopy(response.byteArr, response.StartIndex(), ticketBinary, 0, ticketBinary.Length);
phalasz commented 4 years ago

First thing I noticed is you map a 0 then in verify response you try and get out a ulong. 0 != 0ul when you are mapping it out as the first would map to 4 bytes while a ulong would map to 8 bytes.

Your problem was that your first call to get the id read more data then the internall array had. It also moved the start pointer. Your next call was instructing it to get data from an already moved start pointer that was pointing out of range for the internal array hence the exception.

So your example code should be written like this:

public void AcceptChallenge(
    NetWorker networker,
    BMSByte challenge,
    Action<BMSByte> authServerAction,
    Action rejectServerAction
) {
    ulong steamId = yourMethodThatGetsThis();
    var data = ObjectMapper.BMSByte(steamId, Encoding.UTF8.GetBytes($"test:test"));

    authServerAction(data);
}

Then on the server:

public void VerifyResponse(
    NetWorker networker,
    NetworkingPlayer player,
    BMSByte response,
    Action<NetworkingPlayer> authUserAction,
    Action<NetworkingPlayer> rejectUserAction
) {
    ulong steamid = response.GetBasicType<ulong>();  // <--- Moves internal array pointer.
    byte[] ticketBinary = response.GetBasicType<byte[]>();

    if (AuthUser(steamid, ticketBinary))
    {
        authUserAction(player);
    }
    else
    {
        rejectUserAction(player);
    }
}
eusebiu commented 4 years ago

You are correct but:

If I change as you mention, I now get an OutOfMemoryException: response.GetBasicType<byte[]>() => at x = GetBasicType(StartIndex(), true); // x gets a huge value

If I remove the steamid completely from client and server (since I do not use it), the data sent is not correctly retrieved (in my case it is an encrypted text and decryption does not recognize it).

So, there is an issue of 4 bytes somewhere...

phalasz commented 4 years ago

That is weird. Haven't seen that before and I have used byte[]s multiple times.

Just an fyi you should not touch byteArr as that is the internal array that might not be what you expect it to be. You can get out the correct byte[] by using CompressBytes()

eusebiu commented 4 years ago

So, if I do this on the client side:

var data = ObjectMapper.BMSByte(Encoding.UTF8.GetBytes($"test:test"));
authServerAction(data);

and byte[] ticketBinary = response.CompressBytes(); on server side, the data is not correctly retrieved...

image

phalasz commented 4 years ago

Yes you are seeing the meta data in front. The first 4 bytes is the number of elements as an int of your byte array that you mapped in. Then you have the 9 elements.

Please use byte[] ticketBinary = response.GetBasicType<byte[]>();

You could just map the string instead of the byte array though.

eusebiu commented 4 years ago

Mapping the string directly worked. Thanks! :)