LEGO® Universe Authentication Server

Question

^{Please note that all credit for code goes to humanoid24 and I have only helped the bare minimum required to be able to post it here.}

I am working with some friends to replicate a server for the game LEGO® Universe which was shut down two years ago. This code is included in the LUNIServer v3 here.

The server does what it's supposed to do, but I'm wondering if anything could be improved. If you have any tips to make this more efficient (not necessarily smaller), please post them. I'll need to change stuff before the server grows larger, making a major change hard to do.

/*
    Lego Universe Authentification Server WIP
    Code done by humanoid24 (aka Triver) as of 01.12.2013

    This source code requires RakNet version 3.25 as 
    an external dependency to work with the luni client.
*/
//#define USE_ENCRYPTION    // is actually not required for a connection (but may be useful once login works because of user password)

#include "serverLoop.h"
#include <fstream>
//#if defined USE_ENCRYPTION
    #include "RSACrypt.h"
//#endif

void loadConfig(CONNECT_INFO* cfg)  // do a very simple and dirty config loading
{
    memset(cfg->redirectIp, 0, sizeof(cfg->redirectIp));
    std::ifstream in(".\\config.ini", std::ios::in);
    if(in.is_open())
    {
        std::string temp((std::istreambuf_iterator<char>(in)), std::istreambuf_iterator<char>());
        in.close();
        size_t pos = temp.find("redirect_ip=", 0)+strlen("redirect_ip=");
        size_t end = temp.find_first_of('\n', pos);
        if(end > pos)
        {
            std::string value = temp.substr(pos, end-pos);
            if(value.size() < sizeof(cfg->redirectIp))
                memcpy(cfg->redirectIp, value.c_str(), value.size());
            else
                printf("specified host exceeds limit of 15!\n");
            pos = temp.find("redirect_port=", end)+strlen("redirect_port=");
            end = temp.find_first_of('\n', pos);
            if(end > pos)
            {
                cfg->redirectPort = atoi(temp.substr(pos, end-pos).c_str());
                pos = temp.find("listen_port=", end)+strlen("listen_port=");
                end = temp.find_first_of('\n', pos);
                if(end > pos)
                {
                    cfg->listenPort = atoi(temp.substr(pos, end-pos).c_str());
                    pos = temp.find("use_encryption=", end)+strlen("use_encryption=");
                    end = temp.find_first_of('\n', pos);
                    if(end > pos)
                    {
                        std::string value = temp.substr(pos, end-pos);
                        if(value.compare("true") == 0 || value.compare("1") == 0)
                            cfg->useEncryption = true;
                        else
                            cfg->useEncryption = false;
                        pos = temp.find("log_file=", end)+strlen("log_file=");
                        end = temp.find_first_of('\n', pos);
                        if(end == std::string::npos)
                            end = temp.size();
                        if(end > pos)
                        {
                            value = temp.substr(pos, end-pos);
                            if(value.compare("true") == 0 || value.compare("1") == 0)
                                cfg->logFile = true;
                            else
                                cfg->logFile = false;
                        }
                    }
                }
            }
        }
    }
    else
    {
        printf("Couldn't open config.ini, check if file exists and if you have permission\n");
        cfg->redirectPort = -1;
        cfg->listenPort = 1001; // just set auth port as default
        cfg->logFile = true;
        cfg->useEncryption = false;
    }
}

