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

Making everything executable in config_dir sounds wrong #38

Closed
amishmm opened this issue Apr 23, 2018 · 11 comments
Closed

Making everything executable in config_dir sounds wrong #38

amishmm opened this issue Apr 23, 2018 · 11 comments

Comments

@amishmm
Copy link
Contributor

amishmm commented Apr 23, 2018

chmod -R 755 $config_dir

This line makes everything under config directory (normally /etc/usermin) world executable, including all the config files and var-path, perl-path, version, webmin.acl, miniserv.users, miniserv.conf etc

Which is wrong.

Probably only 2 files, named start and stop require executable permissions.

So please fix it.

Thank you

@amishmm amishmm changed the title Making everything executable in config sounds wrong Making everything executable in config_dir sounds wrong Apr 23, 2018
@jcameron
Copy link
Collaborator

The directories need to be executable so that they can be listed though, and there's no simple portable way to have a chmod command set different permissions on files vs. directories.

@amishmm
Copy link
Contributor Author

amishmm commented Apr 24, 2018

This is not the case for webmin. i.e. /etc/webmin seems to set permission properly.

If you just want to set permissions on directory then its easy.

find "$config_dir" -type d -exec chmod 755 {} \+

For permissions on files:

find "$config_dir" -type f -exec chmod 644 {} \+

You can also combine above two.

find "$config_dir" -type d -exec chmod 755 {} \+ -o -type f -exec chmod 644 {} \+

@swelljoe
Copy link
Collaborator

As far as I know, find is as portable as you'd need for this. The BSDs and Linux are all identical (with regard to the -type flag and its options and -exec, though there were some historic implementations that had differences in -exec, I don't think any of those UNIXen still exist).

@jcameron
Copy link
Collaborator

Is there any downside to making config files executable though?

@amishmm
Copy link
Contributor Author

amishmm commented Apr 25, 2018

That's a strange and unexpected question from you!

Basic principle of *NIX security is not to make files executable unless they are actually meant to be executed.

Executable permission may not harm but no security expert would recommend doing it because you never know, there can be flaw anywhere in software.

@iliajie
Copy link
Collaborator

iliajie commented Apr 25, 2018

@amishxda I don't think you getting the things right, sorry. You need +x bit on the directory to open it. Try removing it from the directory and see what happens.

https://unix.stackexchange.com/q/21251/34581

@amishmm
Copy link
Contributor Author

amishmm commented Apr 25, 2018

Yes so why give 755 to all the files under it? (why -R switch?)

Use find and chmod combination above which will chmod only directories.

PS: You probably missed my 2nd comment here #38 (comment)

@jcameron
Copy link
Collaborator

All those files are just config files though .. even with the executable bit, they can't be run!

@amishmm
Copy link
Contributor Author

amishmm commented Apr 26, 2018

They can be run - the shell will try to run and set environment and then do nothing and exit.

Ofcourse it wouldnt make much sense.

But still all I am saying is, if files are not meant to be executed - the executable bit should not be set unnecessarily. Any "unknown" flaw in usermin can be exploited. (even if chances are very remote)

@amishmm
Copy link
Contributor Author

amishmm commented Apr 28, 2018

Created PR #39.

Note that "find" command is portable and is already used at other places in setup.sh

config files anyway has 644 - so no need to chmod them.

@amishmm
Copy link
Contributor Author

amishmm commented Apr 30, 2018

Closing as issue is fixed by:
PR: #39
Commit: 12a5e70

@amishmm amishmm closed this as completed Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants