Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

do not use os-homedir #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

do not use os-homedir #12

wants to merge 2 commits into from

Conversation

pravi
Copy link

@pravi pravi commented Oct 18, 2016

Its hard coded paths are a serious security issue. Closes: #11

pravi added 2 commits October 18, 2016 22:46
os-homedir hard codes paths which does not work reliably and is a security issue.
remove from os-homedir from package.json
@iarna
Copy link
Contributor

iarna commented Oct 18, 2016

Can't accept this until Node.js 0.10 support is dropped AND we're prepared to break anyone using it.

@doowb
Copy link

doowb commented Oct 19, 2016

@iarna I created this polyfill since we use os.homedir() in some of our modules and want to continue supporting Node.js 0.10 also.

Will you consider using this in place of os-homedir? I think it would just require updating this PR.

@iarna
Copy link
Contributor

iarna commented Apr 21, 2017

@doowb So if I understand correctly, your polyfill differs in that it checks /etc/passwd for home directories before falling back to constructing paths from the username?

// on linux platforms (including OSX) find the current user and get their homedir from the /etc/passwd file

This isn't true for OSX anymore. (Which I'm guessing you're aware given the later construction of /Users/<name> on darwin.)

It's not guaranteed to be true on Linux either, if you have, for instance, an LDAP based user directory. As @pravi considers ever guessing at a homedir a security issue, I'm not sure that improves things any? It's not really for me to say though. The existing osenv uses os-homedir which on Node.js > 0.10 just calls os.homedir.

@doowb
Copy link

doowb commented Apr 21, 2017

Hi @iarna

your polyfill differs in that it checks /etc/passwd

Yes, because I'm following the steps that I found in the libuv function that current versions of os.homedir use. If that doesn't work, then I fall back to using the environment variables.

This isn't true for OSX anymore

I didn't know that, thanks for point it out. I followed what libuv was doing, however, I'm fine with learning more about when and how LDAP / Active Directory systems are used so the polyfill is more accurate. I don't think these are taken into account in recent Node.js versions that have os.homedir, but I can look to see if there have been updates to add those checks.

on Node.js > 0.10 just calls os.homedir

homedir-polyfill does the same thing. I created this because @pravi was also making some requests on other modules that I maintain that used os.homedir and that we wanted to be compatible with Node.js <= 0.10. Since using homedir-polyfill we haven't seen any homedir issues, and that's from almost 14 million downloads.

@iarna
Copy link
Contributor

iarna commented Apr 22, 2017

@doowb I'd like to hear from @pravi if your polyfill actually resolves their concerns? 'cause I don't see how it would? I mean, if you view this as a legitimate security issue of course.

The main difference between your code and libuv, is that libuv gets to call getpwuid_r which does not necessarily talk to /etc/passwd. I am of the opinion that it's the only correct way to get the answer and that if one views the current behavior as a security risk then the only fix for it is to exit with an error if os.homedir and various environmental options aren't available. Falling back to a passwd file might be ok although if it's not actually the canonical source of that information, I dunno.

@pravi
Copy link
Author

pravi commented Apr 22, 2017

We use os.homedir (patch modules) as we have a recent enough nodejs in debian.

@martinheidegger
Copy link

Does #19 address this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants