Skip to content

Wrap call to rest2html in a timeout

Chad Woolley requested to merge timeout-for-rest2html into master

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):

successful-render

Process killed by timeout (no render)

process-killed-with-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'
Edited by Chad Woolley

Merge request reports

Loading