Bug: xstr evaluated in incorrect binding

[numinit]

Currently, xstr handing directly evaluates the xstr. Would there be a way to evaluate this like any other method in the context of the receiving object?

This would enable people to redefine the backtick method in the context of the receiver, which could be useful. I see that line 156 of sandbox.rb currently evaluates it directly, which ignores any overrides that could be present in the source binding.

I've taken a look through evalhook and it doesn't look like handle_xstr is passed the original receiver. Maybe something like the original_receiver object used in process_call could be used in process_(d)xstr? I'm not familiar with how the s-expressions are laid out, though.

[numinit]

Might have found a fix, actually.

edit: nope, not threadsafe in any way.

Still need to figure out a way to get the binding we're eval-ing code in into the hook handler methods.

[tario]

You are talking about hooking something it is not a method call, that is out of scope of this gem. But a nice idea for another gem

I am thinking about a gem allowing to rewrite any node on sexp tree (that is what evalhook is doing, but is specific to method calls and xstr), in the while I will think a quick fix to allow you implement the feature you are looking for

[tario]

I think your problem is too specific, but I can make generic changes to support this kind of things

The key is here:
https://github.com/tario/evalhook/blob/master/lib/evalhook.rb#L75

The preprocess method control how the sexp is processed before it is evaluated (on this case, validation code is prepend to method calls), the next thing to do is to expose new methods on evalhook/shikashi to allow add custom processor there. Also including syntax sugar to allow easily sexp rewriting rules

[numinit]

The solution I thought up was storing the binding as state in the sandbox, then replacing line 156 with something like:

sandbox.get_current_binding.eval "`#{str}`"

However, this wouldn't be threadsafe.

The problem I was trying to fix was the discarding of binding in the call of Kernel#`.

[tario]

Sorry, it doesn't make sense to me on my design. Sandboxing is about allowing or disallowing things, not about hooking or extending env/language behavoir

I will implement another separated feature to support this

[numinit]

IMO it's still a bug, and deviates from Ruby standard behavior.

Consider the following code, where we redefine the method and evaluatewhoami` in its binding:

require 'shikashi'

class Context
  def get_binding
    binding
  end
end

class ContextOne < Context
end

class ContextTwo < Context
  # reassign it here -- standard method override
  # in this example version, we just return a reversed string with a call to ` or %x[].
  def ` str
    str.reverse
  end
end

ctx1 = ContextOne.new
ctx2 = ContextTwo.new

# default ruby behavior
puts "ctx1, stock ruby: " + ctx1.get_binding.eval('`whoami`')
puts "ctx2, stock ruby: " + ctx2.get_binding.eval('`whoami`')

# behavior when using shikashi
sandbox = Shikashi::Sandbox.new
privs   = Shikashi::Privileges.new
privs.allow_xstr

puts "ctx1: shikashi: " + sandbox.run('`whoami`', privileges: privs, binding: ctx1.get_binding)
puts "ctx2: shikashi: " + sandbox.run('`whoami`', privileges: privs, binding: ctx2.get_binding)

On ruby 2.1.2p95, this returns:

ctx1, stock ruby: arctus
ctx2, stock ruby: imaohw
ctx1: shikashi: arctus
ctx2: shikashi: arctus

In any case, thanks for your help on this, even if it has to be separate.

[tario]

if this is a bug, then should be fixed. I think overriding tick method is a new thing on ruby 2.1, that is why I missed it

I will handle this as a bug to fix, not related with all the other things I said on previous comment

[numinit]

Let me check about the 1.9 series too

[numinit]

On ruby 1.9.3p0, same result:

ctx1, stock ruby: arctus
ctx2, stock ruby: imaohw
ctx1: shikashi: arctus
ctx2: shikashi: arctus

Let me know what help you need if you want to fix this (if any)

[tario]

More info: http://devblog.avdi.org/2010/12/20/overriding-the-backtick-in-ruby/

[numinit]

Any updates on this? I've got some ideas, but it might involve adding a binding parameter to several methods in both evalhook and shikashi.

[tario]

I don't think that modification is necessary to fix this, I think it should be more simpler that that.
I have no time to fix this, but I will suggest changing only on evalhook tree processor, here:
https://github.com/tario/evalhook/blob/master/lib/evalhook/tree_processor.rb#L130

    def process_xstr(tree)
      args = s(:arglist, s(:lit, tree[1]) )
      s(:call, hook_handler_reference, :hooked_xstr, args)
    end

It replaces xstr nodes (backticks) with a call to hooked_xstr, in order to fix that, what I would do is to replace with an if instead, ok, this wouldn't be "hooking xstr" , but it would work as expected and will let you control the override of xstr.

Besides all of this, I am not really sure if is secure to allow xstr, remember you are allowing to run ANY system command, maybe a overriding handle_xstr would suit better to your situation. This is what I would do (without changing any gem):

require "shikashi"
class MyHookHandler < Shikashi::EvalhookHandler
  def handle_xstr(str)
     # define your logic here, for example filter(?) forbidden shell commands
     # throw SecurityError exception from here
  end
end

class MySandbox < Shikashi::Sandbox
  def instantiate_evalhook_handler
      newhookhandler = MyHookHandler.new
      @hook_handler_list << newhookhandler
      newhookhandler     
  end
end
sandbox = MySandbox.new
... # define privileges, etc... ...

sandbox.run("`ls -l`") # will call your handle_xstr

Of course, that was a monkey patching hack, but I will add a better way of exposing this feature on shikashi (that would be custom security rules by hooking). If the monkey patching works for your needs then I will postpone this bug in favor or implementing that new feature (anyway, this xstr issue is still a bug and will remain open until I get to fix it)