Add Process.quote and fix shell usages in the compiler #9043

oprypin · 2020-04-11T13:27:08Z

Add a method to produce a command line string from a list of args.
Examples:

files = ["my file.txt", "another.txt"]
`grep -E 'fo+' -- #{Process.quote(files)}`

Process.quote_posix(files) # => "'my file.txt' another.txt"

Process.quote_windows({ %q(C:\"foo" project.txt) }) # => %q("C:\\\"foo\" project.txt")

Yes, all of these examples could've been written better without shell: true but the reality is that sometimes one needs to intermix shell handling and non-shell handling, the best example of which is link_flags in the compiler, which I am also touching here (search for it in the diff!).

The source code of Process.quote_windows is required for the upcoming Process.new port on Windows.
Sure, it doesn't have to be public for that case, but I found another use case for it in the compiler (search for it in the diff!), so why not publish it.

I'm intentionally not hiding quote_posix in something like Crystal::System::Process because it can have more uses (in fact, I used it immediately in the compiler here). E.g. one might want to tell a user to run a Bash command specifically and make sure it's correct and copy-pasteable, and there's no reason that the program should not be able to do that on Windows.

Both quote_posix and quote_windows are available on all systems, then quote just selects for the current OS.

~~Removing the usage of Process.new(, args, shell: true) from the compiler is also a requirement for Windows porting, as this combination won't be supported.~~

Additionally, I did a quick search throughout the codebase for shell misuses which would surely fail eventually (with likely security vulnerabilities!) if any atypical path was involved, and applied the new method there. This just tends to happen more often on Windows, which made me notice.

src/process/shell.cr

oprypin · 2020-04-11T14:18:25Z

Mmm no idea what's up with CI today. Surely some of these failures are not real

RX14 · 2020-04-11T14:48:32Z

I don't like the amount of Process.shell_quote used everywhere. It feels sort of like manual SQL escaping: super easy to miss one, and overly verbose. That was replaced with prepared queries, which is basically what Process.new("command", args, shell: true) does: you can use $1, $2, $@ etc.

RX14 · 2020-04-11T14:49:31Z

github's CI is broken today, the others are fine.

oprypin · 2020-04-11T14:50:03Z

don't like the amount of Process.shell_quote used everywhere

Feel free to rework all of these cases separately. I'm not adding a new problem with this.

src/compiler/crystal/compiler.cr

straight-shoota · 2020-04-11T15:34:38Z

Looks good 👍

