getPrivateKey() can disclose private key to error logs / cron emails

[weshooper]

We've recently had the contents of private keys emailed to us by cron when a connection fails, with the email contents looking something like the following:

PHP Notice:  Connection closed prematurely in /path/to/app/vendor/phpseclib/phpseclib/phpseclib/Net/SSH2.php on line 2886
PHP Notice:  Connection closed by server in /path/to/app/vendor/phpseclib/phpseclib/phpseclib/Net/SSH2.php on line 3205
PHP Notice:  Expected SSH_FXP_STATUS in /path/to/app/vendor/phpseclib/phpseclib/phpseclib/Net/SFTP.php on line 1960
PHP Notice:  Connection closed prematurely in /path/to/app/vendor/phpseclib/phpseclib/phpseclib/Net/SSH2.php on line 3396
PHP Warning:  is_file(): open_basedir restriction in effect. File(-----BEGIN RSA PRIVATE KEY-----
REDACTEDREDACTEDREDACTEDREDACTEDREDACTEDREDACTEDREDACTEDREDACTED in /path/to/app/vendor/league/flysystem-sftp/src/SftpAdapter.php on line 168
PHP Notice:  Cannot connect to hostname.com:22. Error 111. Connection refused in /path/to/app/vendor/phpseclib/phpseclib/phpseclib/Net/SSH2.php on line 1011
PHP Warning:  is_file(): open_basedir restriction in effect. File(-----BEGIN RSA PRIVATE KEY-----
REDACTEDREDACTEDREDACTEDREDACTEDREDACTEDREDACTEDREDACTEDREDACTED in /path/to/app/vendor/league/flysystem-sftp/src/SftpAdapter.php on line 168
PHP Notice:  Cannot connect to hostname.com:22. Error 111. Connection refused in /path/to/app/vendor/phpseclib/phpseclib/phpseclib/Net/SSH2.php on line 1011

Traced it down to the is_file() check in getPrivateKey() and wondered if there might be a better way to check?

Two options we came up with were:

1. check whether $this->privatekey starts with '-----' and only do the is_file check if it doesn't
2. have some kind of class flag to differentiate, rather than having to is_file

Any thoughts, or preference on approach?

[frankdejonge]

Good call, I'd be very happy with a PR which adds a file_exists check before the is_file call. This should prevent the warning. I can merge the PR and ask one of the other league members to tag the release. Could you ping me on Twitter if you have a PR up? I can then put the other things into motion.

[frankdejonge]

@weshooper I'm in Australia on holidays now and my laptop broke down :( that's why it's a bit difficult for me to write this fix. Mobile is all I have now.

[weshooper]

@frankdejonge thanks for the reply, sorry to hear about the laptop!

Looks like file_exists will still generate the same error:

$ php -d open_basedir=/tmp -r 'var_dump(file_exists("-------KEY"));'
PHP Warning:  file_exists(): open_basedir restriction in effect. File(-------KEY) is not within the allowed path(s): (/tmp) in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0
PHP   2. file_exists() Command line code:1
bool(false)

$ php -d open_basedir=/tmp -r 'var_dump(is_file("-------KEY"));'
PHP Warning:  is_file(): open_basedir restriction in effect. File(-------KEY) is not within the allowed path(s): (/tmp) in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0
PHP   2. is_file() Command line code:1
bool(false)

Checking the first few chars of $this->privatekey would seem to prevent the warning, along the lines of:

$ php -d open_basedir=/tmp -r 'var_dump("---" !== substr("-------KEY", 0, 3) && is_file("-------KEY"));'
bool(false)

If that approach would be ok, I'll open up a PR...

[frankdejonge]

That's ok then, please add a comment to the line so I and others know what it's for.

[weshooper]

@frankdejonge I've fixed this over in #42 :)