Sphereserver / Source-X

Ultima Online server emulator
Apache License 2.0
59 stars 47 forks source link

Incorrect use of INT8_MAX instead of UINT8_MAX during login #1337

Open LuxionUO opened 3 days ago

LuxionUO commented 3 days ago

Hi all,

It appears that the login process is using INT8_MAX (127) for a check involving a seed length that is always a positive number. However, this range is insufficient, as the values being processed during login can exceed the upper limit of INT8_MAX. For instance, the seed length is returning a value of 149, which falls outside the allowable range of INT8_MAX.

Proposed Solution To address this issue, I propose replacing INT8_MAX with UINT8_MAX (255). This change would properly accommodate the positive-only range required for this check and resolve issues related to exceeding the maximum limit.

As my server always uses ClassicUO Web, which sends an essential packet with a length of 149, this modification would resolve the issue for anyone using CUO Web. Given that CUO Web is likely to become more widely adopted in the future, this adjustment ensures compatibility going forward.

Steps to Reproduce

  1. Attempt to log in with a setup that processes a seed length exceeding 127.
  2. Observe that the seed length check fails due to the INT8_MAX limitation (e.g., during a breakpoint in Visual Studio).

The expected behavior is that the check should handle values within the range 0–255 without any issues.

Where to change: CNetworkInput.cpp

bool CNetworkInput::processUnknownClientData(CNetState* state, Packet* buffer)
{
    // process data from an unknown client type
    ADDTOCALLSTACK("CNetworkInput::processUnknownClientData");
    EXC_TRY("ProcessNewClient");
    ASSERT(state != nullptr);
    ASSERT(buffer != nullptr);
    CClient* client = state->getClient();
    ASSERT(client != nullptr);

    bool fHTTPReq = false;
    const uint uiOrigRemainingLength = buffer->getRemainingLength();
    const byte* const pOrigRemainingData = buffer->getRemainingData();
    if (state->m_seeded == false)
    {
        fHTTPReq = (uiOrigRemainingLength >= 5 && memcmp(pOrigRemainingData, "GET /", 5) == 0) ||
            (uiOrigRemainingLength >= 6 && memcmp(pOrigRemainingData, "POST /", 6) == 0);
    }
    if (!fHTTPReq && (uiOrigRemainingLength > INT8_MAX)) // Should this be UINT8_MAX instead?
    {
        g_Log.EventWarn("%x:Client connected with a seed length of %u exceeding max length limit of %d, disconnecting.\n", state->id(), uiOrigRemainingLength, INT8_MAX); // Same here?
        return false;
    }

Please let me know if my understanding is incorrect or if there are additional considerations for this change.

Thank you!

Jhobean commented 2 days ago

Since classic UO web is a new client version. Maybe we need to update some value on sphere PacketClientVersion::PacketClientVersion()

OSI just decide to choose this version for the new OSI shard.