@RX14 This might be a bit controversial, but we could consider calling shell_quote implicitly for string interpolation in ` literals.
That's probably the intended usage in most cases. Constructing shell commands with unquoted components would no longer work this way. For that you'd have to manually construct the command string first.
It would also only apply to ` literals, leaving similar methdods like Process.run, system etc. behave differently. This could cause unnoticed security issues when switching from "safe" ` literals to other methods because the same code would implicitly turn unsafe.

oprypin · 2020-04-11T15:37:02Z

So the CI problem is real. Reproduced it locally with

make
CC="cc -fuse-ld=lld"  ./bin/crystal build --threads 1  --exclude-warnings spec/std --exclude-warnings spec/compiler -o .build/std_spec spec/std_spec.cr

Now the final linker command is passed all as one arg to the shell, and, being 162174 bytes long, it surpasses MAX_ARG_STRLEN (131072). This limitation is separate from ARG_MAX (2092454) for the sum of all lengths.

While this is cause to reconsider this approach, I'd like to point out that the limit on the commands is much much smaller on Windows so we probably want to limit this command's size anyway, for example by not linking so many separate files (almost 4000 in this case) at once.

oprypin · 2020-04-11T16:13:20Z

I backed out of the change that caused the CI failure, and what I'm guessing is the main source of worry from @RX14 's comment

oprypin · 2020-04-12T16:35:49Z

I realized that this Windows quoting method works only for CreateProcess, it's not good enough for the shell.
There actually doesn't seem to be any escaping that's always good enough for CMD, e.g. there's no way to insert a newline.

I renamed the methods accordingly and :nodoc:'d the Windows-specific one.

So I'll just keep this method reserved for internal use with shell: true, which on Windows I intend to not actually put through any shell. We can postpone this PR.

oprypin · 2020-04-14T23:03:36Z

So this change is no longer blocking Windows support, but it'd still be really nice to have.

And apply it throughout the compiler for better safety.

Sija · 2020-04-16T11:53:25Z

src/process/shell.cr

+  # Shell-quotes one item, same as `quote({arg})`.
+  def self.quote(arg : String) : String
+    quote({arg})
+  end


Suggested change

# Shell-quotes one item, same as `quote({arg})`.

def self.quote(arg : String) : String

quote({arg})

end

# Shell-quotes given items, same as `quote(*args)`.

def self.quote(*args : String) : String

quote(args)

end

You can be sure that I considered this option, it's not like I didn't know about it.

But I'm wary about the effect of proliferating these splats on the compilation times.

And I also want to stress the case of quoting one item as something that one may need specifically. If there are multiple items, it's nice to make the user think, is this a List situation or is it a Tuple situation.

I don't know. I don't feel that strongly about this though.

src/process/shell.cr

straight-shoota · 2020-04-11T15:26:19Z

src/process/shell.cr

+    args.join(' ') do |arg|
+      if arg.empty?
+        "''"
+      elsif arg.matches? %r([^a-zA-Z0-9%+,\-./:=@_]) # not all characters are safe, needs quoting


IIRC % and : can have special meaning in posix shell.

Not as far as I'm aware.

There's the command ":" but that's not affecting anything.
And oh huh, guess if one wanted to run an executable called "%" (which is in PATH) in Bash, they wouldn't be able to. That's Bash though, not POSIX.

the following may need to be quoted under certain circumstances. That is, these characters may be special depending on conditions described elsewhere in this volume of IEEE Std 1003.1-2001:

* ? [ # ˜ = %

https://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html

It doesn't give more detail about the circumstances. So maybe it's okay.

oprypin · 2020-04-24T17:22:18Z

Just found in the wild
https://github.com/crystal-lang/shards/search?q=escape

straight-shoota · 2020-04-24T22:10:21Z

spec/std/process_spec.cr

+  describe "quote_windows" do
+    it { Process.quote_windows("").should eq %("") }
+    it { Process.quote_windows(" ").should eq %(" ") }
+    it { Process.quote_windows(orig = "%hi%").should eq orig }


Is this really correct? Wouldn't %hi% expand an environment variable?

# NOTE: This is **not** safe to pass to the CMD shell.

#9043 (comment)

Please add this more clearly to the method documentation.

Hm, maybe I just didn't understand it correctly what's already there. So you can leave it as is unless someone else finds this should be more detailled.

straight-shoota · 2020-04-24T22:12:41Z

src/process/shell.cr

+    quote_posix({arg})
+  end
+
+  # :nodoc:


Why is this method nodoc? Even if it might have less use cases, I think it should still be considered documented.

oprypin · 2020-05-05T23:34:23Z

Just found some independent "research" that matches my finding that it's impossible to safely quote for CMD shell.
https://github.com/actions/toolkit/blob/6a744be7ed9bab4b95ca7600529b7ce5ced1e54a/packages/exec/src/toolrunner.ts#L237

So it's good that I'm not attempting to solve that here. The quote_windows function here is still really useful to be available. Realized another example. Tools can sometimes accept the command line from a file instead. cl.exe is one. So if we were to refactor the code forming linker flags to keep them as an array until the end to pass them to the subprocess, we'd still have to explicitly use this function to convert them to a string here

crystal/src/compiler/crystal/compiler.cr

Lines 341 to 342 in 2cbe65b

    
           File.write(args_filename, args_bytes) 
        
           cmd = "#{CL} @#{args_filename}"

and this is not specific just to Crystal's implementation

bcardiff · 2020-05-06T13:13:16Z

I like the idea of making the compiler escape interpolation between `#{expr}`, yet that requires first this PR and then the compiler logic in a separate release

If we eventually want to do that we would need in this PR a ShellString type to mark the safe strings. Making them safe to concat directly in `#{expr}`, otherwise they need to be escaped. This is similar to html template engines where the user is not forced to explicitly escape interpolated values. I haven't ported this kind of design to Crystal yet, not sure if there would be any surprised along the road.

I guess this will come handle here also #8900 (comment)

I find escape more appropiate than quote. Since it will quote if needed.

straight-shoota · 2020-05-06T13:26:33Z

FYI: I've been using a SafeString type with crinja. Works pretty well. Obviously it can be a bit confusing when you need to take care of strings that are not String. Maybe a Stringlike module could be an improvement to that at some point. But for now it's not that bad and in the context of shell escaping it would probably be restricted to a relative tight scope.

