Dont run browser as root #5058
Dont run browser as root #5058
Conversation
ui/webui/webui-desktop
Outdated
| @@ -66,21 +66,23 @@ esac | |||
|
|
|||
| # prepare empty firefox profile dir with theme based on the passed profile id | |||
| FIREFOX_THEME_DIR="/usr/share/anaconda/firefox-theme" | |||
| FIREFOX_PROFILE_PATH="/tmp/anaconda-firefox-profile" | |||
| LIVEUSER=$(id -n -u $PKEXEC_UID) | |||
There was a problem hiding this comment.
So this same script is also used on the boot.iso to run the Web UI there & I don't think there is a live user account available there. I'll try to build a boot.iso with this PR & also a Live image when I'm at it.
There was a problem hiding this comment.
should be okay, if $PKEXEC_UID is unset LIVEUSER should just get set to the current running user.
Maybe the variable LIVEUSER could have a better name (INSTALLER_USER?)
There was a problem hiding this comment.
of course that means firefox continues to run as root on boot.iso, which is less than ideal, but it shouldn't complain in that case, because it won't think it's a user elevated to root, but root all the way through
There was a problem hiding this comment.
Yes, I think renaming the variable would be best, plus maybe a comment we expect it to be liveuser on live?
There was a problem hiding this comment.
Yes, I think renaming the variable would be best, plus maybe a comment we expect it to be liveuser on live?
Agreed with both.
|
/boot-iso --webui |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
halfline
force-pushed
the
dont-run-browser-as-root
branch
from
b3cd860
to
81b53cd
Compare
halfline
force-pushed
the
dont-run-browser-as-root
branch
from
fb9e873
to
d77473c
Compare
|
/kickstart-test --testtype smoke |
|
So, I triggered the tests and it looks like something did not work for webui... |
hmm i just tried it locally again, and it seems to work fine here. do you have more logs ? |
|
note this pull request will need some changes if #5057 lands first (we'll need to stop unsetting XAUTHORITY and XDG_RUNTIME_DIR in webui-desktop) |
halfline
force-pushed
the
dont-run-browser-as-root
branch
from
d77473c
to
75fedfc
Compare
|
i've now rebased this on top of #5057 so let's not land this until it lands. |
| readarray -t user_environment < <(pkexec --user "${INSTALLER_USER}" env XDG_RUNTIME_DIR="/run/user/${PKEXEC_UID}" systemctl --user show-environment) | ||
|
|
||
| for variable in "${user_environment[@]}"; do | ||
| export "$variable" |
There was a problem hiding this comment.
Shellcheck says:
data/liveinst/liveinst:34:16: warning: This does not export 'variable'. Remove $/${} for that, or use ${var?} to quiet. [SC2163]
If you know what you're doing (you do), alternatively, # shellcheck disable=SC2163
|
I strongly suggest we only merge this after we are done with F39 Beta, as I don't think we will realistically have the time to properly test this and fix any possible breakage. |
M4rtinK
force-pushed
the
dont-run-browser-as-root
branch
from
75fedfc
to
cb62a85
Compare
| readarray -t user_environment < <(pkexec --user "${INSTALLER_USER}" env XDG_RUNTIME_DIR="/run/user/${PKEXEC_UID}" systemctl --user show-environment) | ||
|
|
||
| for variable in "${user_environment[@]}"; do | ||
| export "$variable" |
Check warning
Code scanning / shellcheck
This does not export 'variable'. Remove $/${} for that, or use ${var?} to quiet. Warning
|
Rebased on top of latest |
|
/kickstart-test --testtype smoke |
|
/build-image --live |
|
/boot-iso --webui |
|
@halfline Just a heads up that I had to rewrite the first commit message a bit as some of the changes I've previously cherry-picked to the Firefox styling PR that has been merged a while ago. :) |
|
Images built based on commit cb62a85:
Download the images from the bottom of the job status page. |
|
/boot-iso --webui |
|
/build-image --boot.iso --webui |
|
Images built based on commit a1de17d:
Download the images from the bottom of the job status page. |
|
So status as of today:
Looks like the PR might have dropped root privileges not just from the Firefox process or also from the launching script ofr even the backend ? Definitely does not look healthy. |
The titlebar with "Mozilla Firefox" has been fixed but there are some more bits that can be cleaned up. This commit achieves that by: 1. Make sure more of the environment is bubbled through anaconda to the webui launcher. In particular, we need XDG_CURRENT_DESKTOP, but this commit brings it all through, so firefox runs in an environment as close to getting run directly by the live user as possible. 2. Two exceptions are XAUTHORITY and XDG_RUNTIME_DIR which need to remain unset until we can run firefox as a normal user instead of root.
At the moment most of the firefox command line is getting placed in a variable named $BROWSER and then getting run as $BROWSER http://url This only works if $BROWSER is at the very front of the line or if it's run through eval. Instead, make BROWSER into an array so it's positional arguments get expanded positionally.
It's not a good idea to run UI code as root if we can help it, and since the webui separates front end from backend, we don't need to run the front end code as root. This commit changes webui-desktop to start firefox as the liveuser. The entire script could probably be run unprivileged with a few changes to the cockpit parts (different port, new polkit policy, cockpit.spawn changes to run as superuser), but that's a change for another time.
M4rtinK
force-pushed
the
dont-run-browser-as-root
branch
from
a1de17d
to
28ea45b
Compare
|
/build-image --boot.iso --live --webui |
|
Images built based on commit 28ea45b:
Download the images from the bottom of the job status page. |
|
/kickstart-test --testtype smoke |
|
/build-image --live |
|
Images built based on commit 28ea45b:
Download the images from the bottom of the job status page. |
|
Hi all, I was just pinged by Red Hatters who tested accessibility of web UI. They found that our solution doesn't work because root applications don't have access to accessibility DBus. So, I think this information is raising a priority to merge this. |
I don't see how org.a11y.Bus is involved here. Can you ask for some more insights? Is the testing of the accessibility prohibited or the accessibility of the new UI itself is hindered? I am assuming it's the former? |
|
Before this MR, unfortunately the latter. I have no idea how a root process can even find the correct a11y bus. And I can't test with a test image without building a new one of this PR, as they're all expired now. |
|
/build-image --live |
|
Images built based on commit 28ea45b:
Download the images from the bottom of the job status page. |
|
This PR is stale because it has been open 60 days with no activity. |
|
Could we push this forward, please? From the perspective of a11y, we'd have to do a huge and ugly environment variables hack to tell the browser about the unprivileged a11y bus, or we do it cleanly. |
|
@tyrylu thanks for ping. I added this in our current quarter planning. I see the current pr is terribly outdated, it needs a nontrivial amount of work. |
|
Thanks for that. But, if I could set the criteria, when the web UI is the default (no idea when that's planned), and this is not resolved, I'd call it a critical regression, of course, only for the users needing Orca, I know... |
|
This PR is stale because it has been open 60 days with no activity. |
|
Alright, now, F41 has the web-ui installer, and this situation is not resolved, so, we have a return to the dark ages where a visually impaired person could not install an OS... |
|
We are currently working on even keep it there. It's still not accepted by FESCO. We definitely want to add this, however first we need to resolve how to keep Web UI in Fedora 😔 |
Right now firefox gets run as root which it really doesn't like doing. We have a workaround in place (unset XAUTHORITY) to prevent it from even erroring on start up.
There's no good reason to run it as root though. We can just run it unprivileged since all the heavy lifting is in privileged backend code anyway.