PHP code to make a WHOIS request and return the received data

Question

I have written some PHP to make WHOIS queries and return the retrieved WHOIS record.

The code is an excerpt from a much larger program used for monitoring domain name expiry. It's a command-line program designed to be run in a shell such as Bash or PowerShell, and has no web services involved.

Firstly, there is a function to identify the first-level TLD for the domain:

//Return the first-level TLD for a domain
function getTld($domain) {
    //If domain contains dot, find first-level TLD and return it
    if(strpos($domain, ".")) {
        return(pathinfo($domain, PATHINFO_EXTENSION));
    //If domain doesn't contain a dot, just return domain as-is
    } else {
        return($domain);
    }
}

Then, the first-level TLD is prepended to .whois-servers.net, and a WHOIS lookup is made to it over TCP port 43:

//Return raw WHOIS data for domain
function whois($domain) {
    $whoisServer = getTld($domain) . ".whois-servers.net";
    set_error_handler("whoisWarningHandler", E_WARNING);
    $whois = fsockopen($whoisServer, 43);
    restore_error_handler();
    stream_set_timeout($whois, 3);
    stream_set_blocking($whois, false);
    fwrite($whois, $domain . "\r\n");
    $read = [$whois]; $retries = 1; $null = null;
    while(stream_select($read, $null, $null, 5) === false) {
        if($retries++ >= 5) {
            return(0);
        }
        print("Retrying... (attempt " . $retries . ")");
        sleep(1);
    }
    sleep(1);
    return(filter_var(fread($whois, 65535), FILTER_DEFAULT, FILTER_FLAG_STRIP_HIGH));
}

Finally, there is a custom warning handler to terminate the program if a connection to the WHOIS server cannot be established:

//Custom warning handler for WHOIS fsock/stream
function whoisWarningHandler($errno, $errstr) {
    print("An error occurred while connecting to the WHOIS server.\n\nError output:\n\n" . $errstr . "\n");
    exit(2);
}

The code may then be called using e.g. print(whois("example.com"));, which would print the WHOIS record for example.com.

Please could anyone advise as to whether best practise for code quality, efficiency and security has been followed, and any recommended adjustments/improvements that could be made?

Answer 1

I'm not fully versed on the use of streams in PHP, so I may miss stuff. You also haven't specified the context in which this will be used, or a threat-model, so it's hard to asses if it's "secure".

Efficiency

You should close the socket when you're done with it.

What are the sleep(1) lines for? You're already waiting for data to become available on the socket.

I would be tempted to make your timeouts and retry count optional arguments with default values, but that's not a huge deal.

Security

Most of the security concerns are probably outside your control.

How do you know if the server you're opening the socket to is the one you actually wanted? Do you care if a 3rd party is reading these messages? Could a 3rd party alter the messages in transit?

Very likely these aren't problems you need to worry about, but depending on the application they might be.

Code Quality

• Calling exit() from an error handler probably isn't a great idea. If this is going to be used in a web-service of any kind, then this is actually a security issue: Anyone who can cause problems with your connection to the whois server can now break your site.
• Format the comment at the top as a PHPDoc to help IDEs and intellisense.
• Don't do multiple assignments on one line.
• Don't name a variable $null. You could name it $dummy or $temp or have separate $read and $except. You may also have the option of passing anonymous empty arrays in-line.
• return isn't typically written like a function; it's a language command on the level of try or class.
• When you're writing a loop, your first thought should be a for loop (or foreach), which will serve just fine here.
• Use type-signatures for your functions.

All that get's us this far:

/* Return raw WHOIS data for domain.
 * 
 * @param string $domain The domain to ask about.
 * @param int $timeout The time to spend waiting for the WhoIs server, 
 *   which we will do at least twice.
 * @param int $tries How many times to poll the open socket for data.
 */
function whois(string $domain, int $timeout = 3, int $tries = 5):string {
    $retval = 0;
    $whoisServer = getTld($domain) . ".whois-servers.net";

    set_error_handler("whoisWarningHandler", E_WARNING);
    $whois = fsockopen($whoisServer, 43);
    restore_error_handler();

    stream_set_timeout($whois, $timeout);
    stream_set_blocking($whois, false);
    fwrite($whois, $domain . "\r\n");

    for($try = 1; ($try <= $tries) && !$retval; $try++){
        printf(
            'WhoIs try %d. Already waited %d seconds; will wait %d more.',
            $try, ($try - 1) * %timeout, $timeout)
        if(stream_select([$read], [], [], $timeout)){
            $retval = filter_var(
                fread($whois, 65535),
                FILTER_DEFAULT,
                FILTER_FLAG_STRIP_HIGH);
        }
    }

    fclose($whois);

    return $retval;
}

