Adds `Process.chroot` #5577

chris-huxtable · 2018-01-11T22:08:21Z

No description provided.

RX14 · 2018-01-11T22:24:35Z

src/process.cr

+    path.check_no_null_byte
+    return if (LibC.chroot(path) == 0 && LibC.chdir("/") == 0)
+
+    case (Errno.value)


This case isn't required, Errno.new calls strerror and checks Errno.value for you:

if LibC.chroot(path) != 0 raise Errno.new("Failed to chroot") end if LibC.chdir("/") != 0 raise Errno.new("Failed to chdir after chroot") end

would be the idiomatic way to write this method.

However, it does raise the issue of error handling, and that Process.chroot is now not atomic. If chroot succeeds but chdir fails, we're in some undefined state that the application is going to be very confused about.

Sija · 2018-01-11T22:45:35Z

src/process.cr

-    when Errno::EIO          then raise Errno.new("An I/O error occurred while reading from or writing to the file system.")
-    when Errno::EPERM        then raise Errno.new("The caller is not the superuser.")
-    else                          raise Errno.new("...")
+    if (LibC.chroot(path) != 0)


parentheses are obsolete here

Sija · 2018-01-11T22:45:42Z

src/process.cr

+      raise Errno.new("Failed to chroot")
+    end
+
+    if (LibC.chdir("/") != 0)


RX14 · 2018-01-12T00:19:09Z

I'm fine on the code for this, but i'm a little concerned about chdir failing (yes, i know, extremely exceptional circumstances) and being left in an inconsistent state.

jkthorne · 2018-03-16T18:28:00Z

If chdir fails should it rollback to the previous directory it was in?

bew · 2018-03-16T18:51:25Z

Maybe we should only keep the chroot in the method, and let the user chdir by himself?

jkthorne · 2018-03-16T19:18:01Z

I don’t know. Seems like that could be unexpected. Cheers, Jack

…

On Mar 16, 2018, at 1:51 PM, Benoit de Chezelles ***@***.***> wrote: Maybe we should only keep the chroot in the method, and let the user chdir by himself? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

chris-huxtable · 2018-03-16T19:22:26Z

chroot should always be followed by a chdir. If it fails it should raise which would kill the proc if it isn't caught. If it is caught the proc should still be killed.

The importance of keeping it in the chroot method is that we have to assume the person using it doesn't know how to use it properly. All the documentation in the world isn't going to prevent that. This forces a response from the developer without producing a silent inconsistent state.

bew · 2018-03-16T19:25:46Z

ok @chris-huxtable didn't know that it was needed, the exception on error looks good, but there's no way to ensure the proc will exit, the user can always catch the exception..

chris-huxtable · 2018-03-16T19:32:18Z

@bew - The only option I can think of that avoids that is to call exit(1) or something similar instead of raising. I am fine with this, but figured a catch could be useful if you were trying to provide better feedback.

RX14 · 2018-03-16T19:45:18Z

I suspect that we'll want to do a "hard abort" in case chdir() after chroot() fails. If the chroot fails we can raise normally, but if the chdir fails, we have to do something like

errno = Errno.new("chdir after chroot failed")
errno.callstack = Callstack.new
errno.inspect_with_backtrace(STDERR)
STDERR.puts "Unresolvable state, exiting..."
Process.exit(1)

RX14 · 2018-04-02T23:38:44Z

src/process.cr

+
+    if LibC.chdir("/") != 0
+      errno = Errno.new("chdir after chroot failed")
+      errno.callstack = Callstack.new


This doesn't compile, should be CallStack. My bad.

It'd be great if you could add some specs for this PR. You'll have to start a new process for testing chroot, but you can copy this: https://github.com/crystal-lang/crystal/blob/master/spec/std/kernel_spec.cr

Thanks. I will have a look at this when I have a bit of spare time.

straight-shoota · 2018-04-06T07:16:41Z

spec/std/process_spec.cr

@@ -2,6 +2,28 @@ require "spec"
 require "process"
 require "tempfile"

+private def build_and_run(code)


If this is the same implementation as in kernel_spec.cr, it would make sense to extract to a spec helper instead of duplicating.

straight-shoota · 2018-04-06T07:35:22Z

spec/std/process_spec.cr

@@ -81,6 +103,21 @@ describe Process do
    $?.exit_code.should eq(0)
  end

