Giter VIP home page Giter VIP logo

Comments (11)

analogic avatar analogic commented on June 17, 2024 1

I ran into this bug recently - it seems that process.setgid(gid) and process.setuid(uid) are not working properly in v20. I am also unable to understand how it works inside the system, and how it is even possible for a process with downgraded privileges to still work as root.

I've reported this bug to Nodejs (https://github.com/nodejs/node/issues/51148), but as you can see... well... you can't really see. I think it is pretty serious security issue.

You can try it for your self, just use setuid and create dir/file and see which rights are there.

sh@zumpa:~$ sudo docker run -v ~/:/mnt node:16 /mnt/test.js /tmp/test
Dropped privileges to UID: 1, GID: 1
Directory '/tmp/test' created successfully.
Directory owner UID: 1, GID: 1

sh@zumpa:~$ sudo docker run -v ~/:/mnt node:18 /mnt/test.js /tmp/test
Dropped privileges to UID: 1, GID: 1
Directory '/tmp/test' created successfully.
Directory owner UID: 1, GID: 1

sh@zumpa:~$ sudo docker run -v ~/:/mnt node:20 /mnt/test.js /tmp/test
Dropped privileges to UID: 1, GID: 1
Directory '/tmp/test' created successfully.
Directory owner UID: 0, GID: 0

sh@zumpa:~$ sudo docker run -v ~/:/mnt node:21 /mnt/test.js /tmp/test
Dropped privileges to UID: 1, GID: 1
Directory '/tmp/test' created successfully.
Directory owner UID: 0, GID: 0

from haraka.

analogic avatar analogic commented on June 17, 2024 1

One way to resolve the issue would be to not run Haraka as root. But I suspect there are very good reasons why the docs instructs to run it as root and have it drop privileges afterwards instead.

Sure, ports below 1024 were historically meant to be opened only by root.

from haraka.

DoobleD avatar DoobleD commented on June 17, 2024

Thanks for the feedback @analogic.

I was able to reproduce it with a simple node script too. I think the node team most likely knows about this behavior, since they purposefuelly removed ownership management for root scripts in node 18.14:

Explanation: when run as root previous versions of npm attempted to manage file ownership automatically on the user's behalf. this behavior was problematic in many cases and has been removed in favor of allowing users to manage their own filesystem permissions

how it is even possible for a process with downgraded privileges to still work as root.

I'm not 100% sure but it looks like that's because fs.promises.* functions are executed in node's underlying thread pool most of the time, i.e. not in the event loop thread. And the setuid/setgid functions probably only apply to the event loop thread, or threads created after calls to setuid/setgid. Node's thread pool must be created well before the Haraka code dropping privileges is executed.

The fs/promises API provides asynchronous file system methods that return promises.

The promise APIs use the underlying Node.js threadpool to perform file system operations off the event loop thread.

Another thing that lead me to this is that when using sync fs functions - which do execute in the event loop thread - the issue doesn't happen.

For now the only workaround I got is to have the hooks code write into temporary files, and have a separate root script running as daemon that fixes theses temp files ownership and then moves them to the final place. Or if that's acceptable in your case you could use the sync functions (e.g. fs.writeFileSync).

from haraka.

DoobleD avatar DoobleD commented on June 17, 2024

One way to resolve the issue would be to not run Haraka as root. But I suspect there are very good reasons why the docs instructs to run it as root and have it drop privileges afterwards instead.

from haraka.

analogic avatar analogic commented on June 17, 2024

I'm not 100% sure but it looks like that's because fs.promises.* functions are executed in node's underlying thread pool most of the time, i.e. not in the event loop thread. And the setuid/setgid functions probably only apply to the event loop thread, or threads created after calls to setuid/setgid. Node's thread pool must be created well before the Haraka code dropping privileges is executed.

Yes, it could be. My view may be simplistic, but if "ps" lists the process as user-owned, I would expect that if nodejs tries to do root stuff from a process with dropped privileges, the kernel would forbid this behaviour.

I originally thought that all theads inherit the same rights from the process, but... https://stackoverflow.com/a/45398180

And if I go deeper and test it:

# NodeJS v20
sh@zumpa:~$ ps -T -p 306187 -f
UID          PID    SPID    PPID  C STIME TTY          TIME CMD
daemon    306187  306187  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
root      306187  306210  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306187  306211  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
root      306187  306212  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306187  306213  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306187  306214  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306187  306215  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306187  306216  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
root      306187  306217  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306187  306218  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test
root      306187  306219  306168  0 20:55 ?        00:00:00 node /mnt/test.js /tmp/test

...and...

# NodeJS v18
sh@zumpa:~$ ps -T -p 306629 -f
UID          PID    SPID    PPID  C STIME TTY          TIME CMD
daemon    306629  306629  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306650  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306651  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306652  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306653  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306654  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306655  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306656  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306657  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306658  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test
daemon    306629  306659  306606  0 21:05 ?        00:00:00 node /mnt/test.js /tmp/test

The only thing I am surprised about is that it was not discovered and fixed earlier.

from haraka.

DoobleD avatar DoobleD commented on June 17, 2024

Interesting. My previous guess may be wrong: the setuid description linked in the stackoverflow post says that a uid change should apply to all threads according to POSIX.

I'm unsure what's going on. But yeah I would have expected the same behavior as you.

from haraka.

DoobleD avatar DoobleD commented on June 17, 2024

Node is apparently not fully POSIX compliant. Not sure if node's setuid/gid are.

from haraka.

msimerson avatar msimerson commented on June 17, 2024

As was pointed out earlier, running as a non-root user would create issues on any semi-normal *nix system that refuses to permit non-root users from binding to ports less than 1024.

Where we have code that is writing files to disk, and we care about the permissions, a potential solution is to check the ownership of the file immediately after create or append, maybe while we still have the filehandle, and use fs.chown if it doesn't match smtp.ini settings.

from haraka.

DoobleD avatar DoobleD commented on June 17, 2024

Thanks @msimerson. Calls to chown fail with an EPERM error. For some reasons these appear to be executed as the smtp.ini user rather than root, but since the files are written as root...

from haraka.

analogic avatar analogic commented on June 17, 2024

I'm not getting any notifications or replies to my "hidden" bug report, so I've personally downgraded to v18. I still hope someone at Nodejs is working on this, as it makes process.setuid essentially useless and potentially dangerous.

from haraka.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.