Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sudo: Strip environment #22

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Sudo: Strip environment #22

merged 1 commit into from
Jul 24, 2018

Conversation

palinek
Copy link
Contributor

@palinek palinek commented Sep 12, 2016

Leave only required environment variables (for X & locale) to get into
the elevated child process.

closes lxqt/lxqt#899
closes lxqt/lxqt#1138

@agaida
Copy link
Member

agaida commented Sep 20, 2016

GTM

@agaida
Copy link
Member

agaida commented Oct 3, 2016

@palinek - i think, now it the right time to merge it

@palinek
Copy link
Contributor Author

palinek commented Oct 4, 2016

@tsujan Do you see any (new) issues by using this?

@tsujan
Copy link
Member

tsujan commented Oct 4, 2016

@tsujan Do you see any (new) issues by using this?

I'm afraid, yes! Please see lxqt/lxqt#1138 (comment). That was one day after your PR. Should I try it again?

@palinek
Copy link
Contributor Author

palinek commented Oct 4, 2016

I'm afraid, yes! Please see lxqt/lxqt#1138 (comment).

I saw that comment... and added a fixup to not strip PATH (maybe this is somehow configurable for preserving in sudoers)

Should I try it again?

Yes, please.

@tsujan
Copy link
Member

tsujan commented Oct 4, 2016

This time it opens the app as root without changing the permissions of its config file, but... Let me show you two screenshots. This is lxsudo featherpad with your PR:

fp1

This one is with my simple change:

fp2

As you see, the theme isn't preserved in the first case, while it is in the second (exactly as kdesu does under KDE).

@palinek
Copy link
Contributor Author

palinek commented Oct 4, 2016

As you see, the theme isn't preserved in the first case, while it is in the second

Yes. That's the difference between those two:

  • my -> strip everything except few (well) known variables
  • yours -> leave everything except strip few (well) known variables

The result is obvious (in this PR usage all the QT_* are stripped, in yours they are left so you're using e.g. kvantum also while with root privileges).

@tsujan
Copy link
Member

tsujan commented Oct 4, 2016

OK, but shouldn't lxqt-sudo behave as kdesu does? I think the point of an lxqt-sudo instead of simple sudo is that, among other things.

@palinek
Copy link
Contributor Author

palinek commented Oct 4, 2016

OK, but shouldn't lxqt-sudo behave as kdesu does? I think the point of an lxqt-sudo instead of simple sudo is that, among other things.

Sorry. Leaving this to others to judge... (...when people are saying preserve the env and on the other side others are voting for stripping...)

Should the decision be made, I'll hapilly either merge this or reject it.

@tsujan
Copy link
Member

tsujan commented Oct 4, 2016

Sorry. Leaving this to others to judge...

I have no objection.

@tsujan
Copy link
Member

tsujan commented Oct 4, 2016

Sorry, I judged too quickly. FeatherPad, in the above example, can use its own icons. See what happens to lxsudo pcmanfm-qt:

pcman1

This is with my few changes:

pcman2

@palinek
Copy link
Contributor Author

palinek commented Oct 4, 2016

See what happens to lxsudo pcmanfm-qt:

...and the very same happens if you use gksu --sudo-mode pcmanfm-qt

@tsujan
Copy link
Member

tsujan commented Oct 4, 2016

...and the very same happens if you use gksu --sudo-mode pcmanfm-qt

That only indicates gksu isn't a good option. We're dealing with a practical issue here -- it's not just theoretical anymore. Who could use pcmanfm-qt as root in that way?! And it isn't alone in this.

@paulolieuthier
Copy link
Contributor

@palinek do you see any issues with allowing QT_* variables?

@palinek
Copy link
Contributor Author

palinek commented Oct 4, 2016

do you see any issues with allowing QT_* variables?

I wasn't the one who required the stripping of the environment...

@paulolieuthier
Copy link
Contributor

I know. I'm just thinking that if allowing QT_* would not be a problem, then we should do it, because it would fix the theming issues (right?).

@Vladimir-csp what do you think?

@Vladimir-csp
Copy link

I always thought that theming issues are to be expected when using sudo, unless some overlay mechanism is in place, i.e. xsettings. So I do not know. Anyone knows good security expert?

@palinek
Copy link
Contributor Author

palinek commented Jan 17, 2018

Rebased and:

  • added note about (non)stripped env. variables
  • added the QT_PLATFORM_PLUGIN, QT_QPA_PLATFORMTHEME to preserved vars

Leave only required environment variables (for X & locale) to get into
the elevated child process.
@palinek palinek merged commit 07ec9ec into master Jul 24, 2018
Pull Requests automation moved this from Need Approve to Done Jul 24, 2018
@palinek palinek deleted the environ branch July 24, 2018 18:22
@palinek palinek mentioned this pull request Jul 24, 2018
@agaida agaida added this to Archive in PRs Nov 5, 2018
@agaida agaida moved this from Done to Archive in Pull Requests Jan 19, 2019
@sergwish sergwish mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs
  
Archive
Pull Requests
  
Old Archive (fucked up)
Development

Successfully merging this pull request may close these issues.

Lxqt-sudo changes config permissions [lxqt-sudo] Consider (not) preserving of environment with sudo backend
5 participants