void InitSecurity(RakPeerInterface *rakServer, bool useEncryption)
{
    // luni client requires this password, seems to be the same for all versions (even alpha/beta)
    rakServer->SetIncomingPassword("3.25 ND1", 8);
    if(useEncryption)
    {
        // These are the sizes necessary for e,n,p,q
        big::u32 e; RSA_BIT_SIZE n;                                 // e,n is the public key
        BIGHALFSIZE(RSA_BIT_SIZE, p); BIGHALFSIZE(RSA_BIT_SIZE, q); // p,q is the private key

        bool keyLoaded = false;
        FILE* fp = fopen("public_key.bin", "rb");
        if(fp)
        {
            printf("Loading public key... ");
            fread((char*)(&e), sizeof(e), 1, fp);
            fread((char*)(n), sizeof(n), 1, fp);
            fclose(fp);
        }
        else
            printf("Failed to open \"public_key.bin\".\n");

        fp = fopen("private_key.bin", "rb");
        if(fp)
        {
            printf("Loading private key... ");
            fread(p, sizeof(RSA_BIT_SIZE)/2, 1, fp);
            fread(q, sizeof(RSA_BIT_SIZE)/2, 1, fp);
            fclose(fp);
            printf("Done.\n");
            keyLoaded=true;
        }
        else
            printf("Failed to open \"private_key.bin\".\n");

        big::RSACrypt<RSA_BIT_SIZE> rsacrypt;
        if(!keyLoaded)      // if there's no predefined key just generate one (and write them on disk)
        {
            printf("Generating %i byte key.\n", sizeof(RSA_BIT_SIZE));

            rsacrypt.generateKeys();
            rsacrypt.getPublicKey(e,n);
            rsacrypt.getPrivateKey(p,q);
            printf("Keys generated.\n");
            printf("Writing public key... ");
            FILE* fp = fopen("public_key.bin", "wb");
            fwrite((char*)&e, sizeof(e), 1, fp);
            fwrite((char*)n, sizeof(n), 1, fp);
            fclose(fp);

            printf("Writing private key... ");
            fp = fopen("private_key.bin", "wb");
            fwrite(p, sizeof(RSA_BIT_SIZE)/2, 1, fp);
            fwrite(q, sizeof(RSA_BIT_SIZE)/2, 1, fp);
            fclose(fp);
            printf("Done.\n");
        }
        else
            rsacrypt.setPrivateKey(p, q);

        //rakServer->InitializeSecurity(0,0,0,0);                   // directly generate the keys
        rakServer->InitializeSecurity(0, 0, (char*)p, (char*)q);    // e and n are actually not required
    }
}

int main(void)
{
    RakPeerInterface *rakServer = RakNetworkFactory::GetRakPeerInterface();
    CONNECT_INFO cfg;
    loadConfig(&cfg);

    PacketLogger msgConsoleHandler;
    PacketFileLogger msgFileHandler;
    rakServer->AttachPlugin(&msgConsoleHandler);
    if(cfg.logFile)
        rakServer->AttachPlugin(&msgFileHandler);

    printf("Initializing luni test server\n");
    InitSecurity(rakServer, cfg.useEncryption);
    SocketDescriptor socketDescriptor(cfg.listenPort, 0);   // port 1001 == luni auth, ports 2001-2200 == char or world

    bool success = rakServer->Startup(8, 30, &socketDescriptor, 1);
    rakServer->SetMaximumIncomingConnections(8);

    if(success) 
    {
        printf("Successfully initialized, listening for packets on port %i...\n\n", cfg.listenPort);
        printf("RakNet log format:\n");
        msgConsoleHandler.LogHeader();
        if(cfg.logFile)
        {
            msgFileHandler.StartLog(0);
            listenForPackets(rakServer, &msgFileHandler, &cfg);
        }
        else
            listenForPackets(rakServer, NULL, &cfg);
    }
    else
    {
        char* quitMsg = "Failed to initialize the server, exiting...\n";
        msgFileHandler.WriteLog(quitMsg);
        printf(quitMsg);
    }
    rakServer->Shutdown(0);
    RakNetworkFactory::DestroyRakPeerInterface(rakServer);
}

Answer 1

I've only skimmed a little bit of your code, but here's what I found so far:

Always make sure std::string methods do not return std::string::npos.

size_t pos = temp.find("redirect_ip=", 0)+strlen("redirect_ip=");

This can easily lead to undefined behavior. size_t is an unsigned integral type. On some systems, std::string::npos is defined as 0xffffffff. You are guaranteed an overflow if this is the case on your system and your call to find fails.

    size_t end = temp.find_first_of('\n', pos);
    if(end > pos)
    {
        std::string value = temp.substr(pos, end-pos);

If end == std::string::npos, then you're going to be reading way out of bounds.

if(value.compare("true") == 0 || value.compare("1") == 0)

There is no reason to use compare here. An std::string's operator== is overloaded to check for equality. This would be better:

if(value == "true" || value == "1")

Your entire structure of nested if statements is ugly to me. I would rather use exceptions instead or return early.

Be consistent with your API. In loadConfig, you use C++-style streams, but in InitSecurity, you use C-style file descriptors. Unless you've profiled your code and decided C++-style streams are too slow, I would stick to those. If you have profiled your code and decided that C-style descriptors give you an adequate boost, then you should document that detail in a comment.