solderpunk/molly-brown
6
29
Fork 10
Code Issues 10 Pull Requests Releases Wiki Activity

Support CGI in a secure way #16

New Issue
Closed
opened 2020-12-07 19:58:08 +00:00 by makeworld · 7 comments
makeworld commented 2020-12-07 19:58:08 +00:00
Contributor
Copy Link

On the README:

It is very important to be aware that programs written in Go are unable to reliably change their UID once started, due to how goroutines are implemented on unix systems. As an unavoidable consequence of this, CGI processes started by Molly Brown are run as the same user as the server process. This means CGI processes necessarily have read and write access to the server logs and to the TLS private key. There is no way to work around this. As such you must be extremely careful about only running trustworthy CGI applications, ideally only applications you have carefully written yourself. Allowing untrusted users to upload arbitrary executable files into a CGI path is a serious security vulnerability.

This is tracked in issue 1435 on the Go repo, and will be fixed in Go 1.16. I'm not sure when you want to implement this, as restricting the compiling of the repo to only the latest Go version (once 1.16 comes out) might be annoying for users. Maybe you could have this on a separate branch, or just tell people with older Go versions to only compile early commits.

In any case, I wanted to open this issue to make you aware of the fix, and start the discussion.

On the README: > It is very important to be aware that programs written in Go are *unable* to reliably change their UID once started, due to how goroutines are implemented on unix systems. As an unavoidable consequence of this, CGI processes started by Molly Brown are run as the same user as the server process. This means CGI processes necessarily have read and write access to the server logs and to the TLS private key. There is no way to work around this. As such you must be extremely careful about only running trustworthy CGI applications, ideally only applications you have carefully written yourself. Allowing untrusted users to upload arbitrary executable files into a CGI path is a serious security vulnerability. This is tracked in [issue 1435](https://github.com/golang/go/issues/1435) on the Go repo, and [will be fixed](https://github.com/golang/go/issues/1435#issuecomment-740132407) in Go 1.16. I'm not sure when you want to implement this, as restricting the compiling of the repo to only the latest Go version (once 1.16 comes out) might be annoying for users. Maybe you could have this on a separate branch, or just tell people with older Go versions to only compile early commits. In any case, I wanted to open this issue to make you aware of the fix, and start the discussion.
👍 1
sloum commented 2020-12-22 20:04:01 +00:00
Copy Link

I'm not sure if you have tried this or not, but I found it and thought I'd make the suggestion (I have not tested it):

In dynamic.go, starting on line 50:

// Spawn process
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
cmd := exec.CommandContext(ctx, scriptPath)
cmd.Env = []string{}
for key, value := range vars {
	cmd.Env = append(cmd.Env, key+"="+value)
}
response, err := cmd.Output()

You may be able to change the above to the following:

// Spawn process
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
cmd := exec.CommandContext(ctx, scriptPath)
cmd.Env = []string{}
for key, value := range vars {
	cmd.Env = append(cmd.Env, key+"="+value)
}
// Manually change the uid/gid for the comand, rather than the calling goroutine
// In the following line you will need established vars for uid and gid
// You will also need to import syscall
cmd.SysProcAttr = &syscall.SysProcAttr{}
cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uid, Gid: gid}
response, err := cmd.Output()

Again, not sure if this has been tried or is helpful... but maybe? It seems like this should be supported before 1.16. By nature of it using the syscall it will not work on all operating systems, but should work fine on unix/linux systems I believe.

