Unescaped urls are now shell escaped #508
Conversation
This helps address [CVE-2022-25765](https://www.cve.org/CVERecord?id=CVE-2022-25765) allowing shell commands in backticks when calling wkhtmltopdf.
|
The only edit I had to make that I didn't want to publish was downgrading to rack 2.x as bundle resulted in using rack 3, failing tests. (and the binary for my own use) Gemfile source 'https://rubygems.org'
group :test do
gem 'activesupport', ENV['RAILS_VERSION'] || '~> 4.1'
gem 'simplecov', require: false
end
group :development, :test do
gem 'pry'
gem 'wkhtmltopdf-binary'
gem 'rack', '~> 2.0'
end
gemspec |
|
Sorry for the premature PR, but I noticed that there is a test that makes me think that not escaping previously escaped source URLs is by design? Darn I was hoping it'd be that easy. PDFKit::Source#to_input_for_command does not URI escape previously escaped source URL Seems my PR goes against that one. |
Created a check for backticks in the source, and if it exists, shellescape. This allows "PDFKit::Source#to_input_for_command does not URI escape previously escaped source URL" test to pass.
To make the tests pass, I had to add a way to check if shellcode could have been used. In doing so, $() is also something to worry about since it will run shell code as well. Unfortunately, I'm not as well versed in all of the ways shell code could be executed here, so I attempted to make it where you could add to the list to check against.
dblanken
force-pushed
the
fix_cve-2022-25765
branch
from
f352d78
to
63ffbb6
Compare
|
I attempted to get all tests passing while handling the case (minus the connection thrown one that seems to be only for a certain binary version). Currently, backtick insertions are causing an exception to be thrown (but not executed), which for malicious intent could be ok. I'm not sure all of the use cases of this gem to be certain. But that is the reason for the begin/rescue in the test so that we could ensure the file was not created. The only thing I can think of is to sanitize the backticks, which seemed invasive. |
|
Hello @dblanken Thanks for your attention to this issue. This PR could be still injected by those which not executing shell from string interpolation. I thought the root cause is that |
Instead of checking if it is a shell safe url, unencode it as it was previously being tested, and then allow parse to attempt to re-encode it for our use using it's to_s method. This seems to pass all tests minus two that are not expecting single quotes to be encoded, but in my tests, urls placed in browser and in PDFKit.new.to_pdf seem to yield the same results. I did not want to change existing tests until getting feedback from others before doing so as I do not want to cause unexpected results from use, which this seems to do.
|
Hello @TimWei Thank you so much for your message. Yes, you are correct that || and probably && or ; could still be used. It's also possible I'm trying to fix at the wrong spot. The issue I am seeing is that URL::DEFAULT_PARSER.unescape seems to be missing characters that we're interested in. A possibility I am thinking is using URI::parse.to_s to have URI parse the unencoded version if it thinks it is needed and produce a source string for us, and utilize the ABS_PATH regex instead of the ESCAPE regex on unescape to determine if something should happen to it. Then again, we could just pass the whole thing through URI::parse.to_s and not have the test whether it should be encoded or not. The issue I'm seeing is that these changes break the existing test of not URI escaping a previously escaped source URL and escaping source URLs and close them in quotes to accommodate ampersands as it encodes the single quotes. The results seem to still work in browser and in to_pdf, but I would want more feedback before changing those tests to make sure I'm not introducing issues for use cases I'm not thinking of. I've published what my implementation of this would be to see if I am missing more. |
|
Closing as @KWkyle's solution covers cases while allowing existing tests to pass. |
This helps address CVE-2022-25765 referenced in issue #507 allowing shell commands in backticks when calling wkhtmltopdf by shellescaping URLs not needing URI::Parse escaping.