oprypin · 2020-05-06T22:14:54Z

Well, I don't think this PR is blocked on any such future consideration.

I didn't want to name it "escape" because I don't feel it's appropriate for the multi-argument usage. Important to insert quotes in between them, not escaping the spaces.

I don't feel that strongly about it but maybe at least one more vote that it's good to change from quote to escape

oprypin · 2020-05-06T22:23:21Z

I do appreciate the perspective that the action here is escaping but it just happens to be implemented through quoting.

I may also be biased by Python using the name "quote".

Still, though, for Windows this must be done by quoting. And the name "escape" may give a false sense of security that it's safe to use in the shell.

oprypin · 2020-05-19T07:46:29Z

Please consider this for 0.35.0, as I think some follow-up ideas voiced here can't be done before there's a released compiler with this.
This can also be used in Shards nicely (and it needs Windows work too).
And it's a nice security fix in general.

straight-shoota · 2020-05-19T10:40:10Z

... another conflict =)

bcardiff · 2020-05-26T13:21:58Z

I'm a bit lost with something. Why is Process.quote not used in Crystal::System::Process.prepare_args ? Wouldn't that be desired?

oprypin · 2020-05-26T15:37:07Z

But it is used! in src/crystal/system/win32/process.cr

Not for posix though, the concerns there are indeed separate.

oprypin · 2020-05-26T16:42:29Z

For a fuller explanation:

A command can be passed either as a list of args or as one string (this is what the Process API exposes).
On POSIX the native format is a list of args. On Windows the native format is one string.

Because the recommended way to pass args is a list of args, we already have the implementation to convert them to a single string on Windows; this PR only moves it to a public location.

For POSIX there's no conversion required, it's already a list.

However, since we do also have a way to specify a command as one string (for which on Windows the conversion is no-op), we need special support for that on POSIX. We defer that to the shell, and that implementation has been there since the beginning of Crystal.

The reason that the "quote" functions should be public, even on POSIX, is for the use case where one wants "the best of both worlds": conveniently specify an arbitrary string-based command, but also safely intersperse a list of arguments into it. That needs to be used by the caller beforehand, and is never needed by any internal implementation in Process on POSIX.

bcardiff · 2020-05-26T18:25:24Z

Thanks for the clarification @oprypin

bcardiff · 2020-05-27T17:32:11Z

src/compiler/crystal/codegen/cache_dir.cr

        .reverse!
        .skip(10)
-        .each { |name| `rm -rf "#{name}"` rescue nil }
+        .each { |name| `rm -rf -- #{Process.quote(name)}` rescue nil }


Why is the -- needed/introduced here?

Oh. Well it's not directly related to the overall theme of quoting, but does fix a security-related issue. If there was a file named just "-i", rm would see it as a flag, and the quoting wouldn't help either. I don't think it can actually happen here because cache names are controlled by Crystal, but still good practice to always add the double dash to indicate the end of flags.

Why not just do Dir.delete instead?

Unrelated change

I know, so is it adding the -- then, but I'm not saying you have to do it in this PR. Just asking if there is any reason this was made this way. You were doing the analysis of parts of the code that require Process.quote and it could be fixed in a different way instead.

I could've split it into a separate commit but they're ignored under the current merging process.
I could've split it into a separate PR, but the process overhead (mainly, but not exclusively, due to GitHub) is too much.
So I just ignore it.

So but we could/should have a PR that changes this to Dir.delete?

Yes, it'd be good. But I am not interested in sending that out. Maybe the whole codebase could be swept for such things.

oprypin · 2020-05-28T18:45:51Z

src/process/shell.cr

+  end
+
+  private def self.quote_windows(io : IO, args)
+    args.join(' ', io) do |arg|


Aaaa I missed the argument swap

Should we fail CI on warnings? Or maybe we already do on non-Windows

No because there are tests that check that the deprecated methods still work.

But we --exclude-warnings spec/std. Maybe it's not 100% reliable though.

Yes, this should be improved. Not sure if it's not reliable or just not configured properly...

I think that the only reliable way is to fail on warnings on std_spec, and add deprecated_spec target were deprecations can happen. That way we can error on std_spec.

Handled in #9369

Sija reviewed Apr 11, 2020

View reviewed changes

src/process/shell.cr Show resolved Hide resolved

