From b91e2203891ce7ef627a241ea05c3f11180fcfc1 Mon Sep 17 00:00:00 2001 From: Alexander Golubev Date: Sun, 21 Jan 2024 12:12:53 +0300 Subject: [PATCH] tdeioslave/sftp: overhaul publickey auth Several enhancements to public key authentication and some other stuff: - Fix passphrase entry for encrypted keys (was either hanging up or segfaulting) - Use scope guard idiom for cleanup calls for more reliable cleanup in case of errors - Add normal prompt for public key's passphrase entry dialog - Correctly differentiate passphrase to password when cached (yes they are getting cached regardless of keepPassword, at least for some duration of time) - Centrilize AuthInfo initialization and some rejig of it kbd-interactive authentification Signed-off-by: Alexander Golubev --- tdeioslave/sftp/tdeio_sftp.cpp | 255 +++++++++++++++++++++++---------- tdeioslave/sftp/tdeio_sftp.h | 19 ++- 2 files changed, 196 insertions(+), 78 deletions(-) diff --git a/tdeioslave/sftp/tdeio_sftp.cpp b/tdeioslave/sftp/tdeio_sftp.cpp index cc086d5aa..59428ea22 100644 --- a/tdeioslave/sftp/tdeio_sftp.cpp +++ b/tdeioslave/sftp/tdeio_sftp.cpp @@ -33,6 +33,8 @@ #include #include +#include + #include #include #include @@ -92,6 +94,50 @@ extern "C" } } +// Some helper functions/classes +namespace { + +// A quick and dirty scope guard implementation +class ExitGuard { +public: + template + ExitGuard(Callable && undo_func) : f(std::forward(undo_func)) {} + ExitGuard(ExitGuard && other) : f(std::move(other.f)) { + other.f = nullptr; + } + + ~ExitGuard() { + run(); + } + + void run() noexcept { + if(f) { f(); f = nullptr; } + } + + ExitGuard(const ExitGuard&) = delete; + void operator= (const ExitGuard&) = delete; + +private: + std::function f; +}; + +// A small helper to purge passwords. Paranoiac's note: this is not enough to guarantee the +// complete purge of the password and all its copy from memory (ioslaves are sending the passwords +// via dcop, so it's far beyond calling it "secure" in any way), but it's still better than nothing. +void purgeString(TQString &s) { + s.fill('\0'); + s.setLength(0); + s = TQString::null; +} + +// A helper class to cleanup password when it goes out of the scope +class PasswordPurger: public ExitGuard { +public: + PasswordPurger(TQString &pw) : ExitGuard( [&pw](){purgeString(pw);} ) {} +}; + +} /* namespace */ + // The callback function for libssh int auth_callback(const char *prompt, char *buf, size_t len, int echo, int verify, void *userdata) @@ -128,42 +174,73 @@ int sftpProtocol::auth_callback(const char *prompt, char *buf, size_t len, (void) echo; (void) verify; (void) userdata; + (void) prompt; + + Q_ASSERT(len>0); kdDebug(TDEIO_SFTP_DB) << "Entering public key authentication callback" << endl; - if(!pubKeyInfo) - { - pubKeyInfo = new TDEIO::AuthInfo; - } - else - { - // TODO: inform user about incorrect password - } + int rc=0; + mPubKeyAuthData.wasCalled = true; - pubKeyInfo->url.setProtocol("sftp"); - pubKeyInfo->url.setHost(mHost); - pubKeyInfo->url.setPort(mPort); - pubKeyInfo->url.setUser(mUsername); + AuthInfo pubKeyInfo = authInfo(); - pubKeyInfo->caption = i18n("SFTP Login"); - pubKeyInfo->comment = "sftp://" + mUsername + "@" + mHost; - pubKeyInfo->username = mUsername; - pubKeyInfo->readOnly = false; - pubKeyInfo->prompt = TQString::fromUtf8(prompt); - pubKeyInfo->keepPassword = false; // don't save passwords for public key, + pubKeyInfo.readOnly = false; + pubKeyInfo.keepPassword = false; // don't save passwords for public key, // that's the task of ssh-agent. + TQString errMsg; + TQString keyFile; +#if LIBSSH_VERSION_INT < SSH_VERSION_INT(0, 10, 0) + // no way to determine keyfile name on older libssh +#else + char *ssh_key_file = 0; + rc = ssh_userauth_publickey_auto_get_current_identity(mSession, &ssh_key_file); - if (!openPassDlg(*pubKeyInfo)) { - kdDebug(TDEIO_SFTP_DB) << "User canceled entry of public key password." << endl; - return -1; + if (rc == 0 && ssh_key_file && ssh_key_file[0]) { + keyFile = ssh_key_file; } + ssh_string_free_char(ssh_key_file); +#endif - strncpy(buf, pubKeyInfo->password.utf8().data(), len - 1); + bool firstTry = !mPubKeyAuthData.attemptedKeys.contains(keyFile); - pubKeyInfo->password.fill('x'); - pubKeyInfo->password = ""; + if (!firstTry) { + errMsg = i18n("Incorrect or invalid passphrase."); + } - return 0; + // libssh prompt is trash and we know we use this function only for publickey auth, so we'll give + // the user a descent prompt + if (!keyFile.isEmpty()) { + pubKeyInfo.prompt = i18n("Please enter the passphrase for next public key:\n%1").arg(keyFile); + } else { // Generally shouldn't happend but on older libssh + pubKeyInfo.prompt = i18n("Please enter the passphrase for your public key."); + } + + // We don't want to clobber with normal passwords in kpasswdserver's cache + pubKeyInfo.realmValue = "keyfile passphrase:" + keyFile; + + if (openPassDlg(pubKeyInfo, errMsg)) { + if (len < pubKeyInfo.password.utf8().length()+1) { + kdDebug(TDEIO_SFTP_DB) << "Insufficient buffer size for password: " << len + << " (" << pubKeyInfo.password.utf8().length()+1 << "needed)" << endl; + } + + strncpy(buf, pubKeyInfo.password.utf8().data(), len-1); + buf[len-1]=0; // Just to be on the safe side + + purgeString(pubKeyInfo.password); + } else { + kdDebug(TDEIO_SFTP_DB) << "User canceled entry of public key passphrase" << endl; + rc = -1; + mPubKeyAuthData.wasCanceled = true; + } + + // take a note that we already tried unlocking this keyfile + if(firstTry) { + mPubKeyAuthData.attemptedKeys.append(keyFile); + } + + return rc; } void sftpProtocol::log_callback(ssh_session session, int priority, @@ -210,6 +287,15 @@ int sftpProtocol::authenticateKeyboardInteractive(AuthInfo &info) { kdDebug(TDEIO_SFTP_DB) << "prompt=" << prompt << " echo=" << TQString::number(echo) << endl; TDEIO::AuthInfo infoKbdInt(info); + infoKbdInt.keepPassword = false; + + if (!name.isEmpty()) { + infoKbdInt.caption = TQString(i18n("SFTP Login") + " - " + name); + } + + // Those strings might or might not contain some sensitive information + PasswordPurger answerPurger{answer}; + PasswordPurger infoPurger{infoKbdInt.password}; if (!echo) { // ssh server requests us to ask user a question without displaying an answer. In normal @@ -230,7 +316,6 @@ int sftpProtocol::authenticateKeyboardInteractive(AuthInfo &info) { // If the server's request doesn't look like a password, keep the servers prompt and // don't bother saving it infoKbdInt.prompt = prompt; - infoKbdInt.keepPassword = false; } infoKbdInt.readOnly = true; // set username readonly @@ -242,6 +327,7 @@ int sftpProtocol::authenticateKeyboardInteractive(AuthInfo &info) { answer = infoKbdInt.password; if(isPassword) { info.password = infoKbdInt.password; // return the answer to the caller + info.setModified( true ); } } else { /* FIXME: Some reasonable action upon cancellation? <2024-01-10 Fat-Zer> */ @@ -253,22 +339,16 @@ int sftpProtocol::authenticateKeyboardInteractive(AuthInfo &info) { // no means of handle that correctly, so we will have to be creative with the password // dialog. TQString newPrompt; - infoKbdInt.comment = "sftp://" + infoKbdInt.username + "@" + mHost; - - if (!name.isEmpty()) { - infoKbdInt.caption = TQString(i18n("SFTP Login") + " - " + name); - } if (!instruction.isEmpty()) { newPrompt = instruction + "\n\n"; } - newPrompt.append(prompt + "\n\n"); - infoKbdInt.readOnly = false; - infoKbdInt.keepPassword = false; newPrompt.append(i18n("Use the username input field to answer this question.")); infoKbdInt.prompt = newPrompt; + infoKbdInt.readOnly = false; + if (openPassDlg(infoKbdInt)) { answer = infoKbdInt.username; kdDebug(TDEIO_SFTP_DB) << "Got the answer from the password dialog: " << answer << endl; @@ -289,6 +369,22 @@ int sftpProtocol::authenticateKeyboardInteractive(AuthInfo &info) { return err; } +TDEIO::AuthInfo sftpProtocol::authInfo() { + TDEIO::AuthInfo rv; + + rv.url.setProtocol("sftp"); + rv.url.setHost(mHost); + rv.url.setPort(mPort); + rv.url.setUser(mUsername); + + rv.caption = i18n("SFTP Login"); + rv.comment = "sftp://" + mHost + ':' + TQString::number(mPort); + rv.commentLabel = i18n("site:"); + rv.username = mUsername; + + return rv; +} + void sftpProtocol::reportError(const KURL &url, const int err) { kdDebug(TDEIO_SFTP_DB) << "url = " << url.url() << " - err=" << err << endl; @@ -553,18 +649,12 @@ void sftpProtocol::openConnection() { // Setup AuthInfo for use with password caching and the // password dialog box. - AuthInfo info; - - info.url.setProtocol("sftp"); - info.url.setHost(mHost); - info.url.setPort(mPort); - info.url.setUser(mUsername); - info.caption = i18n("SFTP Login"); - info.comment = "sftp://" + mHost + ':' + TQString::number(mPort); - info.commentLabel = i18n("site:"); - info.username = mUsername; + AuthInfo info = authInfo(); info.keepPassword = true; // make the "keep Password" check box visible to the user. + PasswordPurger pwPurger{mPassword}; + PasswordPurger infoPurger{info.password}; + // Check for cached authentication info if no password is specified... if (mPassword.isEmpty()) { kdDebug(TDEIO_SFTP_DB) << "checking cache: info.username = " << info.username @@ -631,8 +721,8 @@ void sftpProtocol::openConnection() { } // Set the username - if (!mUsername.isEmpty()) { - rc = ssh_options_set(mSession, SSH_OPTIONS_USER, mUsername.utf8().data()); + if (!info.username.isEmpty()) { + rc = ssh_options_set(mSession, SSH_OPTIONS_USER, info.username.utf8().data()); if (rc < 0) { error(TDEIO::ERR_OUT_OF_MEMORY, i18n("Could not set username.")); return; @@ -793,25 +883,54 @@ void sftpProtocol::openConnection() { bool firstTime = true; bool dlgResult; while (rc != SSH_AUTH_SUCCESS) { + /* FIXME: if there are problems with auth we are likely to stuck in this loop <2024-01-20 Fat-Zer> */ + // Try to authenticate with public key first - if (rc != SSH_AUTH_SUCCESS && (method & SSH_AUTH_METHOD_PUBLICKEY) && !mPassword) - { + if (rc != SSH_AUTH_SUCCESS && (method & SSH_AUTH_METHOD_PUBLICKEY) && !mPassword) { + // might mess up next login attempt if we won't clean it up + ExitGuard pubKeyInfoCleanser([this]() { + mPubKeyAuthData.wasCalled = 0; + mPubKeyAuthData.wasCanceled = 0; + mPubKeyAuthData.attemptedKeys.clear(); + }); + kdDebug(TDEIO_SFTP_DB) << "Trying to authenticate with public key" << endl; - for(;;) + bool keepTryingPasskey=true; + while(keepTryingPasskey) { + mPubKeyAuthData.wasCalled = 0; rc = ssh_userauth_publickey_auto(mSession, nullptr, nullptr); - if (rc == SSH_AUTH_ERROR) - { - clearPubKeyAuthInfo(); - closeConnection(); - error(TDEIO::ERR_COULD_NOT_LOGIN, i18n("Authentication failed (method: %1).") - .arg(i18n("public key"))); - return; - } - if (rc == SSH_AUTH_DENIED || !pubKeyInfo || !pubKeyInfo->isModified()) - { - clearPubKeyAuthInfo(); - break; + + kdDebug(TDEIO_SFTP_DB) << "ssh_userauth_publickey_auto returned rc=" << rc + << " ssh_err=" << ssh_get_error_code(mSession) + << " (" << ssh_get_error(mSession) << ")" << endl; + + switch (rc) { + case SSH_AUTH_SUCCESS: + case SSH_AUTH_PARTIAL: + keepTryingPasskey=false; + break; + case SSH_AUTH_AGAIN: + // Returned in case of some errors like if server hangs up or there were too many auth attempts + case SSH_AUTH_ERROR: + closeConnection(); + /* FIXME: Use scope guard to close connection <2024-01-20 Fat-Zer> */ + error(TDEIO::ERR_COULD_NOT_LOGIN, i18n("Authentication failed (method: %1).") + .arg(i18n("public key"))); + /* FIXME: add some additional info from ssh_get_error() if available <2024-01-20 Fat-Zer> */ + return; + + case SSH_AUTH_DENIED: + if (!mPubKeyAuthData.wasCalled) { + kdDebug(TDEIO_SFTP_DB) << "Passkey auth denied because it has no matching key" << endl; + keepTryingPasskey = false; + } else if (mPubKeyAuthData.wasCanceled) { + kdDebug(TDEIO_SFTP_DB) << "Passkey auth denied because user canceled" << endl; + keepTryingPasskey = false; + } else { + kdDebug(TDEIO_SFTP_DB) << "User entered wrong passphrase for the key" << endl; + } + break; } } } @@ -923,11 +1042,6 @@ void sftpProtocol::openConnection() { mConnected = true; connected(); - mPassword.fill('x'); - mPassword = ""; - info.password.fill('x'); - info.password = ""; - return; } @@ -1875,12 +1989,3 @@ void sftpProtocol::slave_status() { kdDebug(TDEIO_SFTP_DB) << "connected to " << mHost << "?: " << mConnected << endl; slaveStatus((mConnected ? mHost : TQString()), mConnected); } - -void sftpProtocol::clearPubKeyAuthInfo() -{ - if (!pubKeyInfo) - { - delete pubKeyInfo; - pubKeyInfo = nullptr; - } -} diff --git a/tdeioslave/sftp/tdeio_sftp.h b/tdeioslave/sftp/tdeio_sftp.h index 8e46d815c..2ad069ea7 100644 --- a/tdeioslave/sftp/tdeio_sftp.h +++ b/tdeioslave/sftp/tdeio_sftp.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -138,12 +139,24 @@ private: // Private variables // TQString text; //}; - TDEIO::AuthInfo *pubKeyInfo; + /** Some data needed to interact with auth_callback() */ + struct { + /** true if callback was called */ + bool wasCalled; + /** true if user canceled password entry dialog */ + bool wasCanceled; + /** List of keys user was already prompted to enter the passphrase for. + * Note: Under most sane circumstances the list shouldn't go beyond size=2, + * so no fancy containers here + */ + TQStringList attemptedKeys; + } mPubKeyAuthData; private: // private methods - int authenticateKeyboardInteractive(TDEIO::AuthInfo &info); - void clearPubKeyAuthInfo(); + + /** A small helper function to construct auth info skeleton for the protocol */ + TDEIO::AuthInfo authInfo(); void reportError(const KURL &url, const int err);