-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds Process.chroot
#5577
Adds Process.chroot
#5577
Conversation
src/process.cr
Outdated
path.check_no_null_byte | ||
return if (LibC.chroot(path) == 0 && LibC.chdir("/") == 0) | ||
|
||
case (Errno.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
src/process.cr
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parentheses are obsolete here
src/process.cr
Outdated
raise Errno.new("Failed to chroot") | ||
end | ||
|
||
if (LibC.chdir("/") != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
f2018e5
to
2f4a44b
Compare
I'm fine on the code for this, but i'm a little concerned about |
If |
Maybe we should only keep the |
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.
|
The importance of keeping it in the |
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.. |
@bew - The only option I can think of that avoids that is to call |
I suspect that we'll want to do a "hard abort" in case errno = Errno.new("chdir after chroot failed")
errno.callstack = Callstack.new
errno.inspect_with_backtrace(STDERR)
STDERR.puts "Unresolvable state, exiting..."
Process.exit(1) |
src/process.cr
Outdated
|
||
if LibC.chdir("/") != 0 | ||
errno = Errno.new("chdir after chroot failed") | ||
errno.callstack = Callstack.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will have a look at this when I have a bit of spare time.
spec/std/process_spec.cr
Outdated
@@ -2,6 +2,28 @@ require "spec" | |||
require "process" | |||
require "tempfile" | |||
|
|||
private def build_and_run(code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the same implementation as in kernel_spec.cr
, it would make sense to extract to a spec helper instead of duplicating.
spec/std/process_spec.cr
Outdated
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/var/empty
is a standard location on UNIX filesystems. It is used almost exclusively for chrooting processes which don't need file system access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to ensure it exists.
Chroot requires a privileged user. How would we deal with the failing spec when |
BTW It passes if run as root (not that we should be running as root, we shouldn't). |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed this in 7361ad6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't need to be so detailed ;) A simple "Chroot should not be used as security measure." would have been fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add that as a first sentence to make it Crystal clear? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
How about changing the spec code to succeed if This way it can at least be tested manually running |
spec/std/process_spec.cr
Outdated
it "can chroot" do | ||
status, output = build_and_run <<-CODE | ||
Process.chroot("/var/empty") | ||
working_dir = Dir.current() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the style used throughout the stdlib, empty ()
are considered redundant if not needed.
spec/std/process_spec.cr
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Directoyr
@@ -8,6 +8,7 @@ lib LibC | |||
X_OK = 0x01 | |||
SC_CLK_TCK = 3 | |||
|
|||
fun chroot(dirname : Char*) : Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use consistent naming: path
vs dirname
. Former would be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'.
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. |
@RX14 What speaks about a separate spec file that is usually not run, but can (with root) to verify? |
src/process.cr
Outdated
errno.callstack = CallStack.new | ||
errno.inspect_with_backtrace(STDERR) | ||
STDERR.puts "Unresolvable state, exiting..." | ||
Process.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use abort("Unresolvable state, exiting...")
instead of STDERR.puts + Process.exit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #5413 is merged, it should also set AtExitHandlers.exception = errno
. With this in place errno.inspect_with_backtrace(STDERR)
is needless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AtExitHandlers.exception
prints Unhandled exception
but it is not - and you just can't handle this exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But abort
would be great.
spec/std/process_spec.cr
Outdated
Process.chroot("/var/empty") | ||
puts "FAIL" | ||
rescue ex : Errno | ||
puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: " + (ex.message||"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon the comment on style, but the string concatenation looks really silly and should not be encouraged. Just use "FAIL: #{ex.message}"
. Ditto below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe simply ex.message
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't need to be so detailed ;) A simple "Chroot should not be used as security measure." would have been fine.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add that as a first sentence to make it Crystal clear? 😄
src/process.cr
Outdated
errno.callstack = CallStack.new | ||
errno.inspect_with_backtrace(STDERR) | ||
STDERR.puts "Unresolvable state, exiting..." | ||
Process.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But abort
would be great.
About the failing spec: I told you so 😛 #5577 (comment) |
@straight-shoota - I am perplexed. I took another dive into Linux last week. When I couldn't find |
So the one failure appears to have occurred due to a failure to allocate memory. Is there a way to re-run it? |
spec/std/process_spec.cr
Outdated
Process.chroot("/usr") | ||
puts "FAIL" | ||
rescue ex : Errno | ||
puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: " + (ex.message||"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
puts (ex.errno == Errno::EPERM) ? "Success" : "FAIL: #{ex.message}"
ditto below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#{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.
@chris-huxtable I have |
Is there a problem with the Travis CI? It's failed several times on random things. |
@RX14 - It now has a spec and is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough
Rebased and combined commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just what @straight-shoota said about not duplicating the spec helper than we're good to go.
spec/std/process_spec.cr
Outdated
Process.chroot("/usr") | ||
puts "FAIL" | ||
rescue ex : Errno | ||
puts ( ex.errno == Errno::EPERM ) ? "Success" : "FAIL: \#{ex.message||"No Message"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 format
s. 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks GTG!
I just remembered there is already an (apparently undocumented) folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally!
No description provided.