Wrap call to rest2html in a timeout

Description

Wraps command invocations in a timeout, which will kill the command after 10 seconds.

The timeout seconds can be controlled via the GITLAB_MARKUP_TIMEOUT environment variable.

If the command times out, the process will be killed with a KILL signal, and a TimeoutError with a descriptive message will be raised.

If you set the timeout greater than 60 seconds and load one that takes longer (e.g. dos original test file with GITLAB_MARKUP_TIMEOUT set to 120 seconds), then:

1. Client will get Gitlab::RequestContext::RequestDeadlineExceeded at /root/rst_dos_test/-/wikis/dos after ~60 seconds
2. The worker is still killed after the timeout of 120 seconds even though the request has timed out (thus fixing the original exploit from https://gitlab.com/gitlab-org/gitlab/-/issues/37283).

See https://gitlab.com/gitlab-org/gitlab/-/issues/37283#note_725442087 for more context.

Screenshots

Successful render (no timeout):

Process killed by timeout (no render)

Example of log/exceptions_json.log

{"severity":"ERROR","time":"2021-11-29T18:47:57.170Z","correlation_id":"01FNPG8MGAGEP3JXW6HDJ4KBS5","exception.class":"Timeout::Error","exception.message":"Command was killed, probably due to exceeding GITLAB_MARKUP_TIMEOUT limit of 120 seconds","exception.backtrace":["lib/gitlab/other_markup.rb:11:in `render'","app/helpers/markup_helper.rb:281:in `other_markup_unsafe'","app/helpers/markup_helper.rb:146:in `markup_unsafe'","app/helpers/markup_helper.rb:131:in `render_wiki_content'","app/views/shared/wikis/show.html.haml:30","app/controllers/application_controller.rb:136:in `render'","app/controllers/concerns/wiki_actions.rb:84:in `show'","ee/lib/gitlab/ip_address_state.rb:10:in `with'","ee/app/controllers/ee/application_controller.rb:44:in `set_current_ip_address'","app/controllers/application_controller.rb:504:in `set_current_admin'","lib/gitlab/session.rb:11:in `with_session'","app/controllers/application_controller.rb:495:in `set_session_storage'","lib/gitlab/i18n.rb:105:in `with_locale'","lib/gitlab/i18n.rb:111:in `with_user_locale'","app/controllers/application_controller.rb:489:in `set_locale'","app/controllers/application_controller.rb:483:in `set_current_context'","lib/gitlab/metrics/elasticsearch_rack_middleware.rb:16:in `call'","lib/gitlab/middleware/rails_queue_duration.rb:33:in `call'","lib/gitlab/middleware/speedscope.rb:13:in `call'","lib/gitlab/request_profiler/middleware.rb:17:in `call'","lib/gitlab/query_limiting/middleware.rb:17:in `block in call'","lib/gitlab/query_limiting/transaction.rb:40:in `run'","lib/gitlab/query_limiting/middleware.rb:16:in `call'","lib/gitlab/metrics/rack_middleware.rb:16:in `block in call'","lib/gitlab/metrics/web_transaction.rb:46:in `run'","lib/gitlab/metrics/rack_middleware.rb:16:in `call'","lib/gitlab/jira/middleware.rb:19:in `call'","lib/gitlab/middleware/go.rb:20:in `call'","lib/gitlab/etag_caching/middleware.rb:21:in `call'","lib/gitlab/middleware/query_analyzer.rb:11:in `block in call'","lib/gitlab/database/query_analyzer.rb:42:in `within'","lib/gitlab/middleware/query_analyzer.rb:11:in `call'","lib/gitlab/middleware/multipart.rb:173:in `call'","lib/gitlab/middleware/read_only/controller.rb:50:in `call'","lib/gitlab/middleware/read_only.rb:18:in `call'","lib/gitlab/middleware/same_site_cookies.rb:27:in `call'","lib/gitlab/middleware/handle_malformed_strings.rb:21:in `call'","lib/gitlab/middleware/basic_health_check.rb:25:in `call'","lib/gitlab/middleware/handle_ip_spoof_attack_error.rb:25:in `call'","lib/gitlab/middleware/request_context.rb:21:in `call'","config/initializers/fix_local_cache_middleware.rb:11:in `call'","lib/gitlab/middleware/compressed_json.rb:26:in `call'","lib/gitlab/middleware/static.rb:11:in `call'","lib/gitlab/webpack/dev_server_middleware.rb:34:in `perform_request'","lib/gitlab/middleware/rack_multipart_tempfile_factory.rb:19:in `call'","lib/gitlab/middleware/sidekiq_web_static.rb:20:in `call'","lib/gitlab/metrics/requests_rack_middleware.rb:75:in `call'","lib/gitlab/middleware/release_env.rb:13:in `call'"],"user.username":"root","tags.program":"web","tags.locale":"en","tags.feature_category":"wiki","tags.correlation_id":"01FNPG8MGAGEP3JXW6HDJ4KBS5","extra.project_id":20,"extra.file_name":"dos.rst"}

Testing

Steps to reproduce

See the reproduce section in the issue for steps to reproduce: https://gitlab.com/gitlab-org/gitlab/-/issues/37283#reproduce

Additional testing notes

Ensure you have docutils installed:

python3 -m pip install --upgrade setuptools
python3 -m pip install --upgrade pip
pip3 install docutils

I have verified that the timeout CLI options are the same on MacOS and Linux, and @cmiskell confirmed that this should be supported in all recent OSs

Test locally by replacing the following line to your GitLab Gemfile, bundle and gdk restart rails-web:

gem 'gitlab-markup', '~> 1.7.1', path: '/your/path/to/gitlab-markup/'

See the following repo for examples of large RST files: https://gitlab.com/gitlab-com/gl-security/security-research/sec-research/-/tree/master/005/rst

Here is also an example of a small, valid RST file: example.rst

If you see the following .. math:: as the first line, it means the RST did NOT render due to the timeout.

.. math::

\Huge \sqrt{\sqrt{\sqrt{\sqrt{

If you don't see it, then it rendered successfully. You can also add some fixed text in triple-backticks to easily confirm whether it rendered or not.

You can also tail -f log/exceptions_json.log to look for the errors.

This can also be verified in the debugger by setting a breakpoint in gitlab-markup/lib/github/markup/command_implementation.rb and checking the status of the executed command.

To test the posix branch, add gem "posix-spawn", :platforms => :ruby to your gemfile and restart.

My my machine, currently the sample dos file sized just under 1 meg renders successfully, but the ones 2 meg and above time out.

Gemfile Patch

Here's a patch for the Gemfile/Gemfile.lock changes:

Index: Gemfile.lock
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/Gemfile.lock b/Gemfile.lock
--- a/Gemfile.lock	(revision 7e9466e331e17476bac781ed4447cce86d276ebf)
+++ b/Gemfile.lock	(date 1638207874727)
@@ -1,3 +1,8 @@
+PATH
+  remote: /Users/cwoolley/workspace/gitlab-markup
+  specs:
+    gitlab-markup (1.7.1)
+
 PATH
   remote: vendor/gems/mail-smtp_pool
   specs:
@@ -489,7 +494,6 @@
       with_env (= 1.1.0)
       xml-simple (~> 1.1.5)
     gitlab-mail_room (0.0.9)
-    gitlab-markup (1.7.1)
     gitlab-net-dns (0.9.1)
     gitlab-omniauth-openid-connect (0.8.0)
       addressable (~> 2.7)
@@ -918,6 +922,7 @@
     png_quantizator (0.2.1)
     po_to_json (1.0.1)
       json (>= 1.6.0)
+    posix-spawn (0.3.15)
     premailer (1.11.1)
       addressable
       css_parser (>= 1.6.0)
@@ -1475,7 +1480,7 @@
   gitlab-license (~> 2.0)
   gitlab-license_finder (~> 6.0)
   gitlab-mail_room (~> 0.0.9)
-  gitlab-markup (~> 1.7.1)
+  gitlab-markup (~> 1.7.1)!
   gitlab-net-dns (~> 0.9.1)
   gitlab-omniauth-openid-connect (~> 0.8.0)
   gitlab-sidekiq-fetcher (= 0.8.0)
@@ -1569,6 +1574,7 @@
   pg (~> 1.1)
   pg_query (~> 2.1)
   png_quantizator (~> 0.2.1)
+  posix-spawn
   premailer-rails (~> 1.10.3)
   prometheus-client-mmap (~> 0.15.0)
   pry-byebug
Index: Gemfile
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/Gemfile b/Gemfile
--- a/Gemfile	(revision 7e9466e331e17476bac781ed4447cce86d276ebf)
+++ b/Gemfile	(date 1638207880974)
@@ -153,7 +153,9 @@
 # Markdown and HTML processing
 gem 'html-pipeline', '~> 2.13.2'
 gem 'deckar01-task_list', '2.3.1'
-gem 'gitlab-markup', '~> 1.7.1'
+# gem 'gitlab-markup', '~> 1.7.1'
+gem 'gitlab-markup', '~> 1.7.1', path: '/Users/cwoolley/workspace/gitlab-markup/'
+gem "posix-spawn", :platforms => :ruby
 gem 'github-markup', '~> 1.7.0', require: 'github/markup'
 gem 'commonmarker', '~> 0.23.2'
 gem 'kramdown', '~> 2.3.1'

added groupeditor [DEPRECATED] label

assigned to @cwoolley-gitlab

changed the description

@stanhu @cmestel I've made the change in this MR to add the timeout as Stan suggested in https://gitlab.com/gitlab-org/gitlab/-/issues/37283#note_725442087

However, I can't get the timeout to be executed.

It seems like any file which is big enough to cause the timeout is too big for the wiki to try to render it, and it just gets rendered as normal text?

I need to dig into it more to see what exactly is going on, but going ahead and mentioning it.

cc @fjsanpedro

Setting label(s) devopscreate sectiondev based on ~"group::editor".

added devopscreate sectiondev labels

changed the description

@cmiskell I think this is ready to go - would you like to review it?

Thanks!

requested review from @cmiskell

added 1 commit

• 659aa0f8 - Wrap command executions in a timeout

Compare with previous version

changed the description

I have also confirmed that if you set the timeout greater than 60 seconds and load one that takes longer (e.g. dos original file set to 120 seconds), then:

1. Client will get Gitlab::RequestContext::RequestDeadlineExceeded at /root/rst_dos_test/-/wikis/dos after ~60 seconds
2. The worker is still killed after the timeout of 120 seconds even though the request has timed out (thus fixing the original exploit from https://gitlab.com/gitlab-org/gitlab/-/issues/37283).

approved this merge request