[PATCH 0/3] Handle the case when KernelShark is started as Root

[PATCH 0/3] Handle the case when KernelShark is started as Root

[Yordan Karadzhov (VMware)]

A patch set implementing the improvements suggested here:

https://bugzilla.kernel.org/show_bug.cgi?id=204475

Yordan Karadzhov (VMware) (3):
  kernel-shark: Show warning message when running as Root
  kernel-shark: Don't use pkexec when running as Root
  kernel-shark: Use standart error message in KsMainWindow::_record()

 kernel-shark/src/KsMainWindow.cpp | 73 ++++++++++++++++++++++++-------
 kernel-shark/src/KsMainWindow.hpp |  2 +
 2 files changed, 60 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH 1/3] kernel-shark: Show warning message when running as Root

[Yordan Karadzhov (VMware)]

Running the KernelShark GUI with Root privileges is not recommended due
to security reasons. The user will be allowed to continue on its own risk.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204475
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsMainWindow.cpp | 28 ++++++++++++++++++++++++++++
 kernel-shark/src/KsMainWindow.hpp |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 6439265..2560bf8 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -76,6 +76,9 @@ KsMainWindow::KsMainWindow(QWidget *parent)
 	_createMenus();
 	_initCapture();
 
+	if (geteuid() == 0)
+		_rootWarning();
+
 	_splitter.addWidget(&_graph);
 	_splitter.addWidget(&_view);
 	setCentralWidget(&_splitter);
@@ -1271,3 +1274,28 @@ void KsMainWindow::_deselectB()
 	_mState.updateLabels();
 	_graph.glPtr()->model()->update();
 }
+
+void KsMainWindow::_rootWarning()
+{
+	QString cbFlag("noRootWorn");
+
+	if (_settings.value(cbFlag).toBool())
+		return;
+
+	QMessageBox warn;
+	warn.setText("KernelShark will have Root privileges.");
+	warn.setInformativeText("Continue on your one risk.");
+	warn.setIcon(QMessageBox::Warning);
+	warn.setStandardButtons(QMessageBox::Close);
+
+	QCheckBox cb("Don't show this message again.");
+
+	auto lamCbChec = [&] (int state) {
+		if (state)
+			_settings.setValue(cbFlag, true);
+	};
+
+	connect(&cb, &QCheckBox::stateChanged, lamCbChec);
+	warn.setCheckBox(&cb);
+	warn.exec();
+}
diff --git a/kernel-shark/src/KsMainWindow.hpp b/kernel-shark/src/KsMainWindow.hpp
index 62e66a0..4a7b8ab 100644
--- a/kernel-shark/src/KsMainWindow.hpp
+++ b/kernel-shark/src/KsMainWindow.hpp
@@ -238,6 +238,8 @@ private:
 
 	void _deselectB();
 
+	void _rootWarning();
+
 	void _updateFilterMenu();
 
 	void _filterSyncCBoxUpdate(kshark_context *kshark_ctx);
-- 
2.20.1

[PATCH 2/3] kernel-shark: Don't use pkexec when running as Root

[Yordan Karadzhov (VMware)]

If KernelShark GUI has been started as Root we do not need to use
"pkexec" when starting the Record dialog. Note that the actual place
where "pkexec" gets used is in the script "kshark-su-record".

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsMainWindow.cpp | 47 +++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 2560bf8..e9c6d54 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -883,23 +883,26 @@ void KsMainWindow::_pluginAdd()
 
 void KsMainWindow::_record()
 {
-#ifndef DO_AS_ROOT
+	bool canDoAsRoot(false);
 
-	QErrorMessage *em = new QErrorMessage(this);
-	QString message;
-
-	message = "Record is currently not supported.";
-	message += " Install \"pkexec\" and then do:<br>";
-	message += " cd build <br> sudo ./cmake_uninstall.sh <br>";
-	message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
-	message += " sudo make install";
+#ifdef DO_AS_ROOT
+	canDoAsRoot = true;
+#endif
 
-	em->showMessage(message);
-	qCritical() << "ERROR: " << message;
+	if (geteuid() && !canDoAsRoot) {
+		QErrorMessage *em = new QErrorMessage(this);
+		QString message;
 
-	return;
+		message = "Record is currently not supported.";
+		message += " Install \"pkexec\" and then do:<br>";
+		message += " cd build <br> sudo ./cmake_uninstall.sh <br>";
+		message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
+		message += " sudo make install";
 
-#endif
+		em->showMessage(message);
+		qCritical() << "ERROR: " << message;
+		return;
+	}
 
 	_capture.start();
 }