+  it "can chroot" do
+    status, output = build_and_run <<-CODE
+      Process.chroot("/var/empty")


You need to ensure this actually exists. I would prefer to have it either in the project folder or /tmp (we need to resolve #5811), but not /var.

/var/empty is a standard location on UNIX filesystems. It is used almost exclusively for chrooting processes which don't need file system access.

You still need to ensure it exists.

straight-shoota · 2018-04-06T07:41:07Z

Chroot requires a privileged user. How would we deal with the failing spec when crystal spec is usually run unprivileged?

chris-huxtable · 2018-04-06T07:50:09Z

BTW It passes if run as root (not that we should be running as root, we shouldn't).

straight-shoota · 2018-04-06T07:51:38Z

src/process.cr

@@ -414,6 +414,27 @@ class Process
  private def close_io(io)
    io.close if io
  end
+
+  # Changes the root directory and the current working directory for the current
+  # process.


It should probably be obvious to everyone using it, but it wouldn't hurt to add a note that chroot should not be used as a security mechanism (especially if the process does not drop privilege).

I have addressed this in 7361ad6.

Didn't need to be so detailed ;) A simple "Chroot should not be used as security measure." would have been fine.

Maybe add that as a first sentence to make it Crystal clear? 😄

I don't think "chroot should not be used as security measure." is correct. It can be used effectively as a mitigation, though its not perfect.

straight-shoota · 2018-04-06T08:00:58Z

How about changing the spec code to succeed if Process.chroot raises Errno with EPERM in normal spec run (=unprivileged).
To test that it actually works with appropriate privileges, we could add a special spec file containing the current spec, which would not be included in a normal spec run. It must not be placed in spec/std, but could be like spec/privileged_spec.cr.

This way it can at least be tested manually running bin/crystal spec spec/privileged_spec.cr as root. Adding this to CI should be a task for a subsequent PR.

Sija · 2018-04-06T09:32:18Z

spec/std/process_spec.cr

+  it "can chroot" do
+    status, output = build_and_run <<-CODE
+      Process.chroot("/var/empty")
+      working_dir = Dir.current()


Redundant ()

It passes 'format'. I prefer the inclusion as it makes it visibly a function. As this is a sense of style I would consider this "bike-shedding".

Let's keep the style used throughout the stdlib, empty () are considered redundant if not needed.

Sija · 2018-04-06T09:33:11Z

spec/std/process_spec.cr

+
+      puts "Wrong Directory" if working_dir != "/"
+      puts "Not a Directory" if !File.directory?(working_dir)
+      puts "Directoyr isn't empty" if !Dir.empty?(working_dir)


Typo: Directoyr

Sija · 2018-04-06T09:36:28Z

src/lib_c/x86_64-portbld-freebsd/c/unistd.cr

@@ -8,6 +8,7 @@ lib LibC
  X_OK       = 0x01
  SC_CLK_TCK =    3

+  fun chroot(dirname : Char*) : Int


Please, use consistent naming: path vs dirname. Former would be preferred.

I have made it path when the source/man pages says path and dirname when the source/man pages says dirname. Generally BSD uses dirname, linux uses `path'.

RX14 · 2018-04-06T16:02:25Z

If we need root to spec it, we can't spec it. I'm sorry for leading you on a wild goose chase but it's probably best to remove the spec :(

Still, a typeof "does it compile" spec is required.

straight-shoota · 2018-04-06T18:13:19Z

@RX14 What speaks about a separate spec file that is usually not run, but can (with root) to verify?

Sija · 2018-04-06T18:36:55Z

src/process.cr

+      errno.callstack = CallStack.new
+      errno.inspect_with_backtrace(STDERR)
+      STDERR.puts "Unresolvable state, exiting..."
+      Process.exit(1)


Shouldn't this use abort("Unresolvable state, exiting...") instead of STDERR.puts + Process.exit?

Since #5413 is merged, it should also set AtExitHandlers.exception = errno. With this in place errno.inspect_with_backtrace(STDERR) is needless.

AtExitHandlers.exception prints Unhandled exception but it is not - and you just can't handle this exception.

But abort would be great.

straight-shoota · 2018-04-06T19:10:37Z

spec/std/process_spec.cr

+        Process.chroot("/var/empty")
+        puts "FAIL"
+      rescue ex : Errno
+        puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: " + (ex.message||"")


Pardon the comment on style, but the string concatenation looks really silly and should not be encouraged. Just use "FAIL: #{ex.message}". Ditto below.

Or maybe simply ex.message

straight-shoota · 2018-04-06T19:12:59Z

src/process.cr

@@ -414,6 +414,27 @@ class Process
  private def close_io(io)
    io.close if io
  end
+
+  # Changes the root directory and the current working directory for the current
+  # process.


Didn't need to be so detailed ;) A simple "Chroot should not be used as security measure." would have been fine.

straight-shoota · 2018-04-06T19:13:42Z

src/process.cr

@@ -414,6 +414,27 @@ class Process
  private def close_io(io)
    io.close if io
  end
+
+  # Changes the root directory and the current working directory for the current
+  # process.


Maybe add that as a first sentence to make it Crystal clear? 😄

straight-shoota · 2018-04-06T19:14:08Z

src/process.cr

+      errno.callstack = CallStack.new
+      errno.inspect_with_backtrace(STDERR)
+      STDERR.puts "Unresolvable state, exiting..."
+      Process.exit(1)


But abort would be great.

straight-shoota · 2018-04-06T19:16:52Z

About the failing spec: I told you so 😛 #5577 (comment)

chris-huxtable · 2018-04-06T19:25:18Z

@straight-shoota - I am perplexed. I took another dive into Linux last week. When I couldn't find ifconfig I threw up my hands and went back to OpenBSD. Now I find out it doesn't have /var/empty/. BSD Forever!

chris-huxtable · 2018-04-06T21:50:45Z

So the one failure appears to have occurred due to a failure to allocate memory. Is there a way to re-run it?

Sija · 2018-04-06T22:10:12Z

spec/std/process_spec.cr

+        Process.chroot("/usr")
+        puts "FAIL"
+      rescue ex : Errno
+        puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: " + (ex.message||"")


puts (ex.errno == Errno::EPERM) ? "Success" : "FAIL: #{ex.message}"

ditto below

There appears to be a bug or something in the language (or I am missing something). It wont allow that. Says "undefined local variable or method 'ex'". Also tried:

puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: #{ex.message||"No Message"}"

to no avail.

#{ex.message} will be interpolated in the heredoc. You need to either escape the hash or use a non-interpolating heredoc (<<-'CODE' header) to use interpolation in the source string.

RX14 · 2018-04-07T10:21:13Z

@chris-huxtable I have /var/empty/ on my distro... Also the reason why we can't remove redundant () empty brackets on some calls in the formatter is just a technical reason. They should be removed, and this is a perfect candidate for someone starting a style guide saying this.

chris-huxtable · 2018-04-07T17:26:12Z

Is there a problem with the Travis CI? It's failed several times on random things.

chris-huxtable · 2018-04-07T17:27:51Z

@RX14 - It now has a spec and is ready for review.

RX14

Good enough

chris-huxtable · 2018-04-07T20:36:29Z

Rebased and combined commits.

RX14

Just what @straight-shoota said about not duplicating the spec helper than we're good to go.

Sija · 2018-04-09T14:50:19Z

spec/std/process_spec.cr

+        Process.chroot("/usr")
+        puts "FAIL"
+      rescue ex : Errno
+        puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: \#{ex.message||"No Message"}"


Please, remove whitespaces after ( & before )

||"No Message" part here is redundant, #{ex.message} is enough. ditto below

Would be a tad cleaner to use non-interpolated heredoc (<<-'CODE') as suggested by @straight-shoota, instead of escaping interpolation

@Sija - We very much need a style guide. You seem interested in compelling a specific style. Perhaps you can draft a style guide that can be discussed and adopted. I am not throwing shade; I also have strong style opinions.

The space before and after brackets formats. So based on previously discussed standards of 'correctness' it should be fine unless otherwise declared not to be. I am going to change it this time but I think this is an unnecessarily micro management of style.

@chris-huxtable we have a style guide right now - crystal tool format. This code doesn't format. Feel free to ignore the rest of what @Sija recommended.

I don't care that it doesn't format because it's inside a string, inside a the spec suite, and it doesn't break the build, and honestly who cares, but it seems some people do.

straight-shoota

Looks GTG!

straight-shoota · 2018-04-10T23:14:06Z

I just remembered there is already an (apparently undocumented) folder spec/manual. Could place the privileged spec there so it can at least be verified manually.

RX14

Finally!

RX14 requested changes Jan 11, 2018

View reviewed changes

Sija reviewed Jan 11, 2018

View reviewed changes

src/process.cr Outdated

raise Errno.new("Failed to chroot")

end

if (LibC.chdir("/") != 0)

Copy link

Contributor

Sija Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

chris-huxtable force-pushed the chroot branch from f2018e5 to 2f4a44b Compare January 11, 2018 22:52

RX14 approved these changes Jan 12, 2018

View reviewed changes

RX14 requested changes Apr 2, 2018

View reviewed changes

chris-huxtable force-pushed the chroot branch from d2e4f75 to f046cfd Compare April 6, 2018 06:38

straight-shoota reviewed Apr 6, 2018

View reviewed changes

Sija reviewed Apr 6, 2018

View reviewed changes

straight-shoota reviewed Apr 6, 2018

View reviewed changes

Sija reviewed Apr 6, 2018

View reviewed changes

RX14 approved these changes Apr 7, 2018

View reviewed changes

chris-huxtable force-pushed the chroot branch from c23c800 to 40fcca4 Compare April 7, 2018 20:34

chris-huxtable changed the title ~~Adds chroot to Process~~ Adds chroot to Process Apr 7, 2018

chris-huxtable changed the title ~~Adds chroot to Process~~ Adds Process.chroot Apr 7, 2018

jhass approved these changes Apr 9, 2018

View reviewed changes

RX14 requested changes Apr 9, 2018

View reviewed changes

Sija reviewed Apr 9, 2018

View reviewed changes

chris-huxtable added 3 commits April 9, 2018 11:58

Moves build_and_run to spec_helper

0ced529

Adds chroot bindings

dab91ff

Adds chroot to Process

8643263

chris-huxtable force-pushed the chroot branch from 40fcca4 to 8643263 Compare April 9, 2018 15:59

straight-shoota approved these changes Apr 9, 2018

View reviewed changes

RX14 approved these changes Apr 11, 2018

View reviewed changes

sdogruyol approved these changes Apr 14, 2018

View reviewed changes

sdogruyol added topic:stdlib kind:feature labels Apr 14, 2018

sdogruyol merged commit 913cd8d into crystal-lang:master Apr 14, 2018

sdogruyol added this to the Next milestone Apr 14, 2018

Adds Process.chroot #5577

Adds Process.chroot #5577

Conversation

chris-huxtable commented Jan 11, 2018

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

RX14 commented Jan 12, 2018

jkthorne commented Mar 16, 2018

bew commented Mar 16, 2018

jkthorne commented Mar 16, 2018 via email

chris-huxtable commented Mar 16, 2018

bew commented Mar 16, 2018

chris-huxtable commented Mar 16, 2018

RX14 commented Mar 16, 2018 • edited

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

chris-huxtable Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Apr 6, 2018

chris-huxtable commented Apr 6, 2018

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

straight-shoota commented Apr 6, 2018

Choose a reason for hiding this comment

chris-huxtable Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Apr 6, 2018

straight-shoota commented Apr 6, 2018 • edited

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

Choose a reason for hiding this comment

straight-shoota commented Apr 6, 2018

chris-huxtable commented Apr 6, 2018

chris-huxtable commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Apr 7, 2018

chris-huxtable commented Apr 7, 2018 • edited

chris-huxtable commented Apr 7, 2018

RX14 left a comment

Choose a reason for hiding this comment

chris-huxtable commented Apr 7, 2018

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-huxtable Apr 9, 2018 • edited

Choose a reason for hiding this comment

RX14 Apr 9, 2018 • edited

Choose a reason for hiding this comment

straight-shoota left a comment

Choose a reason for hiding this comment

straight-shoota commented Apr 10, 2018

RX14 left a comment

Choose a reason for hiding this comment

Adds `Process.chroot` #5577

Adds `Process.chroot` #5577

RX14 commented Mar 16, 2018 •

edited

chris-huxtable Apr 6, 2018 •

edited

chris-huxtable Apr 6, 2018 •

edited

Sija Apr 6, 2018 •

edited

straight-shoota commented Apr 6, 2018 •

edited

chris-huxtable commented Apr 7, 2018 •

edited

chris-huxtable Apr 9, 2018 •

edited

RX14 Apr 9, 2018 •

edited