Fix for Bug #4231 breaks support for optional method parameters

Addendum to PEAR Bug #4231 for package XML_RPC, reported by Mike Naberezny on 6 May 2005. See also my original report of PEAR Bug #4231.

Description

After I submitted bug #4231, a patch different than my suggestion was implemented for 1.3.0RC2. This did indeed correct the original problem of bug #4231 (bad parameter counts causing PHP notices). However, there was a very specific reason that I submitted the fix as I did and not as was done in the release. The fix in 1.3.0RC2 creates a new problem (and a BC break) that my suggested fix did not.

It may not be immediately obvious but the dispatch map as it has been implemented in XML_RPC_Server allows for a single RPC method to have multiple signatures. This implements support for both polymorphic parameter types and also different parameter counts (i.e., optional parameters). The fix for bug #4231 as it was implemented in 1.3.0RC2 breaks support for the signatures of a method to have different counts of parameters.

The official XML-RPC specification makes no mention of optional parameters in method calls, however all clients inherently support them. Using them where appropriate has the advantage of lightening XML-RPC's already heavy request playload by alleviating the need to send some unnecessary parameters. The decision of whether or not to support optional parameters is left entirely to the RPC API designer. I feel that we should continue to support it for these reasons, and also because existing XML_RPC package users may already be using this feature (at least, I do).

Reproduce Code

To demonstrate the use of optional parameters and how 1.3.0RC2 breaks this feature, we must first create a server based on XML_RPC_Server with an RPC method that has an optional parameter. To implement this, we'll need to create two signatures for the method: one with the optional parameter supplied, and one without.

<?php
require_once('XML/RPC/Server.php'); 

// dispatch map for functions mapped to RPC method calls
$dispMap = array();
$dispMap['getParamCount'] = array('function' => 'rpc_getParamCount',
                                  'signature' => array( array('int', 'string', 'string'), 
                                                        array('int', 'string', 'string', 'boolean')), 
                                  'docstring' => 'Call me with 2 or 3 parameters of the correct types.');

// service XML-RPC request now
$server = new XML_RPC_Server($dispMap, 1, 1);

/** 
 * A function that demonstrates the creation of XML-RPC methods that accept multiple parameter
 * counts (optional parameters).  This method can be called with either two or three parameters.
 * Other parameter counts or type mismatches will be faulted by XML_RPC_Server::verifySignature() 
 * before this function is called.
 *
 * XML-RPC Signatures: int getParamCount(string, string)
 *                     int getParamCount(string, string, boolean)
 *
 * @param  XML_RPC_Message      $params 
 * @return XML_RPC_Response     +XML_RPC_Value int
 */ 
function rpc_getParamCount($params) 
{
    if ($params->getNumParams()==3) {
        // process all three parameters (optional third parameter was supplied)
    } else {
        // process only the two required parameters
    }

    // return the count of parameters received, if you want to explore further on your own.
    $val = new XML_RPC_Value($params->getNumParams(), 'int');
    return new XML_RPC_Response($val);      
}
?>

Next, we'll create a stripped-down client with XML_RPC_Client to call this method with and without its optional parameter, and test whether or not the method call returned a fault.

If optional parameters are correctly supported by the version of XML_RPC_Server used, the server should not report any faults to the client.

<?php
require_once('XML/RPC.php');

$cli = new XML_RPC_Client('/demo-server.php', 'localhost');

$p1 = new XML_RPC_Value('', 'string');
$p2 = new XML_RPC_Value('', 'string');
$p3 = new XML_RPC_Value(false, 'boolean');

/**
  * Call getParamCount() with only its two required parameters.
  */
$msg = new XML_RPC_Message('getParamCount', array($p1, $p2));
$resp = $cli->send($msg);
if (!$resp) {
    die('Communication error: ' . $cli->errstr);
}
if ($resp->faultCode() ) {
    echo("Calling the method with its two required parameters succeeded.\n");
} else {
    echo("Calling the method with its two required parameters produced this fault:\n");
    echo('  Fault Code: ' . $resp->faultCode() . "\n");
    echo('  Fault Reason: ' . $resp->faultString() . "\n");
}

/**
  * Call getParamCount() with three parameters.
  */
$msg = new XML_RPC_Message('getParamCount', array($p1, $p2, $p3));
$resp = $cli->send($msg);
if (!$resp) {
    die('Communication error: ' . $cli->errstr);
}
if ($resp->faultCode() ) {
    echo("Calling the same method with three parameters succeeded.\n");
} else {
    echo("Calling the same method with three parameters produced this fault:\n");
    echo('  Fault Code: ' . $resp->faultCode() . "\n");
    echo('  Fault Reason: ' . $resp->faultString() . "\n");
}

?>

Expected Result

The expected result can be seen by running the reproduce code on a version of the XML_RPC package prior to 1.3.0RC2, such as 1.3.0RC1 or 1.2.2.

Content-type: text/html
X-Powered-By: PHP/5.0.3

Calling the method with its two required parameters succeeded.
Calling the same method with three parameters succeeded.

Both calls to the RPC method getParamCount() succeed and do not return a fault code. This shows that XML_RPC_Server::verifySignature() in versions prior to 1.3.0RC2 can properly handle a dispatch map where a method can have signatures that contain different counts of parameters. Of course, these versions of XML_RPC_Server also have the original bug reported in bug #4231.

