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

Hard-coded path in backintime_qt_pol always starts installed instead of dev version #1361

Open
aryoda opened this issue Nov 13, 2022 · 7 comments
Assignees
Labels
Bug Low relevant, but not urgent

Comments

@aryoda
Copy link
Contributor

aryoda commented Nov 13, 2022

When I clone the source code from Git for development, debugging or testing purposes I cannot start BiT (root)
from the source code since it always starts the installed version (if any):

pkexec --disable-internal-agent $PREFIX "/usr/bin/backintime-qt" "$@"

Edit: make --prefix will most probably also be ignored

Expected behavior:

  • This path should be set by qt/configure to the absolute path of the source code (for development)
  • make install should install the file and replace the path by the installation path

Alternative: Use two different files for dev and installation.

@aryoda aryoda added the Bug label Nov 13, 2022
@buhtz
Copy link
Member

buhtz commented Nov 13, 2022

Interesting. Can't we just handle the path like it is done in the none-root start script?

CUR_PATH="$(dirname $(readlink -m $0))"
if [ -f "${CUR_PATH}/backintime.py" ]; then
APP_PATH=$CUR_PATH
else
APP_PATH=$(readlink -m "${CUR_PATH}/../share/backintime/common")
fi
python3 -Es $APP_PATH/backintime.py "$@"

EDIT: I don't like having to much labels but isn't it a good idea to have label for all that polkit- & root-related BIT issues?

@aryoda aryoda self-assigned this Nov 13, 2022
@aryoda
Copy link
Contributor Author

aryoda commented Nov 13, 2022

Related to #1349 (only because it also requires to fix the same starter shell script).

I will fix this together (after having more understanding of the Wayland background and known issues)...

@Germar I think you have "inherited" this implementation but perhaps you can remember ever having discussed the reason to hard code the "BiT (root)" path (eg. security reasons)?

@Germar
Copy link
Member

Germar commented Nov 14, 2022

I decided to not use the code from common/backintime because this could be used to run malicious code as root. Just create backintime-qt and hope someone will call backintime-qt_polkit inside that folder would give you root permissions.

For common/backintime I thought it wouldn't be a problem if the non-priv user would trick himself into running a script with his own priveleges. But rethinking it today brings me to a shared folder, where someone else create a malicious backintime.py and trick an other person to run backintime inside that folder. So I'd rather replace the code in common/backintime instead of reusing it in qt/backintime-qt_polkit

@aryoda
Copy link
Contributor Author

aryoda commented Nov 14, 2022

Yes, PATH or LD_LIBRARY_PATH manipulations can only be avoided with hard-coded full paths together with read-only rights to the script file.

I intend to

  1. implement a clear separation of backintime-qt and backintime-qt_polkit since currently the latter calls the former and this "indirection" makes an attack invisible in pkexec which normally shows the full path of the file to be executed to give the user a chance to cancel.
    => backintime-qt_polkit shall execute app.py directly

  2. rename backintime-qt_polkit to backintime-qt-root (which then is referenced in backintime-qt-root.desktop and is therefore a more intuitive name than the internally used polkit technology)

  3. fix the known Wayland issues (mainly running as root)

  4. make "BiT root" startable in the dev folder too (not yet sure what is the best way without creating another starter script for DEV while avoiding to install the wrong hard-coded full path with make install)

The current implementation of determining the path of the running backintime-qt starter script in

CUR_PATH="$(dirname $(readlink -m $0))"

is IMHO secure enough but the following line with a relative path

APP_PATH=$(readlink -m "${CUR_PATH}/../share/backintime/qt")

could be exploited but is currently required after installing it to /usr/bin.
./configure could solve this by eg. seding a full path (or for now hard-code the full path since we are anyhow not able to fully support another installation path via --prefix in configure and make ATM due to more hard-coded paths in other places like the .desktop files).

The cases like

  • a user downloads and executes a malicous file
  • a user directly executes a file from a mounted (or tmp) folder which was placed by an attacker

I would consider out-of-scope for the BiT starter scripts since this cannot be prevented by any clever script design.

@aryoda
Copy link
Contributor Author

aryoda commented Nov 14, 2022

BTW: I will solve the X11 too issue which prevents to start BiT root from the dev folder:

~/dev/backintime/qt (PR/add_doc_dev_with_wayland_doc)  > pkexec /home/user1/dev/backintime/qt/backintime-qt

Back In Time
Version: 1.3.3-dev

No protocol specified
qt.qpa.xcb: could not connect to display :0.0
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: eglfs, linuxfb, minimal, minimalegl, offscreen, vnc, wayland-egl, wayland, wayland-xcomposite-egl, wayland-xcomposite-glx, xcb.

Aborted (core dumped)

Reason: The polkit policy file grants GUI rights only to the hard-coded installation path and file name
but not to the dev path (cloned from Github):

<annotate key="org.freedesktop.policykit.exec.path">/usr/bin/backintime-qt</annotate>
<annotate key="org.freedesktop.policykit.exec.allow_gui">true</annotate>

By default pkexec does not allow to run X11 applications as another user since the $DISPLAY and $XAUTHORITY environment variables are not set. These two variables will be retained if the org.freedesktop.policykit.exec.allow_gui property is set.

For a fix the two mentioned env vars must be set explicitly.

See: https://www.freedesktop.org/software/polkit/docs/latest/pkexec.1.html

PS: Sorry for writing some lengthy comments here but my intention is to document issues and their solutions so that we have a documentation in case of further problems in the future ("why did we do it that way" ;-)

@Germar
Copy link
Member

Germar commented Nov 15, 2022

  1. implement a clear separation of backintime-qt and backintime-qt_polkit since currently the latter calls the former and this "indirection" makes an attack invisible in pkexec which normally shows the full path of the file to be executed to give the user a chance to cancel.
    => backintime-qt_polkit shall execute app.py directly

👍 I think we should rename app.py as well into something dscriptive. There are several places where app.py is shown.

  • rename backintime-qt_polkit to backintime-qt-root (which then is referenced in backintime-qt-root.desktop and is therefore a more intuitive name than the internally used polkit technology)

👍 we had backintime-root (starting with gksudo or kdesudo) back in the days before I added polkit. That's why I changed to backintime-qt_polkit

4. make "BiT root" startable in the dev folder too (not yet sure what is the best way without creating another starter script for DEV while avoiding to install the wrong hard-coded full path with make install)

I'd vote for seding from .configure. We could make all "start scripts" similar with same naming for path, app-name, options, etc. and sed over all.

@buhtz buhtz added this to the 1.3.5 or 1.4.0 milestone Mar 7, 2023
@buhtz
Copy link
Member

buhtz commented Dec 7, 2023

Fixing #1575 will make this problem go away. Put this Issue on-hold.

@buhtz buhtz added the Low relevant, but not urgent label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Low relevant, but not urgent
Projects
None yet
Development

No branches or pull requests

3 participants