I'm not sure if you have tried this or not, but I found it and thought I'd make the suggestion (I have not tested it): In dynamic.go, starting on line 50: ``` go // Spawn process ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() cmd := exec.CommandContext(ctx, scriptPath) cmd.Env = []string{} for key, value := range vars { cmd.Env = append(cmd.Env, key+"="+value) } response, err := cmd.Output() ``` You may be able to change the above to the following: ``` go // Spawn process ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() cmd := exec.CommandContext(ctx, scriptPath) cmd.Env = []string{} for key, value := range vars { cmd.Env = append(cmd.Env, key+"="+value) } // Manually change the uid/gid for the comand, rather than the calling goroutine // In the following line you will need established vars for uid and gid // You will also need to import syscall cmd.SysProcAttr = &syscall.SysProcAttr{} cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uid, Gid: gid} response, err := cmd.Output() ```` Again, not sure if this has been tried or is helpful... but maybe? It seems like this should be supported before 1.16. By nature of it using the syscall it will not work on all operating systems, but should work fine on unix/linux systems I believe.
winter commented 2023-01-05 02:52:55 +00:00
Copy Link

It looks like this is fully fixed in Go 1.16:

https://go.dev/doc/go1.16

It'd be great if a solution (maybe similar to the above?) could be implemented. Would be nice to be able to use molly brown to safely serve CGI scripts in the shared hosting environment I'm at.

It looks like this is fully fixed in Go 1.16: https://go.dev/doc/go1.16 It'd be great if a solution (maybe similar to the above?) could be implemented. Would be nice to be able to use molly brown to safely serve CGI scripts in the shared hosting environment I'm at.
solderpunk commented 2023-01-18 18:14:42 +00:00
Owner
Copy Link

Thanks for letting me know, that is great news. I will try to take advantage of this in a not too-distant future release .

Thanks for letting me know, that is great news. I will try to take advantage of this in a not too-distant future release .
nervuri commented 2023-02-15 19:15:33 +00:00
Copy Link

I had a look at at commit 4e6a8fcd05. I'll just point out two things:

  1. Don't forget to set the primary and supplementary groups (setgid and setgroups). Molly Brown, if started as root, can access files with root group read permission even after running DropPrivs.

  2. Using "nobody" is not ideal, since other programs might also use it (see https://wiki.ubuntu.com/nobody). It may be ok as a default value, but I would allow the user to provide another username.

Here's how I tackled this for another project:
https://tildegit.org/nervuri/client-hello-mirror/src/branch/master/drop_privileges.go

I had a look at at commit 4e6a8fcd05e94686925b4294716cf4b4fac92898. I'll just point out two things: 1. Don't forget to set the primary and supplementary groups (setgid and setgroups). Molly Brown, if started as root, can access files with root group read permission even after running DropPrivs. 2. Using "nobody" is not ideal, since other programs might also use it (see https://wiki.ubuntu.com/nobody). It may be ok as a default value, but I would allow the user to provide another username. Here's how I tackled this for another project: https://tildegit.org/nervuri/client-hello-mirror/src/branch/master/drop_privileges.go
👍 1
solderpunk commented 2023-02-15 19:36:27 +00:00
Owner
Copy Link

Thanks for the feedback! I already had it on my TODO list to add a config variable to switch out nobody, but I will admit the groups issue had entirely slipped my mind.

Thanks for the feedback! I already had it on my TODO list to add a config variable to switch out `nobody`, but I will admit the groups issue had entirely slipped my mind.
solderpunk commented 2023-02-15 20:17:52 +00:00
Owner
Copy Link

The unprivileged user is now configurable via UnprivUsername. Need to log off for tonight but will not close this issue until the setgid() stuff is also addressed.

The unprivileged user is now configurable via `UnprivUsername`. Need to log off for tonight but will not close this issue until the `setgid()` stuff is also addressed.
solderpunk commented 2023-02-23 20:04:16 +00:00
Owner
Copy Link

Some implementation details have changed from my above comments (see README), but it is now possible, on unix systems, to use setuid(), set_gid() and set_groups() to change privileges after opening files (when started as root, it drops to a user-specified unprivileged user specified on the command line, when started as a setuid binary it changes from the binary owner to the user who executed it) and also to chroot() Molly Brown into its own corner of the filesystem.

This is not the end-all, be-all of security, but I believe it meets the traditional security expectations of simple web servers which were used for multiuser CGI sites "back in the day", and is a substantial improvement over the previous status quo where Molly supported CGIs but we had to loudly warn people not to use it in what was supposed to be one of the server's main user cases, on a pubnix. I think we're at the point now where doing so wouldn't be nuts, and to put my money where my mouth is the current version of Molly is now running at the Zaibatsu - not on a public port yet as I need to smoothly transition some folk off GeGoBi, but I will eventually be letting sundogs run CGI apps available to the public.

Because we've met this bar I'm going to close this issue, but additional issues and/or PRs related to security are absolutely always welcome. I promise not to take two years next time. 👍 Thanks all for your input!

Some implementation details have changed from my above comments (see README), but it is now possible, on unix systems, to use `setuid()`, `set_gid()` and `set_groups()` to change privileges after opening files (when started as root, it drops to a user-specified unprivileged user specified on the command line, when started as a setuid binary it changes from the binary owner to the user who executed it) and also to `chroot()` Molly Brown into its own corner of the filesystem. This is not the end-all, be-all of security, but I believe it meets the traditional security expectations of simple web servers which were used for multiuser CGI sites "back in the day", and is a substantial improvement over the previous status quo where Molly supported CGIs but we had to loudly warn people not to use it in what was supposed to be one of the server's main user cases, on a pubnix. I think we're at the point now where doing so wouldn't be nuts, and to put my money where my mouth is the current version of Molly is now running at the Zaibatsu - not on a public port yet as I need to smoothly transition some folk off GeGoBi, but I will eventually be letting sundogs run CGI apps available to the public. Because we've met this bar I'm going to close this issue, but additional issues and/or PRs related to security are absolutely always welcome. I promise not to take two years next time. :+1: Thanks all for your input!
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: solderpunk/molly-brown#16
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?