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 |
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 😔 |
|
This PR is stale because it has been open 60 days with no activity. |
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.