4
\$\begingroup\$

I coded an executable program (.exe) that I only want run either from my home computer, our main server, or people in our development team.

I have coded logic that will only allow the program to be run from certain IP addresses.

The .txt file that's referenced in the getIPlist function looks like this:

[123.456.78.90] = Main Server
[77.34.555.392] = My computer at home
[333.455.3.3] = Assistant coder

And the HTTP address that the parseAllowableIPs function points to is a PHP file that is simply coded:

<?php echo $_SERVER['REMOTE_ADDR']; ?>

This code is working great (except for a bit of memory not being freed), so a review or some efficiency tips are welcome.

bool compareIPs(std::string IPlist, std::string userIP)
{
    const char *U = userIP.c_str();
    char str[20];

    const char *goodList[25] = { '\0' };

    std::string uIP = "";

    while (*U)
    {
        if (atoi(U))
        {
            uIP += itoa(atoi(U), str, 10);
            while (*U && *U != '.')
                *U++;

            if (*U && *U == '.')
                uIP += ".";
        }

        if (*U)
            *U++;
    }

    const char *L = IPlist.c_str();
    int count = 0;
    std::string thisIP = "";

    while (*L)
    {
        switch (*L)
        {
        case '[':
            *L++;

            while (*L && *L != ']')
            {
                thisIP += *L;
                *L++;
            }

            goodList[count] = _strdup(thisIP.c_str());
            thisIP = "";
            count++;
            break;
        default:
            break;
        }

        if (*L && *L != '[')
            *L++;
    }

    std::string comp = "";

    // Now check to see if the user's IP matches any in the goodList[]
    for (int a = count; a >= 0; a--)
    {
        if (!goodList[a])
            continue;

        comp = goodList[a];

        if (!uIP.compare(comp))
        {
            // I need to free() what was _strdup()'d
            // but this causes the progam to crash.

            //free(&goodList[a]);

            return true;
        }
    }

    return false;
}

#pragma comment(lib, "WinInet.Lib")

std::string getIPlist()
{
    HINTERNET hInternet, hFile;
    DWORD rSize;
    char buffer[1024];

    hInternet = InternetOpen(NULL, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 0);
    hFile = InternetOpenUrl(hInternet, "http://www.myWebServer.com/ip/authorizedIPlist.txt", NULL, 0, INTERNET_FLAG_RELOAD, 0);
    InternetReadFile(hFile, &buffer, sizeof(buffer), &rSize);
    buffer[rSize] = '\0';

    InternetCloseHandle(hFile);
    InternetCloseHandle(hInternet);

    std::string result = buffer;

    return result;
}

bool parseAllowableIPs()
{
    // Grap the list of allowable IP's and store it in a string
    // to be parsed in compareIPs() function.
    std::string allowableIPlist = getIPlist();

    HINTERNET hInternet, hFile;
    DWORD rSize;
    char buffer[1024];

    hInternet = InternetOpen(NULL, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 0);
    hFile = InternetOpenUrl(hInternet, "http://www.myWebServer.com/ip/index.php", NULL, 0, INTERNET_FLAG_RELOAD, 0);
    InternetReadFile(hFile, &buffer, sizeof(buffer), &rSize);
    buffer[rSize] = '\0';

    InternetCloseHandle(hFile);
    InternetCloseHandle(hInternet);

    std::string result = buffer;

    if (!compareIPs(allowableIPlist, result))
        return false;

    return true;
}

int main()
{
    //... blah blah blah

    // If not an authorized IP, just exit the game.
    if (!parseAllowableIPs())
    {
        MySQL__disconnect();
        exit(588924);
    }
}
\$\endgroup\$
2
\$\begingroup\$

It took me a while to figure out what the code was trying to do. I guess that http://www.myWebServer.com/ip/index.php serves a purpose similar to http://www.whatismyip.com, echoing the HTTP client's IP address. That is better than just enumerating the IP addresses on the client machine's interfaces, as it ensures that you pick an address that is actually routable.

Like all DRM schemes, this one is crackable too. One simple workaround is to use a transparent HTTP proxy that intercepts the call-home message and fakes a response. Using HTTPS with server certificate validation would help prevent that man-in-the-middle attack.

The parseAllowableIPs() function is misnamed. It's not just parsing — it's also fetching, reading, and making an allow/deny decision. A more appropriate name would be isClientIPAuthorized().

compareIPs() is doing more than its fair share. Instead of returning a std::string, getIPlist() should also split the text into a std::vector<std::string>. There should also be a getMyIP() function. Then, compareIPs() would be a simple verification of whether an item is in a list.

In summary, the outline should be more like:

bool isClientIPAuthorized()
{
    std::string me = getMyIP();
    std::vector<std::string> allowed = getAllowableIPs();
    return std::find(allowed.begin(), allowed.end(), me) != allowed.end();
}

int main()
{
    //... blah blah blah

    // If not an authorized IP, just exit the game.
    if (!isClientIPAuthorized())
    {
        MySQL__disconnect();
        exit(588924);
    }
}

However, I'd go a step further. The dotted-quad parsing code in compareIPs() has some minor issues, such as superfluous pointer dereferencing. (Note that it would be better to write U++; rather than *U++;, even though the effect is the same.) It would be better to avoid reinventing the wheel, and using a standard IP address parsing routine. (It's such a common task, a few minutes with Google and MSDN is worth it.) There's the Winsock function inet_aton(), which parses a IPv4 dotted-quad address into a long, and the newer InetPton() that also handle IPv6. So, getMyIP() and getAllowableIPs() should probably return long and std::vector<long>, respectively, instead of strings.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Given your code as-is, use

free(goodList[a]);

instead of

free(&goodList[a]);

since goodList[a] is already a pointer to the const char that you want to free.

And then there will be no more memory leak.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.