@@ -1134,9 +1137,24 @@ void KsMainWindow::loadSession(const QString &fileName)
 
 void KsMainWindow::_initCapture()
 {
+	bool canDoAsRoot(false);
+
 #ifdef DO_AS_ROOT
+	canDoAsRoot = true;
+#endif
+
+	if (geteuid() && !canDoAsRoot)
+		return;
 
-	_capture.setProgram("kshark-su-record");
+	if (geteuid()) {
+		_capture.setProgram("kshark-su-record");
+	} else {
+		QStringList argv;
+
+		_capture.setProgram("kshark-record");
+		argv << QString("-o ") + QDir::homePath();
+		_capture.setArguments(argv);
+	}
 
 	connect(&_capture,	&QProcess::started,
 		this,		&KsMainWindow::_captureStarted);
@@ -1155,7 +1173,6 @@ void KsMainWindow::_initCapture()
 	connect(&_captureLocalServer,	&QLocalServer::newConnection,
 		this,			&KsMainWindow::_readSocket);
 
-#endif
 }
 
 void KsMainWindow::_captureStarted()
-- 
2.20.1

[PATCH 3/3] kernel-shark: Use standart error message in KsMainWindow::_record()

[Yordan Karadzhov (VMware)]

The error message is printed using the method KsMainWindow::_error().
The message itself remains unchanged. If we want to change the message,
this can be done in another patch.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsMainWindow.cpp | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index e9c6d54..b462ded 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -890,7 +890,6 @@ void KsMainWindow::_record()
 #endif
 
 	if (geteuid() && !canDoAsRoot) {
-		QErrorMessage *em = new QErrorMessage(this);
 		QString message;
 
 		message = "Record is currently not supported.";
@@ -899,8 +898,7 @@ void KsMainWindow::_record()
 		message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
 		message += " sudo make install";
 
-		em->showMessage(message);
-		qCritical() << "ERROR: " << message;
+		_error(message, "recordCantStart", false, false);
 		return;
 	}
 
-- 
2.20.1

Re: [PATCH 1/3] kernel-shark: Show warning message when running as Root

[Yordan Karadzhov (VMware)]

Hey Mike,
Very well spotted, thanks!

It will be great if you can test the patches on your machine.

cheers,
Yordan

On 9.08.19 г. 13:33 ч., Mike Auty wrote:
> Hiya,
> 
> I can't really review the rest of the code, my main language is python,
> but I spotted a couple of minor typos that might be worth fixing?  I
> didn't want to post them to everyone since they're fairly trivial.  5:S
> 
> "noRootWorn" might want to "noRootWarn"
> "your one risk." might want to be "your own risk."
> 
> Thanks for putting the patches together so quickly!  5:)
> 
> Mike  5:)
> 
> On 09/08/2019 09:06, Yordan Karadzhov (VMware) wrote:
>> Running the KernelShark GUI with Root privileges is not recommended due
>> to security reasons. The user will be allowed to continue on its own risk.
>>
>> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204475
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   kernel-shark/src/KsMainWindow.cpp | 28 ++++++++++++++++++++++++++++
>>   kernel-shark/src/KsMainWindow.hpp |  2 ++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
>> index 6439265..2560bf8 100644
>> --- a/kernel-shark/src/KsMainWindow.cpp
>> +++ b/kernel-shark/src/KsMainWindow.cpp
>> @@ -76,6 +76,9 @@ KsMainWindow::KsMainWindow(QWidget *parent)
>>   	_createMenus();
>>   	_initCapture();
>>   
>> +	if (geteuid() == 0)
>> +		_rootWarning();
>> +
>>   	_splitter.addWidget(&_graph);
>>   	_splitter.addWidget(&_view);
>>   	setCentralWidget(&_splitter);
>> @@ -1271,3 +1274,28 @@ void KsMainWindow::_deselectB()
>>   	_mState.updateLabels();
>>   	_graph.glPtr()->model()->update();
>>   }
>> +
>> +void KsMainWindow::_rootWarning()
>> +{
>> +	QString cbFlag("noRootWorn");
>> +
>> +	if (_settings.value(cbFlag).toBool())
>> +		return;
>> +
>> +	QMessageBox warn;
>> +	warn.setText("KernelShark will have Root privileges.");
>> +	warn.setInformativeText("Continue on your one risk.");
>> +	warn.setIcon(QMessageBox::Warning);
>> +	warn.setStandardButtons(QMessageBox::Close);
>> +
>> +	QCheckBox cb("Don't show this message again.");
>> +
>> +	auto lamCbChec = [&] (int state) {
>> +		if (state)
>> +			_settings.setValue(cbFlag, true);
>> +	};
>> +
>> +	connect(&cb, &QCheckBox::stateChanged, lamCbChec);
>> +	warn.setCheckBox(&cb);
>> +	warn.exec();
>> +}
>> diff --git a/kernel-shark/src/KsMainWindow.hpp b/kernel-shark/src/KsMainWindow.hpp
>> index 62e66a0..4a7b8ab 100644
>> --- a/kernel-shark/src/KsMainWindow.hpp
>> +++ b/kernel-shark/src/KsMainWindow.hpp
>> @@ -238,6 +238,8 @@ private:
>>   
>>   	void _deselectB();
>>   
>> +	void _rootWarning();
>> +
>>   	void _updateFilterMenu();
>>   
>>   	void _filterSyncCBoxUpdate(kshark_context *kshark_ctx);
>>
> 

