CVE 2022 25765 fix

[MuhammetDilmac]

I wrote a small code for the CVE 2022-25765. Please review and test it.
#517 rubysec/ruby-advisory-db#517 It's too dirty please refactor this :)

[MuhammetDilmac]

Please check this @kiafaldorius @postmodern

[postmodern]

Not to derail here, but the root cause seems to be that the command is executed via IO.popen with a String which executes the command as a shell command (ex: $SHELL -c '...'). If IO.popen is called with an Array it will be executed as a separate process with individual arguments without shell interpolation.

# with a String
IO.popen("echo $HOME").read
# => "/home/postmodern\n"

# with an array
IO.popen(['echo', '$HOME']).read
# => "$HOME\n"

Does the wkhtmltopdf command absolutely need to be executed in a shell vs a separate process?

[kiafaldorius]

@MuhammetDilmac Thanks for opening the PR! I'm not entirely sure this works, since to_input_for_command is escaping the opening and ending quote? I'd have to run to check, but gut feeling is that may pass in the argument incorrectly to the command. The to_input_for_command should be using single quotes, single quotes shouldn't interpolate (but be careful, because single quotes aren't escaped by URI::DEFAULT_PARSER!)...or better yet, not quoting the string at all and passing the arguments in a way that it's executed as an argument without interpolation. @postmodern has what I think is a cleaner approach by passing explicit arguments separately.

@postmodern This looks to be the cleaner approach. I think it needs to use IO.popen to pipe the output into rubyland as the result of #to_pdf, but I don't know about whether it needs the shell. I would expect not, but the maintainers may have other requirements.

If it absolutely requires it for some use cases, might I suggest a separate #unsafe_to_pdf or similar that executes behind the shell, and having the default be a safe, non-shell execution.

[MuhammetDilmac]

I agree with IO open pipe. But I don't know why maintainers use shell.
@kiafaldorius I use Shellwords.escapeshell for the escaping shell characters.

[postmodern]

FYI this is the call to IO.popen:

pdfkit/lib/pdfkit/pdfkit.rb

Lines 74 to 80 in 6213317

	invoke = command(path)
	result = IO.popen(invoke, "wb+") do |pdf|
	pdf.puts(@source.to_s) if @source.html?
	pdf.close_write
	pdf.gets(nil) if path.nil?
	end

Without digging through the Git history I suspect the original authors just through passing a String to IO.popen was "how it was done" in Ruby; I forget when Ruby added support for passing an Array to IO.popen.

I guess it depends on whether users of pdfkit really expect environment variables or shell commands to be embedded in PDF input or not. If not, then I say IO.popen with a command Array for maximum safety.

[kiafaldorius]

I see why this is here, but needing this is a sign that there may be other edge cases with special characters when escaped characters are inside strings vs as a regular argument. The #to_input_for_command will be better off not having the quotes when the content is already escaped.

Spaces are escaped, so the core part of why the quotes need to be there--to have the command treat its content as a singular argument rather than multiple arguments--should be handled already by the escaping.

(I don't know how/whether Shellwords works in Windows, it may not.)

[kiafaldorius]

You're assuming a properly formatted URL. That's a dangerous thing...What's the result when someone passes in url = http://localhost:3000/haxor$HOME/hello. Hint: It doesn't escape the important part....

[kiafaldorius]

I wouldn't bother checking this, shouldn't hurt to always escape. People should not be passing in shell formatted content to the library anyway.

[postmodern]

There's some specs that test whether a previously escaped source URL does not get double-escaped. Example, what if someone passes in a URL that was formatted by URI::HTTP.build and already contains '%XX' escaped characters? Should we assume the user must always pass in an unescaped URL? Or should we assume it's the user's responsibility to properly format the URL? Should those tests be removed?

[kiafaldorius]

@postmodern I'm pretty sure IO.popen has allowed the array format since 1.9. I remember updating some stuff from back in 1.8.7 transition to use the new format upgrading to 1.9...of course it is a breaking change, but I would hope no one is running 1.8.7 nowadays.

[kiafaldorius]

@MuhammetDilmac Thanks for the PR, I added some quick review notes. Unfortunately, it doesn't handle all the necessary cases.

Security is hard!

[postmodern]

@kiafaldorius I could try sending a PR that changes #command to return an Array of arguments. This should solve the shell escaping issue, but I'm not sure about whether the given URL should always be URI escaped, auto-URI-escaped, or never URI escaped.

Although, I noticed a lot of code is for adding special shell escaping specifically for Windows. I'm not sure how IO.popen(array) behaves on Windows, but I assume it runs the command as a separate process, not in a shell.

[MuhammetDilmac]

This PR is so dirty. @postmodern PR #519 is more clear and more secure.