Is there a glaring security hole in this Ruby wrapper around GPG?

Question

The code:

require 'tempfile'
require 'open3'

def valid_pgp_key?(unsafe_user_input)

  tempfile = Tempfile.new('pgp_pubkey')

  command = ['gpg',
             '--no-default-keyring',
             '--keyring',
             tempfile.path,
             '--import',
             '-']

  stdin, stderrout, thread = Open3.popen2e(command)

  stdin.puts(unsafe_user_input)
  stdin.close

  exit_status = thread.value
  tempfile.unlink

  exit_status.success? ? true : false
end

unsafe_user_input is a completely unsanitized string that we're hoping is a valid PGP public key (but accept that it could validly be a secret key).

Questions:

• I am of the understanding that passing the command as an array calls execve() directly, so there's no need to worry about shell injection - is this true?
• Is it obviously possible for a malicious user to exploit this approach to key validation, and if so what workaround do you suggest?

Answer 1

Hm.. It seems to be very simple and not robust enough, but let's look at it this way:

1. GPG's commandline params end with single -, so IIRC GPG assumes all following as data and there's no way it could interpret it as its options
2. it's stdin of precisely GPG process, not whole shell. If injection succeeded, it could hardly do anything other than running the GPG in an odd way. So even if attack succeeds, if GPG is implemented properly and does not expose any exploits on its own, it will achieve nothing more than failing the GPG task. (And I'd assume that GPG is checked better than Ruby's runtime and the above-script itself, so that seems not very likely)
3. the command and execve part seems to me a nonsense. The Command consists of some strings plainly visible in the code, and a temppath, which is generated automatically in a standard way. Everything is either constant or not-user-depended at all, so the command is as safe as the constant parameters for GPG are.
4. The only thing that comes from the user is the unsafe_input that is passed to stdin. And it is passed via puts, so as stringized form, to standard input. IIRC, it will end up in to_s provided by the unsafe_input object, but it will still end up in writing string-data to the pipe, which already is treated by GPG as data. Again, it's just as safe as GPG and Ruby runtime are.

So, for me, this script is "relatively safe" - "safe" because that's simple code and I can read it and I don't see any issues - and "relatively" comes only from the fact that I don't perceive myself as an expert, neither in Ruby, nor in security.

I can say I trust in GPG's code. If I were looking for some exploits, I'd probably look for issues, buffer overruns, etc in the Ruby runtime it self. I bet it's less checked than GPG. I.e. what if the user-provided data has ill-formed character encoding and improper length? Will puts on stream/pipe go crazy? etc. Also, I see a lot of other more or less wicked possibilities: let's inject customized Open3.popen2e implementation on the fly, and off the safety goes. But aside from such things, if we're not allowing the "user" to load any of his code, if I'm using Ruby to do the job, and if I trust its runtime - I see no issues here.