Re: [PATCH 1/3] kernel-shark: Show warning message when running as Root

[Steven Rostedt]

On Fri,  9 Aug 2019 11:06:21 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> Running the KernelShark GUI with Root privileges is not recommended due
> to security reasons. The user will be allowed to continue on its own risk.
> 
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204475
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/KsMainWindow.cpp | 28 ++++++++++++++++++++++++++++
>  kernel-shark/src/KsMainWindow.hpp |  2 ++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 6439265..2560bf8 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -76,6 +76,9 @@ KsMainWindow::KsMainWindow(QWidget *parent)
>  	_createMenus();
>  	_initCapture();
>  
> +	if (geteuid() == 0)
> +		_rootWarning();
> +
>  	_splitter.addWidget(&_graph);
>  	_splitter.addWidget(&_view);
>  	setCentralWidget(&_splitter);
> @@ -1271,3 +1274,28 @@ void KsMainWindow::_deselectB()
>  	_mState.updateLabels();
>  	_graph.glPtr()->model()->update();
>  }
> +
> +void KsMainWindow::_rootWarning()
> +{
> +	QString cbFlag("noRootWorn");

Mike already pointed this out (Thanks Mike!)

> +
> +	if (_settings.value(cbFlag).toBool())
> +		return;
> +
> +	QMessageBox warn;
> +	warn.setText("KernelShark will have Root privileges.");
> +	warn.setInformativeText("Continue on your one risk.");

Probably better to say:

  "KernelShark is running with Root privileges."
  "Continue at your own risk."

-- Steve

> +	warn.setIcon(QMessageBox::Warning);
> +	warn.setStandardButtons(QMessageBox::Close);
> +
> +	QCheckBox cb("Don't show this message again.");
> +
> +	auto lamCbChec = [&] (int state) {
> +		if (state)
> +			_settings.setValue(cbFlag, true);
> +	};
> +
> +	connect(&cb, &QCheckBox::stateChanged, lamCbChec);
> +	warn.setCheckBox(&cb);
> +	warn.exec();
> +}
> diff --git a/kernel-shark/src/KsMainWindow.hpp b/kernel-shark/src/KsMainWindow.hpp
> index 62e66a0..4a7b8ab 100644
> --- a/kernel-shark/src/KsMainWindow.hpp
> +++ b/kernel-shark/src/KsMainWindow.hpp
> @@ -238,6 +238,8 @@ private:
>  
>  	void _deselectB();
>  
> +	void _rootWarning();
> +
>  	void _updateFilterMenu();
>  
>  	void _filterSyncCBoxUpdate(kshark_context *kshark_ctx);

Re: [PATCH 2/3] kernel-shark: Don't use pkexec when running as Root

[Steven Rostedt]

