[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-release-team
Subject: Request for deprecation of KDESu::SshProcess and removal of
From: "Friedrich W. H. Kossebau" <kossebau () kde ! org>
Date: 2009-12-18 20:02:47
Message-ID: 200912182102.47631.kossebau () kde ! org
[Download RAW message or body]
Hi,
KDESu::SshProcess (in kdelibs) and the commandline shell for it, kdessh (in
kdeutils) are horribly broken (as in: do not work and may be insecure) and (at
least for me) seem not easy to be fixed.
I guess most of you do not even know these things exist, so:
kdessh is a wrapper to ssh and, instead of executing the original remote
command, first (via KDESu::SshProcess) fires up kdesu_stub on the remote
computer to setup the environment variables as needed for a better integration
into the local session, only then executes the original command.
Additionally it also caches the passwords (but does not use KWallet).
It is not working at all currently, as this commit
"Move kdesu_stub to libexec"
http://websvn.kde.org/?view=revision&revision=666108
moved kdesu_stub out of the $PATH, so the ssh server will not find it.
Is there a chance somebody remembers why it was moved to ? And not
perhaps renamed kdesu_stub to kdesu_stub4? Or just have it conflict with the
KDE 3 version, like e.g. KWrite has a conflict, too.
The class KDESu::SshProcess/StubProcess itself has a wild mixtures of
undocumented return values, seems to forget about child processes in some
conditions, has password strings in unsecured memory, does not reuse the
running ssh connection after testing for password needs, does not do a proper
check for false passwords and whatelse.
From lxr.kde.org it seems kdessh is the only user of KDESu::SshProcess,
besides kvpnc in playground/network (no idea about its state). And with zero
reports about this problem on b.k.o kdessh also seems without any users.
As noone has ever had a closer look at kdessh until now (starting kdessh did
nothing, including no obvious harm, so it got ignored), including the kdeutils
coordinator (who is writing here) it was only now decided to move kdessh from
kdeutils to tags/unmaintained after the Beta2 release. Sorry for any
inconvenience.
Additionally the class KDESu::SshProcess in kdelibs should be marked as
deprecated. Perhaps it could be even removed, as I do not think anyone is
using this class/these symbols?
Also kdesu_stub does no longer needed to be built and installed, as long as it
ends in lib/kde4/libexec.
Still I think such a utility for the integrated execution of remote programs
is nice to have. But with X11-forwarding-enabled ssh client/servers and ssh-
agent/-add it should perhaps have another approach, including integration of
KWallet. I also wonder how much remote X clients can and should be integrated
in the local session at all, including the Session D-Bus?
Cheers
Friedrich
PS: In case you are interested find attached two patches which made kdessh at
least working again, until I found SshProcess too broken to continue to clean
up for all possible conditions. Patches do s/magic numbers/enums/g, renames
kdesu_stub to kdesu_stub4 and installes it to bin/ again, code style cleanup,
more caring for child processes.
--
Okteta - KDE Hex Editor - http://utils.kde.org/projects/okteta
["attemptToFixSshProcess.patch" (text/x-patch)]
Index: kdelibs/kdesu/ssh.h
===================================================================
--- kdelibs/kdesu/ssh.h (Revision 1063492)
+++ kdelibs/kdesu/ssh.h (Arbeitskopie)
@@ -29,8 +29,11 @@
const QByteArray &command = QByteArray());
~SshProcess();
- enum Errors { SshNotFound=1, SshNeedsPassword, SshIncorrectPassword };
+ enum Errors { NoError=0, SshNeedsNoPassword=0, SshNotFound=1, SshNeedsPassword, \
SshIncorrectPassword }; + enum ExtendedErrors { GeneralError=-1 };
+ enum ExecType { NormalExec=0, PasswordCorrectCheck=1, PasswordNeededCheck=2 };
+
/**
* Sets the target host.
*/
@@ -59,7 +62,7 @@
/**
* Executes the command.
*/
- int exec(const char *password, int check=0);
+ int exec(const char *password, int check=NormalExec);
QByteArray prompt() const;
QByteArray error() const;
@@ -69,6 +72,7 @@
virtual QByteArray displayAuth();
private:
+ enum SshResult { SshProtocolError=-1, SshNoError=0, StoppedAtPasswordPrompt=1, \
SshOtherError=255 }; int ConverseSsh(const char *password, int check);
protected:
Index: kdelibs/kdesu/stub.cpp
===================================================================
--- kdelibs/kdesu/stub.cpp (Revision 1063492)
+++ kdelibs/kdesu/stub.cpp (Arbeitskopie)
@@ -105,95 +105,101 @@
int StubProcess::ConverseStub(int check)
{
- QByteArray line, tmp;
-
+ // stub header
while (1)
{
- line = readLine();
- if (line.isNull())
- return -1;
+ QByteArray line = readLine();
+ if (line.isNull()) {
+ return StubProtocolError;
+ }
- if (line == "kdesu_stub")
- {
- // This makes parsing a lot easier.
- enableLocalEcho(false);
- if (check) writeLine("stop");
- else writeLine("ok");
- break;
- }
+ if (line == "kdesu_stub") {
+ // This makes parsing a lot easier.
+ enableLocalEcho(false);
+ writeLine(check ? "stop" : "ok");
+ break;
+ }
}
+ // stub commands
+ StubResult stubResult;
+
while (1)
{
- line = readLine();
- if (line.isNull())
- return -1;
+ QByteArray line = readLine();
+ if (line.isNull()) {
+ stubResult = StubProtocolError;
+ break;
+ }
if (line == "display") {
- writeLine(display());
- } else if (line == "display_auth") {
+ writeLine(display());
+ } else if (line == "display_auth") {
#ifdef Q_WS_X11
- writeLine(displayAuth());
+ writeLine(displayAuth());
#else
- writeLine("");
+ writeLine(QByteArray());
#endif
- } else if (line == "command") {
- writeLine(m_Command);
- } else if (line == "path") {
- QByteArray path = qgetenv("PATH");
- if (!path.isEmpty() && path[0] == ':')
+ } else if (line == "command") {
+ writeLine(m_Command);
+ } else if (line == "path") {
+ QByteArray path = qgetenv("PATH");
+ if (!path.isEmpty() && path[0] == ':') {
path = path.mid(1);
- if (m_User == "root") {
- if (!path.isEmpty())
- path = "/sbin:/bin:/usr/sbin:/usr/bin:" + path;
- else
- path = "/sbin:/bin:/usr/sbin:/usr/bin";
}
- writeLine(path);
- } else if (line == "user") {
- writeLine(m_User);
- } else if (line == "priority") {
- tmp.setNum(m_Priority);
- writeLine(tmp);
- } else if (line == "scheduler") {
- if (m_Scheduler == SchedRealtime) writeLine("realtime");
- else writeLine("normal");
- } else if (line == "xwindows_only") {
- if (m_bXOnly) writeLine("no");
- else writeLine("yes");
- } else if (line == "app_startup_id") {
- QList<QByteArray> env = environment();
- QByteArray tmp;
- for(int i = 0; i < env.count(); ++i)
- {
- const char startup_env[] = "DESKTOP_STARTUP_ID=";
- if (env.at(i).startsWith(startup_env))
- tmp = env.at(i).mid(sizeof(startup_env) - 1);
- }
- if( tmp.isEmpty())
- tmp = "0"; // krazy:exclude=doublequote_chars
- writeLine(tmp);
- } else if (line == "app_start_pid") { // obsolete
- // Force the pid_t returned from getpid() into
- // something QByteArray understands; avoids ambiguity
- // between short and unsigned short in particular.
- tmp.setNum((PIDType<sizeof(pid_t)>::PID_t)(getpid()));
- writeLine(tmp);
- } else if (line == "environment") { // additional env vars
- QList<QByteArray> env = environment();
- for (int i = 0; i < env.count(); ++i)
- writeLine(env.at(i));
- writeLine( "" );
- } else if (line == "end") {
- return 0;
- } else
- {
- kWarning(kdesuDebugArea()) << k_lineinfo << "Unknown request:" << line;
- return 1;
- }
+ if (m_User == "root") {
+ if (!path.isEmpty())
+ path = "/sbin:/bin:/usr/sbin:/usr/bin:" + path;
+ else
+ path = "/sbin:/bin:/usr/sbin:/usr/bin";
+ }
+ writeLine(path);
+ } else if (line == "user") {
+ writeLine(m_User);
+ } else if (line == "priority") {
+ writeLine(QByteArray::number(m_Priority));
+ } else if (line == "scheduler") {
+ writeLine((m_Scheduler==SchedRealtime) ? "realtime" : "normal");
+ } else if (line == "xwindows_only") {
+ writeLine(m_bXOnly ? "no" : "yes");
+ } else if (line == "app_startup_id") {
+ const QList<QByteArray> env = environment();
+ QByteArray appStartupId;
+ const char startup_env[] = "DESKTOP_STARTUP_ID=";
+ const int idOffset = sizeof(startup_env) - 1;
+ foreach(const QByteArray& envVar, env)
+ {
+ if (envVar.startsWith(startup_env)) {
+ appStartupId = envVar.mid(idOffset);
+ break;
+ }
+ }
+ if (appStartupId.isEmpty())
+ appStartupId = "0"; // krazy:exclude=doublequote_chars
+ writeLine(appStartupId);
+ } else if (line == "app_start_pid") { // obsolete
+ // Force the pid_t returned from getpid() into
+ // something QByteArray understands; avoids ambiguity
+ // between short and unsigned short in particular.
+ QByteArray startPid = \
QByteArray::number((PIDType<sizeof(pid_t)>::PID_t)( getpid() )); + \
writeLine(startPid); + } else if (line == "environment") { // additional env \
vars + const QList<QByteArray> env = environment();
+ foreach(const QByteArray& envVar, env) {
+ writeLine(envVar);
+ }
+ writeLine(QByteArray());
+ } else if (line == "end") {
+ stubResult = StubNoError;
+ break;
+ } else {
+ kWarning(kdesuDebugArea()) << k_lineinfo << "Unknown request:" << line;
+ stubResult = StubUnknownCommand;
+ break;
+ }
}
- return 0;
+ return stubResult;
}
Index: kdelibs/kdesu/stub.h
===================================================================
--- kdelibs/kdesu/stub.h (Revision 1063492)
+++ kdelibs/kdesu/stub.h (Arbeitskopie)
@@ -19,7 +19,7 @@
#include <kdesu/kdesu_export.h>
namespace KDESu {
-
+
namespace KDESuPrivate { class KCookie; }
@@ -71,8 +71,16 @@
protected:
/**
- * Exchange all parameters with kdesu_stub.
+ * .
*/
+ enum StubResult { StubProtocolError=-1, StubNoError=0, StubUnknownCommand=1, \
StubOtherError=255 }; +
+ /**
+ * Exchange all parameters with kdesu_stub, e.g. the authentication tokens \
(X11). + * @param check Flag to indicate if this is a normal execution (=Zero)
+ * or just a check (other values than Zero).
+ * @return a value from StubResult
+ */
int ConverseStub(int check);
/**
Index: kdelibs/kdesu/CMakeLists.txt
===================================================================
--- kdelibs/kdesu/CMakeLists.txt (Revision 1063492)
+++ kdelibs/kdesu/CMakeLists.txt (Arbeitskopie)
@@ -37,9 +37,9 @@
set(kdesu_stub_SRCS kdesu_stub.c )
-kde4_add_executable(kdesu_stub NOGUI ${kdesu_stub_SRCS})
+kde4_add_executable(kdesu_stub4 NOGUI ${kdesu_stub_SRCS})
-install(TARGETS kdesu_stub DESTINATION ${LIBEXEC_INSTALL_DIR} )
+install(TARGETS kdesu_stub4 ${INSTALL_TARGETS_DEFAULT_ARGS} )
########### install files ###############
Index: kdelibs/kdesu/ssh.cpp
===================================================================
--- kdelibs/kdesu/ssh.cpp (Revision 1063492)
+++ kdelibs/kdesu/ssh.cpp (Arbeitskopie)
@@ -45,7 +45,7 @@
public:
SshProcessPrivate(const QByteArray &host)
: m_Host( host )
- , m_Stub( "kdesu_stub" )
+ , m_Stub( "kdesu_stub4" )
{}
QByteArray m_Prompt;
QByteArray m_Host;
@@ -82,74 +82,74 @@
int SshProcess::checkInstall(const char *password)
{
- return exec(password, 1);
+ return exec(password, PasswordCorrectCheck);
}
int SshProcess::checkNeedPassword()
{
- return exec(0L, 2);
+ return exec((const char*)0, PasswordNeededCheck);
}
int SshProcess::exec(const char *password, int check)
{
- if (check)
- setTerminal(true);
+ if (check != NormalExec)
+ setTerminal(true);
QList<QByteArray> args;
args += "-l"; args += m_User;
args += "-o"; args += "StrictHostKeyChecking=no";
args += d->m_Host; args += d->m_Stub;
- if (StubProcess::exec("ssh", args) < 0)
- {
- return check ? SshNotFound : -1;
+ if (StubProcess::exec("ssh", args) < 0) {
+ return (check != NormalExec) ? (int)SshNotFound : (int)GeneralError;
}
- int ret = ConverseSsh(password, check);
- if (ret < 0)
- {
- if (!check)
+ const int sshResult = ConverseSsh(password, check);
+kDebug()<<"ssh executing..."<<sshResult;
+ if (sshResult == SshProtocolError) {
+ if (check == NormalExec)
kError(kdesuDebugArea()) << k_lineinfo << "Conversation with ssh \
failed.";
- return ret;
+ return GeneralError;
}
- if (check == 2)
- {
- if (ret == 1)
- {
- kill(m_Pid, SIGTERM);
- waitForChild();
- }
- return ret;
+ if (check == PasswordNeededCheck) {
+ kill(m_Pid, SIGTERM);
+ waitForChild();
+
+ return (sshResult == StoppedAtPasswordPrompt) ? SshNeedsPassword : \
SshNeedsNoPassword; }
- if (m_bErase && password)
+ if (m_bErase && password) {
memset(const_cast<char *>(password), 0, qstrlen(password));
+ }
- ret = ConverseStub(check);
- if (ret < 0)
- {
- if (!check)
+ const int stubResult = ConverseStub(check);
+kDebug()<<"stub conversing..."<<stubResult;
+ if (stubResult == StubProtocolError) {
+ if (check == NormalExec)
kError(kdesuDebugArea()) << k_lineinfo << "Conversation with kdesu_stub \
failed.";
- return ret;
+ return GeneralError;
}
- else if (ret == 1)
- {
+ // TODO: unknown command is not a good way to check for incorrect password,
+ // could also be a broken kdesu_stub
+ // exit code of ssh (255) would be a better way
+ else if (stubResult == StubUnknownCommand) {
kill(m_Pid, SIGTERM);
waitForChild();
- ret = SshIncorrectPassword;
+ return SshIncorrectPassword;
}
- if (check == 1)
- {
+ if (check == PasswordCorrectCheck) {
waitForChild();
- return 0;
+ return NoError;
}
setExitString("Waiting for forwarded connections to terminate");
- ret = waitForChild();
- return ret;
+ // TODO: results of waitForChild are not defined
+ int result = waitForChild();
+
+ return result;
}
QByteArray SshProcess::prompt() const
@@ -178,68 +178,68 @@
int SshProcess::ConverseSsh(const char *password, int check)
{
- unsigned i, j, colon;
+ int result;
- QByteArray line;
- int state = 0;
+ enum ConverseState { AwaitingPasswordPrompt, PasswordGiven };
+ ConverseState state = AwaitingPasswordPrompt;
- while (state < 2)
+ while (true)
{
- line = readLine();
- const uint len = line.length();
- if (line.isNull())
- return -1;
+ const QByteArray line = readLine();
+kDebug()<<line;
+ if (line.isNull()) {
+ result = SshProtocolError;
+ break;
+ }
- switch (state) {
- case 0:
+ if (state== AwaitingPasswordPrompt) {
// Check for "kdesu_stub" header.
if (line == "kdesu_stub")
{
unreadLine(line);
- return 0;
+ result = NoError;
+ break;
}
// Match "Password: " with the regex ^[^:]+:[\w]*$.
- for (i=0,j=0,colon=0; i<len; i++)
- {
- if (line[i] == ':')
- {
- j = i; colon++;
+ uint j = 0;
+ uint colon = 0;
+ const uint len = line.length();
+ for (unsigned int i=0; i<len; i++) {
+ if (line[i] == ':') {
+ j = i;
+ ++colon;
continue;
}
- if (!isspace(line[i]))
- j++;
+ if (!isspace(line[i])) {
+ ++j;
+ }
}
- if ((colon == 1) && (line[j] == ':'))
- {
- if (check == 2)
- {
+ if ((colon == 1) && (line[j] == ':')) {
+ if (check == PasswordNeededCheck) {
d->m_Prompt = line;
- return SshNeedsPassword;
+ result = StoppedAtPasswordPrompt;
+ break;
}
WaitSlave();
write(fd(), password, strlen(password));
write(fd(), "\n", 1);
- state++;
- break;
+ state = PasswordGiven;
+ continue;
}
// Warning/error message.
d->m_Error += line; d->m_Error += '\n';
- if (m_bTerminal)
+ if (m_bTerminal) {
fprintf(stderr, "ssh: %s\n", line.constData());
+ }
+ } else if (state == PasswordGiven) {
+ result = line.isEmpty() ? SshNoError : SshProtocolError;
break;
-
- case 1:
- if (line.isEmpty())
- {
- state++;
- break;
- }
- return -1;
}
}
- return 0;
+
+ return result;
}
["adaptKdesshToSshProcessFix.patch" (text/x-patch)]
Index: kdeutils/kdessh/kdessh.cpp
===================================================================
--- kdeutils/kdessh/kdessh.cpp (Revision 1063316)
+++ kdeutils/kdessh/kdessh.cpp (Arbeitskopie)
@@ -71,7 +71,7 @@
options.add("+host", ki18n("Specifies the remote host"));
options.add("+command", ki18n("The command to run"));
options.add("u <user>", ki18n("Specifies the target uid"));
- options.add("s <path>", ki18n("Specify remote stub location"), "kdesu_stub");
+ options.add("s <path>", ki18n("Specify remote stub location"), "kdesu_stub4");
options.add("n", ki18n("Do not keep password"));
options.add("q", ki18n("Stop the daemon (forgets all passwords)"));
options.add("t", ki18n("Enable terminal output (no password keeping)"));
@@ -184,7 +184,7 @@
}
QByteArray password;
- if (needpw != 0)
+ if (needpw == SshProcess::SshNeedsPassword)
{
KDEsshDialog *dlg = new KDEsshDialog(host, user, stub,
proc.prompt(), keep && !terminal);
_______________________________________________
release-team mailing list
release-team@kde.org
https://mail.kde.org/mailman/listinfo/release-team
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic