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

updater.sh: cope if run with sudo [withdrawn see #1651 instead] #1595

Closed
wants to merge 1 commit into from

Conversation

icpantsparti2
Copy link
Contributor

hi @earthlng, some small additions that 'might' help with #1587. Thank you.

in version 3.5 if you run updater.sh with sudo then: (1) user.js may only be readable by root and not load for the Firefox user (2) if the script is run first time with sudo, then again without sudo, permissions from the previous run may cause issues (eg no backup or update of user.js) with little or no warning.

Therefore v3.6 to: (1) set some file/dir permissions (on those created) if run with sudo (2) show info if some access permissions are not as expected (3) added some further error messages

hi @earthlng, some small additions that 'might' help with [arkenfox#1587](arkenfox#1587)). Thank you.

in version 3.5 if you run updater.sh with sudo then: (1) user.js may only be readable by root and not load for the Firefox user (2) if the script is run first time with sudo, then again without sudo, permissions from the previous run may cause issues (eg no backup or update of user.js) with little or no warning.

Therefore v3.6 to: (1) set some file/dir permissions (on those created) if run with sudo (2) show info if some access permissions are not as expected (3) added some further error messages
@icpantsparti2 icpantsparti2 changed the title v3.6 updater.sh v3.6 Nov 24, 2022
@CelestialNebula
Copy link
Contributor

(I don't know how macOS works they may do different stuff for root.)

https://stackoverflow.com/questions/18215973/how-to-check-if-running-as-root-in-a-bash-script
I can't see any reason to use sudo, so I think this is simpler (added at
the start):

if [ "${EUID:-$(id -u)}" -eq 0 ]
then
	printf "You shouldn't run this as root.\n"
	exit 1
fi

I doubt it will matter but this may catch more https://stackoverflow.com/a/65358284:

ls /root > /dev/null 2>&1 && { printf "You shouldn't run this as root.\n"
			       exit 1; }

@mhmdanas
Copy link
Contributor

mhmdanas commented Jan 8, 2023

@CelestialNebula's suggestion makes sense, you should definitely not run a command or script with sudo unless it's absolutely necessary. I would be especially careful with running sudo for anything downloaded from the internet.

@CelestialNebula I have macOS and wouldn't mind testing anything if needed.

@CelestialNebula
Copy link
Contributor

CelestialNebula commented Jan 18, 2023

@mhmdanas
I heard that macos switched to zsh and it seems that supports EUID [1] and it does more or less the same thing.

If you could test this to confirm:

test.sh:

#!/usr/bin/env sh

if [ "${EUID:-$(id -u)}" -eq 0 ]
then
	printf "You shouldn't run this as root.\n"
	exit 1
fi

$ sudo sh test.sh

If it worked you should see: You shouldn't run this as root.
Then run
$ echo $?
and it should be 1

Edit:
Alternatively look at the group file and see if root has a 0 in it.

$ less /etc/group

Look for a line with something like root:*:0:root

[1] https://zsh.sourceforge.io/Doc/Release/Parameters.html#Parameters-Set-By-The-Shell

@mhmdanas
Copy link
Contributor

mhmdanas commented Jan 27, 2023

@CelestialNebula apologies for the delay. I tested the script you provided with macOS's Dash, Bash, and Zsh (because macOS allows you to choose which of these shells /bin/sh points to), and it worked just fine under all of them.

Look for a line with something like root:*:0:root

There is no line in my /etc/group file that begins with root.

@CelestialNebula
Copy link
Contributor

@mhmdanas can you test this?

This is going to run recursively so you should probably make a directory
for it:

$ mkdir testdir
$ cd testdir/

test.sh:

#!/usr/bin/env sh

# Check if running as root and if any files have the owner/group as root/wheel.
if [ "${EUID:-$(id -u)}" -eq 0 ]; then
	printf 'You should not run this as root (such as with doas/sudo).\n'
	exit 1
elif [ -n "$(find ./ -user 0 -o -group 0)" ]; then
	printf 'It looks like this script was previously run with elevated privileges,
you will need to change ownership of the following files to your user:\n'
	find ./ -user 0 -o -group 0
	exit 1
fi

echo Worked

First you will need to make a file with sudo
# touch zfakefile
Then run test.sh as the normal user:
$ sh ./test.sh
it should say:

It looks like this script was previously run with elevated privileges,
you will need to change ownership of the following files to your user:
zfakefile

Then remove zfakefile and rerun. Should say:
Worked

@mhmdanas
Copy link
Contributor

Sorry for the delay, I totally forgot about your comment. I followed your steps with both Dash, Bash, and Zsh, and it worked with all of them. Note that the output was this with all shells:

It looks like this script was previously run with elevated privileges,
you will need to change ownership of the following files to your user:
.//zfakefile

CelestialNebula added a commit to CelestialNebula/user.js that referenced this pull request Apr 1, 2023
Check if running as root and if any files have the owner/group as root|wheel.
Abort on both.

Should (hopefully) prevent stuff like: arkenfox#1587
Discussion: arkenfox#1595
@icpantsparti2 icpantsparti2 changed the title updater.sh v3.6 updater.sh: cope if run with sudo [withdrawn see #1651 instead] Apr 1, 2023
@icpantsparti2
Copy link
Contributor Author

Withdrawn in favor of #1651. Closing without further action.

earthlng added a commit that referenced this pull request Apr 22, 2023
* updater.sh/prefsCleaner.sh: Check for root and abort

Check if running as root and if any files have the owner/group as root|wheel.
Abort on both.

Should (hopefully) prevent stuff like: #1587
Discussion: #1595

---------

Co-authored-by: Mohammed Anas <triallax@tutanota.com>
Co-authored-by: earthlng <earthlng@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants