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

Ensure script only runs in users HOME directory #1221

Closed
wants to merge 1 commit into from
Closed

Ensure script only runs in users HOME directory #1221

wants to merge 1 commit into from

Conversation

antonosmond
Copy link

I had a minor issue with NVM after installing on an Ubuntu Server.
The issue was caused by the fact the nvm.sh script tries to perform actions that require permissions in the current working directory (an unknown directory at the time the users .bashrc is sourced.
Say I have a user bob with NVM and node installed.
If I log on to my Ubuntu instance as user root and am in the root home directory /root then do su bob to switch to the bob user, bob's ~/.bashrc is sourced and NVM sources "$NVM_DIR/nvm.sh". In most cases this isn't a problem, but in the example I provided, as I was in the /root directory when I switched to user bob, the current directory is maintained which is /root and bob doesn't have read/write access there so when the NVM script is sourced it fails with permission errors. In theory, nothing sourced in the .bashrc should rely on the unkown current working directory and to fix NVM, all that's needed is to ensure the script runs in a location where it has access. As NVM is sourced by a users .bashrc I think it makes to sense that it runs in the users home directory, hence the change I'm proposing.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

Since this script is sourced, however, wouldn't that change the current working directory for the user?

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

What are the operations "sourcing nvm.sh" does that require permissions? I'd rather take a look at those, I think.

@ljharb ljharb added the needs followup We need some info or action from whoever filed this issue/PR. label Sep 7, 2016
@antonosmond
Copy link
Author

Sourcing .bashrc won't change the working directory and neither will switching to a user from another user. This means if I'm in a directory I don't have permissions to, when .bashrc is sourced and in turn sources nvm.sh, it fails. Not sure what nvm.sh is doing that requires permissions in the current working directory as I didn't delve too deep into the script but you can try the following by manually running these commands in the terminal on an Ubuntu box to see for yourself:

# switch to root user
sudo su

# change to root's $HOME directory
cd "$HOME"

# create a new user called bob
adduser --disabled-password --gecos "" bob

# check the current working directory
pwd

# at this point, you should be in the /root directory which bob won't have permissions to

# switch to user bob
su bob

# here, you'll have a new shell session as user bob and bob's .bashrc will have been sourced
# note that everything so far is fine with no errors
# check the current directory now you're logged in as bob
pwd

# you should still be in /root as you've not changed directories, however user bob won't have permissions to do anything there
# try a command like `ls` and you'll get permission denied
ls

# install NVM
curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.31.7/install.sh | bash

# interstingly, nothing in your install.sh script tries to perform operations in the current directory 
# so NVM installs successfully with no permission errors

# exit bob's session
exit

# now log in as bob again - this time nvm.sh is in bob's .bashrc and will be sourced on login
su bob

# note we can still log in as bob without errors despite the fact we're in the /root directory

# if we try and install node now, we'll get permission errors because we're in the /root dir 
# so something in the install process tries to operate in the current directory
nvm install 6.5.0

# if we make sure we're in bobs $HOME dir where we do have permissions we can install node successfully
cd "$HOME"
nvm install 6.5.0

# exit bobs session again
exit

# get back to roots home dir
cd "$HOME"

# now when we try to login as bob, we'll get the same error we had when trying to install node
su bob

# so we managed to switch to user bob while in the /root directory without any problems before installing node
# now we've installed it we get errors and it's because something in nvm.sh
# or something that nvm.sh executes 
# is attempting to perform an operation in the current directory.

# if we exit bob's session again
exit

# change to a directory bob will have access to e.g.
cd /home/bob

# try logging in as bob
su bob

# there's no error
# we're still sourcing nvm.sh in the .bashrc and still performing ops on the current dir
# but as the current dir is now something bob has access to we don't get an error

The only reason the working directory isn't changed in this example is because I'm not doing a full login as bob i.e. su -l bob.
This being said however, anything being put in .bashrc should be safe for every type of bash shell session or in some more obscure / complex scenarios it can break things as it has done in this example. Adding a cd "$HOME" in nvm.sh is a quick & dirty fix I admit, but should be safe and solve the issue.
Alternatively, if you want to find what's operating on the current directory it must be something in nvm.sh that only gets executed if node is installed - when we just installed NVM without installing node itself, we didn't get an error and only after installing node did we start getting the error.
Hope this info is useful.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

I'm talking about when sourcing nvm.sh directly on the command line, which should also work, and should not change the PWD.

@antonosmond
Copy link
Author

bob@vagrant-ubuntu-trusty-64:/root$ source ~/.nvm/nvm.sh
Error: EACCES: permission denied, scandir '/root'
    at Error (native)

Error: EACCES: permission denied, open 'npm-debug.log.1920111915'
    at Error (native)

nvm is not compatible with the npm config "prefix" option: currently set to ""
Run `nvm use --delete-prefix v6.5.0 --silent` to unset it.

@antonosmond
Copy link
Author

So yeah, something in nvm.sh or something it executes is trying to perform ops in the current working directory which is an unknown entity therefore can't be safely relied on.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

@antonosmond does the same thing still happen if you source ~/.nvm/nvm.sh --no-use?

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

Also, if you do npm config get prefix (assuming you have a system npm) what happens?

@antonosmond
Copy link
Author

I know it's a pretty obscure edge case but thought I'd share it as me & colleague spent a couple of hours today trying to work out why it wasn't working! lol! To give some context I'm a DevOps engineer and was trying to automate NVM installation on a GOCD agent in AWS. I wanted the install to happen on boot (AWS provide userdata where we can run script to do stuff like this) but that runs as root and I needed to install NVM for the go user (the user the GOCD agents runs as).
We were hacking around on an ubuntu instance trying to get it to install as another user but finding when we were switching to the other user to check the installation, we were getting those errors. It was only this evening I realised that when we were switching users, we were still in the /root dir! lol! I'm sure this could cause problems in other obscure scenarios though so appreciate you taking the time to look at it.

@antonosmond
Copy link
Author

Doesn't happen if I do source ~/.nvm/nvm.sh --no-use
If I do npm config get prefix as the bob user it fails because of that error sourcing .nvm.sh so it doesn't know where npm is:

bob@vagrant-ubuntu-trusty-64:/root$ npm config get prefix
The program 'npm' is currently not installed. To run 'npm' please ask your administrator to install the package 'npm'

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

OK, i think the error is happening within npm itself - it's just that the "use" path invokes it.

When I make a dir that's owned by somebody else and i have -x on, I get:

$ npm config get prefix
path.js:1144
          cwd = process.cwd();
                        ^

Error: EACCES: permission denied, uv_cwd
    at Error (native)
    at Object.resolve (path.js:1144:25)
    at Function.Module._resolveLookupPaths (module.js:376:17)
    at Function.Module._resolveFilename (module.js:446:31)
    at Function.Module._load (module.js:403:25)
    at Module.require (module.js:483:17)
    at require (internal/module.js:20:19)
    at $HOME/.nvm/versions/node/v6.5.0/lib/node_modules/npm/bin/npm-cli.js:25:13
    at Object.<anonymous> ($HOME/.nvm/versions/node/v6.5.0/lib/node_modules/npm/bin/npm-cli.js:75:3)
    at Module._compile (module.js:556:32)

@antonosmond
Copy link
Author

Hmmm makes sense as the error I posted earlier looked like npm may be the issue:

Error: EACCES: permission denied, open 'npm-debug.log.1920111915'
    at Error (native)

I don't think this would be a problem with a native npm installation though, it's just because of the way we're sourcing in the .bashrc I think.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

In other words, this PR doesn't fix the problem - running nvm use node in that dir will cause the same problem.

I'd be interested in exploring the callsite where npm config get prefix runs tho - altho it is usually important that it runs in the current directory.

@antonosmond
Copy link
Author

Yes and no RE fixing the problem. It's expected behaviour that I can't run operations that rely on the CWD. However, in this case it's crashing on just switching to a user and borking that users env. By adding the cd "$HOME" in nvm.sh, I no longer get an error when switching users and my env doesn't get borked by the crash sourcing the .bashrc.
Scenario A (no changes): switch user, immediate error, env slightly screwed i.e. no NVM etc even if I change to a different directory.
Scenario B (with cd $HOME): switch user, no crash, env intact, can CD and run NVM happily.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

Even with the error though - that should just prevent the nvm use operation that nvm install calls internally. That shouldn't actually interfere with the install, or with the env.

@antonosmond
Copy link
Author

Yes you're right. It'd still be nice not to have something in the .bashrc that can cause an error just by switching to that user. If there's no risks with adding the change to prevent the error I'd like to see it go in but guess it's your call. It could be made safer by storing the CWD at the start, cd'ing to $HOME and doing it's stuff then cd'ing back to the original CWD, just to make sure nothing else sourced or executed in the .bashrc is affected by the cd. Either way I'm not too fussed if you add it or not as it is a pretty obscure edge case. :)

@ljharb
Copy link
Member

ljharb commented Sep 7, 2016

I'd say a better approach would be to, only inside nvm use, detect if npm config get prefix is going to fail (ie, checking the perms) and then use pushd/popd to temporarily cd to $HOME for that call, and back after. That's a bit complex, but if you want to adapt this PR i'm definitely willing to include it.

@antonosmond
Copy link
Author

Sounds good to me but I'm not going to look at it right now as not sure where you are but it's 12:35am here and I have to be up for work 2moro! lol!
It's actually kind of a linux bug to be fair.
If I'm in /root and do su bob, essentially I'm saying I want to start a shell session as bob in directory /root. That command should really fail if bob doesn't have permissions to /root. If I do su -l bob and try and cd to /root it won't let me as I'll get permission denied. So it's a little worrying that I'm actually able to start a shell session in a directory I shouldn't have access to! lol!

@ljharb
Copy link
Member

ljharb commented Jun 13, 2017

@antonosmond are you still interested in implementing the approach here?

@ljharb
Copy link
Member

ljharb commented Jun 25, 2018

@antonosmond are you no longer interested in this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs followup We need some info or action from whoever filed this issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants