fix(cli) handle race condition in changing env perms #3057

p0pr0ck5 · 2017-11-30T00:39:01Z

Summary

Handle a potential vulnerability in how we secured the Kong env file. Previously we chmod'd the file after writing the contents via penlight's wrapper of the Lua io lib, which in turn uses stdio. This allowed for a miniscule window in which the file contents could be read (fopen() creates files with 0666 octal permissions, as modified by the processes's umask).

The new behavior is to leverage the open() libc wrapper directly via FFI, which allows us to create the file with appropriate permissions from the outset.

Full changelog

Open, write, and close the .kong_env file via libc wrappers around open/write/read syscalls (via FFI)

thibaultcha · 2017-11-30T00:41:10Z

kong/cmd/utils/prefix_handler.lua

  if not ok then
-    log.warn("Unable to set kong env permissions: ", err)
+    return false, err


This function returns nil on error. Could we preserve this behavior?

Ah yep, my mistake.

thibaultcha · 2017-11-30T00:45:01Z

kong/plugins/file-log/handler.lua

@@ -17,10 +17,6 @@ local S_IROTH = system_constants.S_IROTH()
 local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
 local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)

-ffi.cdef[[
-int write(int fd, const void * ptr, int numbytes);


It is a bold bet to move this away from this plugin and into a remote "utility" module. A future change could easily break this, say if the core splits the utils.lua module and the FFI cdef isn't loaded anymore on the code path followed by the core up to the file-log plugin execution... Ideally this would be standardized and implemented in a more robust fashion (Plugins API?) so that the core comes with an FFI cdef definition that other plugins are assured they can use.

Thoughts?

There is precedent for moving FFI cdefs into this module (we did this when we update the random bytes function to read from urandom). Having said that, I'm not opposed to moving all cdefs into a separate module. Currently we only call ffi.cdef in this module. I'm sure this could be standardized and implemented robustly as part of a plugins API. Is that the scope here? I can create a separate module like kong/tools/cdefs.lua or some such if that would help assuage your concerns here.

Could we just ffi.cdef the write function is both places and avoid this issue altogether? Makes the plugin more self-contained (and we'll revisit the topic of exported functionality anyway when we do the Plugin API and do the eventual cleanup if it's the case). AFAIK (and my quick tests showed) LuaJIT doesn't have a problem with duplicated cdefs.

@hishamhm no, duplicating the cdef is illegal. This is why we moved the open() and close() defs in #2536

Ouch, is it? I didn't find it in the docs and didn't seem to bark on my tests, but I know all safety guarantees are off the window when it comes to the FFI.

Still, this looks real odd: this means when using two third-party FFI-based modules, one can only hope that they are not using the same C functions internally? Ew.

Ok, I must be sniffing glue. calling cdef in the module scope lets require take care of not executing the def multiple times. I swear I didn't (did) have the problem before... ill revert this change and do some more digging

calling cdef in the module scope lets require take care of not executing the def multiple times.

What I meant was not two modules calling the same FFI-using module, but two modules calling two different third-party FFI modules which happen to declare open (require won't protect that).

p0pr0ck5 · 2017-12-02T01:08:58Z

(update PR name and synopsis based on rv comments)

thibaultcha · 2017-12-02T02:01:21Z

kong/tools/utils.lua

@@ -48,6 +48,7 @@ const char *ERR_reason_error_string(unsigned long e);

 int open(const char * filename, int flags, int mode);
 size_t read(int fd, void *buf, size_t count);
+int write(int fd, const void * ptr, int numbytes);


Slightly pedantic, but eventually we should probably agree upon asterisk placement for pointers in the function prototypes we put in ffi.cdef.

Ah yep. This was a copypasta. I agree, I don't like the whitespace between the defer op and the symbol. Fixed.

thibaultcha · 2017-12-02T02:08:59Z

kong/cmd/utils/prefix_handler.lua

+  local c = require "lua_system_constants"
+
+  local flags = bit.bor(c.O_CREAT(), c.O_WRONLY())
+  local mode  = bit.bor(c.S_IRUSR(), c.S_IWUSR(), c.S_IRGRP())


pedantic as well since we are dealing with cold code path for now, but since we are doing a require and identical bitwise computation on every call, maybe we could cache those in a lexical do .. end block around the function. Not a blocker though of course...

So this should be compared as a performance vs. readability condition. I'd argue the current format is more readable (what I was going for); performance is not a concern here at all. Happy to adjust this this if needed.

Oh really? I personally find it more readable if the function's body is only executing code based on its input, and if all invariant code is left out. But no, no performance concern here, so I think this is fine as is if you prefer this form 👍

Pedantic comment:

maybe we could cache those in a lexical do .. end block around the function

This is not a cache, but a localization trick for the compiler (transpiler?), to my understanding. To get a bit pedantic, I would say a do ... end block is harder to read, mentally, so the current form is acceptable. But, I am 100% on board with writing referential code, e.g., code that can be used for style/perf references in the future, so I'm happy to make adjustments. We're getting quite "weedy" here? A good point for discussions, though 👍 I have no strong feeling either way, given this is a cold path and we are noting non-functional differences.

But, I am 100% on board with writing referential code

Yep, this is very much my goal most of the time when pointing those out.

thibaultcha · 2017-12-02T02:09:59Z

kong/cmd/utils/prefix_handler.lua

+  local fd = ffi.C.open(path, flags, mode)
+  if fd < 0 then
+    local errno = ffi.errno()
+    return nil, "Unable to open env path " .. path .. " (" ..


Ok, this is going to be very pedantic, but: this is the only capitalized error message returned by this function. Other messages are lower-cased (as are or should be all other error messages in the codebase).

I said it would be pedantic! 😁

Fixed! Nice 👀. We should indeed be consistent. Solid review 👍

This commit handles a potential vulnerability in how we secured the Kong env file. Previously we chmod'd the file after writing the contents via penlight's wrapper of the Lua io lib, which in turn uses stdio. This allowed for a miniscule window in which the file contents could be read (fopen() creates files with 0666 octal permissions, as modified by the processes's umask). The new behavior is to leverage the open() libc wrapper directly via FFI, which allows us to create the file with appropriate permissions from the outset.

thibaultcha

LGTM!

p0pr0ck5 · 2017-12-02T02:40:50Z

Merging. I think the comments here serve as a good reference point for discussion. Glad to see this in :)

p0pr0ck5 requested review from thibaultcha, hishamhm and kikito November 30, 2017 00:41

p0pr0ck5 added the pr/status/please review label Nov 30, 2017

thibaultcha reviewed Nov 30, 2017

View reviewed changes

p0pr0ck5 force-pushed the fix/cli-env-perms branch 2 times, most recently from b835a91 to 6e82457 Compare December 1, 2017 17:57

p0pr0ck5 changed the title ~~Fix/cli env perms~~ fix(cli) handle race condition in changing env perms Dec 2, 2017

thibaultcha reviewed Dec 2, 2017

View reviewed changes

p0pr0ck5 force-pushed the fix/cli-env-perms branch from 6e82457 to 932ec4c Compare December 2, 2017 02:03

thibaultcha reviewed Dec 2, 2017

View reviewed changes

p0pr0ck5 force-pushed the fix/cli-env-perms branch from 932ec4c to fada84e Compare December 2, 2017 02:13

thibaultcha approved these changes Dec 2, 2017

View reviewed changes

thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/please review labels Dec 2, 2017

p0pr0ck5 merged commit 3729700 into master Dec 2, 2017

p0pr0ck5 deleted the fix/cli-env-perms branch December 2, 2017 02:41

p0pr0ck5 restored the fix/cli-env-perms branch December 2, 2017 02:41

p0pr0ck5 deleted the fix/cli-env-perms branch December 2, 2017 02:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli) handle race condition in changing env perms #3057

fix(cli) handle race condition in changing env perms #3057

p0pr0ck5 commented Nov 30, 2017 •

edited

Loading

thibaultcha Nov 30, 2017

p0pr0ck5 Nov 30, 2017

thibaultcha Nov 30, 2017 •

edited

Loading

p0pr0ck5 Nov 30, 2017

hishamhm Nov 30, 2017

p0pr0ck5 Nov 30, 2017

hishamhm Dec 1, 2017 •

edited

Loading

p0pr0ck5 Dec 1, 2017 •

edited

Loading

hishamhm Dec 2, 2017

p0pr0ck5 commented Dec 2, 2017

thibaultcha Dec 2, 2017

p0pr0ck5 Dec 2, 2017 •

edited

Loading

thibaultcha Dec 2, 2017

p0pr0ck5 Dec 2, 2017

thibaultcha Dec 2, 2017

p0pr0ck5 Dec 2, 2017

thibaultcha Dec 2, 2017

thibaultcha Dec 2, 2017

p0pr0ck5 Dec 2, 2017

thibaultcha left a comment

p0pr0ck5 commented Dec 2, 2017

fix(cli) handle race condition in changing env perms #3057

fix(cli) handle race condition in changing env perms #3057

Conversation

p0pr0ck5 commented Nov 30, 2017 • edited Loading

Summary

Full changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamhm Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

p0pr0ck5 Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0pr0ck5 commented Dec 2, 2017

Choose a reason for hiding this comment

p0pr0ck5 Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha left a comment

Choose a reason for hiding this comment

p0pr0ck5 commented Dec 2, 2017

p0pr0ck5 commented Nov 30, 2017 •

edited

Loading

thibaultcha Nov 30, 2017 •

edited

Loading

hishamhm Dec 1, 2017 •

edited

Loading

p0pr0ck5 Dec 1, 2017 •

edited

Loading

p0pr0ck5 Dec 2, 2017 •

edited

Loading