Error handling

Your best bet here is to trash your work so far and start over in any other language besides PHP.

There are at least four different kinds of failures you should be concerned about:

• Exceptions could be thrown.
  • Don't assume we know where this might happen. Generally, if an expected exception happens then, as long as the necessary logging and cleanup are happening, it's appropriate for the system to crash.
• Warnings could be thrown, and can also cause execution to stop or otherwise jump.
  • fsockopen does this, as you've noticed.
  • stream_select does this.
• Functions can set error flags which you need to then inspect.
  • fsockopen does this.
• Functions can return values that represent failure.
  • Most functions do this.

You need to make sure that, no matter what happens, failures are logged appropriately and system resources like the socket you're opening get closed. Depending on the nature of the failure, you may also want execution to continue in a predictable way.

This can get really ugly and verbose if you try to brute-force cover every angle. My suggestion would be to write a function that takes as an argument a function that takes an open socket as an argument. The outer function can be in charge of setting up and then closing the socket "no matter what", and the inner function can focus on happy-path business logic.

In practice, as I said before, the efficient way forward is to either accept your code as imperfect, or start over in a different language.

Answer 2

I have accepted the answer from @ShapeOfMatter, however I have documented the actions/comments for each of their findings below.

This was my first question on Code Review SE, and I realise I should have been more clear about the usage context of the code and the fact that it's an excerpt from a larger application.

Efficiency

You should close the socket when you're done with it.

Thanks, implemented in the full program.

What are the sleep(1) lines for? You're already waiting for data to become available on the socket.

The sleeps are to add a delay between WHOIS lookups if whois() is called from a loop (which it is in the full program). This is to help prevent hitting WHOIS server rate limits.

Having the sleep inside the whois() function isn't ideal, so I've moved this to the loop that calls the function instead. In the full program the sleep time can also be manually overridden via a command-line argument.

I would be tempted to make your timeouts and retry count optional arguments with default values, but that's not a huge deal.

The hard-coded timeout and retries are fine for now, but I have added a backlog item to reconsider in future.

Security

Most of the security concerns are probably outside your control.

How do you know if the server you're opening the socket to is the one you actually wanted? Do you care if a 3rd party is reading these messages? Could a 3rd party alter the messages in transit?

Very likely these aren't problems you need to worry about, but depending on the application they might be.

The program is designed with the idea that all WHOIS data is untrusted, as it is a plaintext protocol that has no native encryption or integrity checking.

The attacks that I am primarily concerned about are RCE, local information disclosure and DoS caused by reading malformed/malicious data from a [spoofed/compromised] WHOIS server. In the full program, the WHOIS data is parsed and certain content extracted out of it, however this is only string manipulation - the content is never executed, used to write arbitrary files, used to connect to arbitrary network locations, etc.

The WHOIS lookups and responses do not need to be confidential in this case.

I have conducted around 12 hours of fuzz testing against the WHOIS function using Radamsa, where fuzzed WHOIS responses were repeatedly served in order to test the resilience and robustness of the code. No crashes, hangs or other issues were identified, and all errors were handled as expected.

Code Quality

Calling exit() from an error handler probably isn't a great idea. If this is going to be used in a web-service of any kind, then this is actually a security issue: Anyone who can cause problems with your connection to the whois server can now break your site.

This is part of a command-line application with no associated web services. I should have clarified this in the original question.

Outputting the error and exiting is the intended action for the error handler.

Format the comment at the top as a PHPDoc to help IDEs and intellisense.

Good idea, thanks. Added this as a backlog item.

Don't do multiple assignments on one line.

The full program doesn't have this, I had actually added these manually when writing the Stack Exchange question, to reduce the number of code lines in the question box.

Don't name a variable $null. You could name it $dummy or $temp or have separate $read and $except. You may also have the option of passing anonymous empty arrays in-line.

Thanks, adjusted in the full program.

return isn't typically written like a function; it's a language command on the level of try or class.

Thanks, adjusted all returns in the full program.

When you're writing a loop, your first thought should be a for loop (or foreach), which will serve just fine here.

A while loop is acceptable for now, however I have added this as a backlog item to reconsider in future.

Use type-signatures for your functions.

Thanks, adjusted all functions in the full program.