Update public key authentication to avoid data corruption potential

Description

An interaction within ssh2_auth_pubkey_file has proven problematic under the PHP7 zend_string paradigm, particularly in the freeing of the parsed parameters pubkey and privkey that occurs when providing pathnames preceding with ~/.

    /* Explode '~/paths' stopgap fix because libssh2 does not accept tilde for homedir
      This should be ifdef'ed when a fix is available to support older libssh2 versions*/
    pws = getpwuid(geteuid());
    if (pubkey_len >= 2 && *pubkey == '~' && *(pubkey+1) == '/') {
        newpath = emalloc(strlen(pws->pw_dir) + strlen(pubkey));
        strcpy(newpath, pws->pw_dir);
        strcat(newpath, pubkey+1);
        efree(pubkey);
        pubkey = newpath;
    }
    if (privkey_len >= 2 && *privkey == '~' && *(privkey+1) == '/') {
        newpath = emalloc(strlen(pws->pw_dir) + strlen(privkey));
        strcpy(newpath, pws->pw_dir);
        strcat(newpath, privkey+1);
        efree(privkey);
        privkey = newpath;
    }

The following script be can used to produce a segmenation fault as a result of the above logic.

$publicKey = '~/.ssh/id_rsa.pub';
$privateKey = '~/.ssh/id_rsa';
$connection = ssh2_connect('127.0.0.1', 22222);
if (!is_resource($connection)) {
        die("Failed to connect");
}
if (!ssh2_auth_pubkey_file($connection, 'user', $publicKey, $privateKey)) {
    die("Failed to authenticate with public key");
}
echo "Authenticated...\n"; // somehow relevant to exhibit error, though data corruption is present regardless

Printing the backtrace upon encountering the error along with the interned strings hash structure represented by ht=0x555555c56fc0 yields the following, where it is apparent some corruption to entries at 0x7ffff3804080 and 0x7ffff38040c0 has occurred.

Program received signal SIGBUS, Bus error.
0x00007ffff49a144e in _int_free (av=0x101be4d0101be48, p=0x7ffff386b4f0, have_lock=0) at malloc.c:3940
3940          (void)mutex_lock(&av->mutex);
(gdb) bt
#0  0x00007ffff49a144e in _int_free (av=0x101be4d0101be48, p=0x7ffff386b4f0, have_lock=0) at malloc.c:3940
#1  0x000055555583c88d in zend_hash_destroy (ht=0x555555c56fc0 <compiler_globals+416>) at /usr/src/debug/php-7.2.9/Zend/zend_hash.c:1234
#2  0x0000555555856c03 in zend_interned_strings_deactivate () at /usr/src/debug/php-7.2.9/Zend/zend_string.c:205
#3  0x00005555557c5b32 in php_request_shutdown (dummy=dummy@entry=0x0) at /usr/src/debug/php-7.2.9/main/main.c:1925
#4  0x00005555558d52d8 in do_cli (argc=2, argv=0x555555c5bc10) at /usr/src/debug/php-7.2.9/sapi/cli/php_cli.c:1178
#5  0x000055555563ee5e in main (argc=2, argv=0x555555c5bc10) at /usr/src/debug/php-7.2.9/sapi/cli/php_cli.c:1404
(gdb) print_ht 0x555555c56fc0
Hash(12)[0x555555c56fc0]: {
  [0] STDIN => [0x7ffff3804000] (refcount=0) string: STDIN
  [1] STDOUT => [0x7ffff3804020] (refcount=1440566752) string: STDOUT
  [2] STDERR => [0x7ffff3804040] (refcount=1440566800) string: STDERR
  [3] publicKey => [0x7ffff3804060] (refcount=-209275368) string: publicKey
  [4] �R��� => [0x7ffff3804080] (refcount=-209301072) string: �R���
  [5] privateKey => [0x7ffff38040a0] (refcount=-209275768) string: privateKey
  [6] ��� => [0x7ffff38040c0] (refcount=-209275728) string: ���
  [7] connection => [0x7ffff38040e0] (refcount=1747940472) string: connection
  [8] 127.0.0.1 => [0x7ffff3804100] (refcount=0) string: 127.0.0.1
  [9] Failed to connect => [0x7ffff3804120] (refcount=-291176710) string: Failed to connect
  [10] Failed to authenticate with public key => [0x7ffff3804140] (refcount=0) string: Failed to authenticate with public key
  [11] Authenticated...
 => [0x7ffff3804160] (refcount=0) string: Authenticated...

}

Compare the above results with the following, where a preceding ~/ is not present in publicKey and privateKeyvariables in the error reproduction script posted above.

(gdb) print_ht 0x555555c56fc0
Hash(12)[0x555555c56fc0]: {
  [0] STDIN => [0x7ffff3804000] (refcount=1) string: STDIN
  [1] STDOUT => [0x7ffff3804020] (refcount=1) string: STDOUT
  [2] STDERR => [0x7ffff3804040] (refcount=1) string: STDERR
  [3] publicKey => [0x7ffff3804060] (refcount=0) string: publicKey
  [4] /home/vagrant/.ssh/id_rsa.pub => [0x7ffff3804080] (refcount=0) string: /home/vagrant/.ssh/id_rsa.pub
  [5] privateKey => [0x7ffff38040a0] (refcount=0) string: privateKey
  [6] /home/vagrant/.ssh/id_rsa => [0x7ffff38040c0] (refcount=0) string: /home/vagrant/.ssh/id_rsa
  [7] connection => [0x7ffff38040e0] (refcount=0) string: connection
  [8] 127.0.0.1 => [0x7ffff3804100] (refcount=0) string: 127.0.0.1
  [9] Failed to connect => [0x7ffff3804120] (refcount=0) string: Failed to connect
  [10] Failed to authenticate with public key => [0x7ffff3804140] (refcount=0) string: Failed to authenticate with public key
  [11] Authenticated...
 => [0x7ffff3804160] (refcount=0) string: Authenticated...

}

This difference highlights the potential for data corruption when freeing string variables which have been interned by PHP as a result of passing them as parameters to extension exposures.

Changes

This patch converts the parameters passed to the ssh2_auth_pubkey_file extension exposure to the zend_string datatype, replacing references to the string value and length with the ZSTR_VAL and ZSTR_LEN macros, respectively. Support for replacement of preceding ~/ characters in the key path parameters has been maintained but with use of zend_string_release instead of efree, which is safe to call in the presence of interned strings.

php / pecl-networking-ssh2

Update public key authentication to avoid data corruption potential #41

Description

Changes

References