oprypin force-pushed the winquote branch from 60cbff9 to e6555ed Compare April 11, 2020 14:01

oprypin commented Apr 11, 2020

View reviewed changes

src/compiler/crystal/compiler.cr Outdated Show resolved Hide resolved

oprypin mentioned this pull request Apr 11, 2020

Bootstrapping compiler on Windows (tracking bug) #9032

Closed

11 tasks

oprypin changed the title ~~Add Process.shell_quote and fix shell usages in the compiler~~ Add Process.quote and fix shell usages in the compiler Apr 12, 2020

oprypin force-pushed the winquote branch from fa20610 to 5c1895b Compare April 12, 2020 17:37

oprypin mentioned this pull request Apr 13, 2020

Pass the command line to cl.exe through a file #9062

Merged

oprypin force-pushed the winquote branch from 19c5115 to fd6ddf0 Compare April 14, 2020 15:31

Add a method to produce a command line string from args

5436a57

And apply it throughout the compiler for better safety.

oprypin force-pushed the winquote branch from 6bbe510 to 5436a57 Compare April 15, 2020 18:40

Sija reviewed Apr 16, 2020

View reviewed changes

j8r reviewed Apr 18, 2020

View reviewed changes

src/process/shell.cr Show resolved Hide resolved

straight-shoota reviewed Apr 18, 2020

View reviewed changes

oprypin added 2 commits April 18, 2020 20:21

Merge remote-tracking branch 'origin/master' into winquote

2842146

Quote flags in link.cr

066d59b

oprypin force-pushed the winquote branch from 052d567 to 066d59b Compare April 20, 2020 00:38

Merge remote-tracking branch 'origin/master' into winquote

184e39f

straight-shoota reviewed Apr 24, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into winquote

335343d

Merge remote-tracking branch 'origin/master' into winquote

d1d1699

straight-shoota approved these changes May 19, 2020

View reviewed changes

bcardiff added kind:feature topic:stdlib:system labels May 19, 2020

This comment has been minimized.

Sign in to view

bcardiff mentioned this pull request May 26, 2020

Minor security issue for git dependencies crystal-lang/shards#396

Closed

bcardiff added this to the 0.35.0 milestone May 26, 2020

bcardiff approved these changes May 26, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into winquote

d100f65

bcardiff reviewed May 27, 2020

View reviewed changes

bcardiff merged commit d60a1db into crystal-lang:master May 28, 2020

oprypin commented May 28, 2020

View reviewed changes

bcardiff added a commit to bcardiff/crystal that referenced this pull request May 28, 2020

Enumerable#join args swap. Follow up crystal-lang#9043

27e916a

bcardiff mentioned this pull request May 28, 2020

Enumerable#join args swap. Follow up #9043 #9369

Merged

bcardiff pushed a commit that referenced this pull request May 28, 2020

Enumerable#join args swap. Follow up #9043 (#9369)

0dd0c08

Uh oh!

Add Process.quote and fix shell usages in the compiler #9043

Add Process.quote and fix shell usages in the compiler #9043

Uh oh!

Conversation

oprypin commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oprypin commented Apr 11, 2020

Uh oh!

RX14 commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RX14 commented Apr 11, 2020

Uh oh!

oprypin commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

straight-shoota commented Apr 11, 2020

Uh oh!

oprypin commented Apr 11, 2020

Uh oh!

oprypin commented Apr 11, 2020

Uh oh!

oprypin commented Apr 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oprypin commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oprypin commented Apr 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oprypin commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcardiff commented May 6, 2020

Uh oh!

straight-shoota commented May 6, 2020

Uh oh!

oprypin commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oprypin commented May 6, 2020

Uh oh!

oprypin commented May 19, 2020

Uh oh!

straight-shoota commented May 19, 2020

Uh oh!

This comment has been minimized.

bcardiff commented May 26, 2020

Uh oh!

oprypin commented May 26, 2020

Uh oh!

oprypin commented Apr 11, 2020 •

edited

Loading

RX14 commented Apr 11, 2020 •

edited

Loading

oprypin commented Apr 11, 2020 •

edited

Loading

oprypin commented Apr 12, 2020 •

edited

Loading

oprypin commented Apr 14, 2020 •

edited

Loading

straight-shoota Apr 24, 2020 •

edited

Loading

oprypin commented May 5, 2020 •

edited

Loading

oprypin commented May 6, 2020 •

edited

Loading