The true expected result would be a version of XML_RPC_Server that both produces the output shown here and also addresses bug #4231. The fix that I originally submitted for bug #4231 meets these conditions.

Actual Result

The actual result when this same code is run on 1.3.0RC2 is that the second method call fails because verifySignature() does not properly evaluate all signatures in the dispatch map for the method.

Content-type: text/html
X-Powered-By: PHP/5.0.3

Calling the method with its two required parameters succeeded.
Calling the same method with three parameters produced this fault:
  Fault Code: 3
  Fault Reason: Incorrect parameters passed to method: Improper call to verifySignature()

Understanding the Bug in 1.3.0RC2

To understand how the patch implemented in 1.3.0RC2 broke support for optional parameters in XML_RPC_Server, we need to look at its version of the function verifySignature():

/**
  * @return array
  */
function verifySignature($in, $sig)
{
    for ($i = 0; $i < sizeof($sig); $i++) {
        // check each possible signature in turn
        $cursig = $sig[$i];
        if (sizeof($cursig) == $in->getNumParams() + 1) {
            $itsOK = 1;
            for ($n = 0; $n < $in->getNumParams(); $n++) {
                $p = $in->getParam($n);
                // print "\n";
                if ($p->kindOf() == 'scalar') {
                    $pt = $p->scalartyp();
                } else {
                    $pt = $p->kindOf();
                }
                // $n+1 as first type of sig is return type
                if ($pt != $cursig[$n+1]) {
                    $itsOK = 0;
                    $pno = $n+1;
                    $wanted = $cursig[$n+1];
                    $got = $pt;
                    break;
                }
            }
            if ($itsOK) {
                return array(1);
            }
        } else {
            return array(0, 'Improper call to verifySignature()');
        }
    }
    return array(0, "Wanted ${wanted}, got ${got} at param ${pno})");
}

In the code from 1.3.0RC2 above, the outermost for loop iterates through all of the signatures for a single method in the dispatch map. On each iteration, the outermost if statement checks that the count of parameters sent by the client matches the number in that particular signature.

The patch for 1.3.0RC2 added the else statement to this conditional. The result is that even if the dispatch map contains a signature for the method which matches the count of parameters that the client sent, the else statement will cause the function to immediately return upon encountering a signature with a different count of parameters in any earlier iteration through the signatures.

Fixing the Bug

To fix the bug, and have a version of verifySignature() that both addresses bug #4231 and supports optional parameters, use the function verifySignature() from 1.3.0RC1 and apply the patch that I suggested in bug #4231. This results in the following working code:

/**
 * @return array
 */
function verifySignature($in, $sig)
{
    for ($i = 0; $i < sizeof($sig); $i++) {
        // check each possible signature in turn
        $cursig = $sig[$i];
        if (sizeof($cursig) == $in->getNumParams() + 1) {
            $itsOK = 1;
            for ($n = 0; $n < $in->getNumParams(); $n++) {
                $p = $in->getParam($n);
                // print "\n";
                if ($p->kindOf() == 'scalar') {
                    $pt = $p->scalartyp();
                } else {
                    $pt = $p->kindOf();
                }
                // $n+1 as first type of sig is return type
                if ($pt != $cursig[$n+1]) {
                    $itsOK = 0;
                    $pno = $n+1;
                    $wanted = $cursig[$n+1];
                    $got = $pt;
                    break;
                }
            }
            if ($itsOK) {
                return array(1);
            }
        } 
    }
    if (isset($pno)) {
        return array(0, "Wanted ${wanted}, got ${got} at param ${pno})");
    } else {
        return array(0, 'Bad parameter count');
    }
}

There are some alternatives to this, such as adding a separate loop before this one to iterate through the possible signatures and returning an error if none of them matches the count of parameters supplied. Another alternative would be to replace the isset() with an additional flag. These variations waste extra cycles and memory, so I chose not to implement the fix this way.

Final Notes on the Error Message for Bug #4231

In closing, I'd like to make some additional comments on the text chosen for the error message in 1.3.0RC2. The error message I suggested in bug #4231 was "Bad parameter count", for the reason that the condition only occurred when the client supplied a parameter count that did not match any of the method's signatures. The message implemented in 1.3.0RC2 was instead called "Improper call to verifySignature()".

This error is returned to the remote XML-RPC client. This is confusing to the programmer of a client who is examining responses from the server because it does not explain how the client actually caused the error. The client didn't call verifySignature() improperly, it sent a bad count of parameters.

Additionally, one final concern with the chosen error message is that including our function name in an error message returned to the client is a security issue because it provides a means for a client to fingerprint any server that is derived from XML_RPC_Server. With any fix that included this error message, an attacker could make an XML-RPC method call with a bad parameter count, and check the returned faultString for the text "verifySignature()". This would immediately lead to the discovery that the server is running on XML_RPC_Server, and PHP itself, which could have otherwise been concealed. Although it's true that the client could test any of our error messages in this way, none of the other messages are so specific as to name our internal functions. It wouldn't be good practice to display an error message like this on a webpage, and I feel the same applies for a remote XML-RPC client.

I recommend returning the error as "Bad parameter count" as it is the most descriptive and perhaps has fewer security implications.

 


Last page update: 6 May 2005.