On Fri,  9 Aug 2019 11:06:22 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> If KernelShark GUI has been started as Root we do not need to use
> "pkexec" when starting the Record dialog. Note that the actual place
> where "pkexec" gets used is in the script "kshark-su-record".
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/KsMainWindow.cpp | 47 +++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 2560bf8..e9c6d54 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -883,23 +883,26 @@ void KsMainWindow::_pluginAdd()
>  
>  void KsMainWindow::_record()
>  {
> -#ifndef DO_AS_ROOT
> +	bool canDoAsRoot(false);

Is this the C++ way of doing:

	bool canDoAsRoot = false; ?
>  
> -	QErrorMessage *em = new QErrorMessage(this);
> -	QString message;
> -
> -	message = "Record is currently not supported.";
> -	message += " Install \"pkexec\" and then do:<br>";
> -	message += " cd build <br> sudo ./cmake_uninstall.sh <br>";
> -	message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
> -	message += " sudo make install";
> +#ifdef DO_AS_ROOT

BTW, I think we should rename DO_AS_ROOT to "HAS_PKEXEC" as that is
much more descriptive of what that macro means.

> +	canDoAsRoot = true;
> +#endif
>  
> -	em->showMessage(message);
> -	qCritical() << "ERROR: " << message;
> +	if (geteuid() && !canDoAsRoot) {
> +		QErrorMessage *em = new QErrorMessage(this);
> +		QString message;
>  
> -	return;
> +		message = "Record is currently not supported.";
> +		message += " Install \"pkexec\" and then do:<br>";
> +		message += " cd build <br> sudo ./cmake_uninstall.sh <br>";
> +		message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
> +		message += " sudo make install";
>  
> -#endif
> +		em->showMessage(message);
> +		qCritical() << "ERROR: " << message;
> +		return;
> +	}
>  
>  	_capture.start();
>  }
> @@ -1134,9 +1137,24 @@ void KsMainWindow::loadSession(const QString &fileName)
>  
>  void KsMainWindow::_initCapture()
>  {
> +	bool canDoAsRoot(false);
> +
>  #ifdef DO_AS_ROOT
> +	canDoAsRoot = true;
> +#endif

As we are repeating this, why not just do at the top of the file:

#ifdef HAS_PKEXEC
# define has_pkexec 1
#else
# define has_pkexec 0
#endif

And remove the deplicate logic.

> +
> +	if (geteuid() && !canDoAsRoot)
> +		return;
>  
> -	_capture.setProgram("kshark-su-record");
> +	if (geteuid()) {

Also, "geteuid()" is a system call. It causes a call to the kernel each
time. We should cache that in a local variable instead, and use that.

	uid_t euid = geteuid();

Then use "euid" for other locations. But its OK to calculated it in the
function itself. That is, don't use the value from a different
function, as we don't want to worry about adding commands that change
the euid later.

-- Steve


> +		_capture.setProgram("kshark-su-record");
> +	} else {
> +		QStringList argv;
> +
> +		_capture.setProgram("kshark-record");
> +		argv << QString("-o ") + QDir::homePath();
> +		_capture.setArguments(argv);
> +	}
>  
>  	connect(&_capture,	&QProcess::started,
>  		this,		&KsMainWindow::_captureStarted);
> @@ -1155,7 +1173,6 @@ void KsMainWindow::_initCapture()
>  	connect(&_captureLocalServer,	&QLocalServer::newConnection,
>  		this,			&KsMainWindow::_readSocket);
>  
> -#endif
>  }
>  
>  void KsMainWindow::_captureStarted()

Re: [PATCH 3/3] kernel-shark: Use standart error message in KsMainWindow::_record()

[Steven Rostedt]

On Fri,  9 Aug 2019 11:06:23 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

Typo in subject "standard".

Other than that, it looks good.

-- Steve


> The error message is printed using the method KsMainWindow::_error().
> The message itself remains unchanged. If we want to change the message,
> this can be done in another patch.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/KsMainWindow.cpp | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index e9c6d54..b462ded 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -890,7 +890,6 @@ void KsMainWindow::_record()
>  #endif
>  
>  	if (geteuid() && !canDoAsRoot) {
> -		QErrorMessage *em = new QErrorMessage(this);
>  		QString message;
>  
>  		message = "Record is currently not supported.";
> @@ -899,8 +898,7 @@ void KsMainWindow::_record()
>  		message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
>  		message += " sudo make install";
>  
> -		em->showMessage(message);
> -		qCritical() << "ERROR: " << message;
> +		_error(message, "recordCantStart", false, false);
>  		return;
>  	}
>