Merge pull request from GHSA-p3q9-qff5-97p7

* Implement checksums for POST requests

Previously, the checksum logic always looked at the query string, even
on POST requests where the parameters are in the body. Add checksum
support for POST parameters (this is somewhat complicated by the fact
that scalelite's internal API uses Rails nested parameters), and match
BigBlueButton's behaviour of rejecting a request if parameters are
present in both the query string and POST request body.

* Validate BigBlueButton API request content-type

* Check 'GET' checksum on 'POST' request with json content type

The checksum helper is also used on the Scalelite administration API,
which is designed to be used with POST requests with json request body.
The checker designed for the BigBlueButton APIs rejected this as an
unsupported content type.

The expected behaviour of these requests is to have the checksum in the
query string, and the body isn't covered by the checksum. Adapt the code
to support this behaviour.

* Use right content type in Admin API tests, reject form data

* Remove checksum validation workaround for admin api

* Update supported request methods to match BigBlueButton

* Remove incorrect POST req checksumming

* Update tests for supported methods and content types

* Remove unused GET_CHECKSUM_MIME_TYPES

* Ensure create call does not pass parameters in POST body as-is to BBB

* Fix formatting issues reported by rubocop

* Correct list of endpoints supporting GET for content type check

Author: kepstin

app/controllers/api/scalelite_api_controller.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module Api
+  class ScaleliteApiController < ApplicationController
+    include ApiHelper
+
+    skip_before_action :verify_authenticity_token
+
+    before_action :verify_content_type
+    before_action -> { verify_checksum(true) }
+
+    def verify_content_type
+      return unless request.post? && request.content_length.positive?
+
+      raise UnsupportedContentType unless request.content_mime_type == Mime[:json]
+    end
+  end
+end

app/controllers/api/servers_controller.rb

@@ -1,12 +1,7 @@
 # frozen_string_literal: true
 
 module Api
-  class ServersController < ApplicationController
-    include ApiHelper
-
-    skip_before_action :verify_authenticity_token
-
-    before_action -> { verify_checksum(true) }
+  class ServersController < ScaleliteApiController
     before_action :set_server, only: [:get_server_info, :update_server, :delete_server, :panic_server]
 
     # Return a list of the configured BigBlueButton servers

app/controllers/api/tenants_controller.rb

@@ -1,12 +1,7 @@
 # frozen_string_literal: true
 
 module Api
-  class TenantsController < ApplicationController
-    include ApiHelper
-
-    skip_before_action :verify_authenticity_token
-
-    before_action -> { verify_checksum(true) }
+  class TenantsController < ScaleliteApiController
     before_action :check_multitenancy
     before_action :set_tenant, only: [:get_tenant_info, :update_tenant, :delete_tenant]
 

app/controllers/bigbluebutton_api_controller.rb

@@ -5,6 +5,13 @@ class BigBlueButtonApiController < ApplicationController
 
   skip_before_action :verify_authenticity_token
 
+  # Check content types on endpoints that accept POST requests. For most endpoints, form data is permitted.
+  before_action :verify_content_type, except: [:create, :insert_document, :join, :publish_recordings, :delete_recordings]
+  # create allows either form data or XML
+  before_action :verify_create_content_type, only: [:create]
+  # insertDocument only allows XML
+  before_action :verify_insert_document_content_type, only: [:insert_document]
+
   before_action :verify_checksum, except: [:index, :get_recordings_disabled, :recordings_disabled, :get_meetings_disabled,
                                            :analytics_callback,]
 
@@ -198,6 +205,8 @@ def create
 
     params[:voiceBridge] = meeting.voice_bridge
 
+    have_preuploaded_slide = request.post? && request.content_mime_type == Mime[:xml]
+
     logger.debug("Creating meeting #{params[:meetingID]} on BigBlueButton server #{server.id}")
     params_hash = params
 
@@ -209,8 +218,8 @@ def create
     uri = encode_bbb_uri('create', server.url, server.secret, pass_through_params(excluded_params))
 
     begin
-      # Read the body if POST
-      body = request.post? ? request.body.read : ''
+      # Read the body if preuploaded slide XML is present
+      body = have_preuploaded_slide ? request.raw_post : ''
 
       # Send a GET/POST request to the server
       response = get_post_req(uri, body, **bbb_req_timeout(server))

app/controllers/concerns/api_helper.rb

@@ -19,38 +19,39 @@ module ApiHelper
   #     which should be accessed only by superadmins.
   def verify_checksum(force_loadbalancer_secret = false)
     secrets = fetch_secrets(force_loadbalancer_secret: force_loadbalancer_secret)
-
-    raise ChecksumError if params[:checksum].blank?
     raise ChecksumError if secrets.empty?
 
-    algorithm = case params[:checksum].length
-                   when CHECKSUM_LENGTH_SHA1
-                     'SHA1'
-                   when CHECKSUM_LENGTH_SHA256
-                     'SHA256'
-                   when CHECKSUM_LENGTH_SHA512
-                     'SHA512'
-                   else
-                     raise ChecksumError
-                   end
-
-    # Camel case (ex) get_meetings to getMeetings to match BBB server
-    check_string = action_name.camelcase(:lower)
-    check_string += request.query_string.gsub(
-      /&checksum=#{params[:checksum]}|checksum=#{params[:checksum]}&|checksum=#{params[:checksum]}/, ''
-    )
+    checksum = request.params[:checksum]
+    raise ChecksumError if checksum.blank?
 
+    algorithm = guess_checksum_digest_algorithm(checksum)
     allowed_checksum_algorithms = Rails.configuration.x.loadbalancer_checksum_algorithms
-    raise ChecksumError unless allowed_checksum_algorithms.include? algorithm
+    raise ChecksumError unless allowed_checksum_algorithms.include?(algorithm)
 
-    secrets.each do |secret|
-      return true if ActiveSupport::SecurityUtils.secure_compare(get_checksum(check_string + secret, algorithm),
-                                                                 params[:checksum])
+    check_string = action_name.camelcase(:lower) + query_string_remove_checksum(checksum)
+    return true if secrets.any? do |secret|
+      ActiveSupport::SecurityUtils.secure_compare(get_checksum(check_string + secret, algorithm), checksum)
     end
 
     raise ChecksumError
   end
 
+  # Remove the checksum from the request query string. This is done as string manipulation, rather than decoding then re-encoding the parameters,
+  # since there's multiple possible valid encodings for query parameters, and the one used by Ruby might not match.
+  def query_string_remove_checksum(checksum)
+    checksum = Regexp.escape(checksum)
+    request.query_string.gsub(/&checksum=#{checksum}|checksum=#{checksum}&|checksum=#{checksum}/, '')
+  end
+
+  def guess_checksum_digest_algorithm(checksum)
+    case checksum.length
+    when CHECKSUM_LENGTH_SHA1 then 'SHA1'
+    when CHECKSUM_LENGTH_SHA256 then 'SHA256'
+    when CHECKSUM_LENGTH_SHA512 then 'SHA512'
+    else raise ChecksumError
+    end
+  end
+
   def fetch_secrets(tenant_name: nil, force_loadbalancer_secret: false)
     return Rails.configuration.x.loadbalancer_secrets if force_loadbalancer_secret || !Rails.configuration.x.multitenancy_enabled
 
@@ -98,6 +99,27 @@ def checksum_algorithm
     end
   end
 
+  # Verify that the Content-Type of POST requests is a "form data" type (applies to most APIs)
+  def verify_content_type
+    return unless request.post? && request.content_length.positive?
+    raise UnsupportedContentType unless request.form_data?
+  end
+
+  # Verify that the Content-Type of a POST request is a format permitted by the create API.
+  # This can either be form data containing params, or an XML document for pre-uploaded slides
+  CREATE_PERMITTED_CONTENT_TYPES = Set.new([Mime[:url_encoded_form], Mime[:multipart_form], Mime[:xml]]).freeze
+  def verify_create_content_type
+    return unless request.post? && request.content_length.positive?
+    raise UnsupportedContentType unless CREATE_PERMITTED_CONTENT_TYPES.include?(request.content_mime_type)
+  end
+
+  # Verify that the Content-Type of a POST request is a format permitted by the insertDocument API.
+  # Only an XML document for pre-uploaded slides (same format as create) is permitted.
+  def verify_insert_document_content_type
+    return unless request.post? && request.content_length.positive?
+    raise UnsupportedContentType unless request.content_mime_type == Mime[:xml]
+  end
+
   # Encode URI and append checksum
   def encode_bbb_uri(action, base_uri, secret, bbb_params = {})
     # Add slash at the end if its not there

app/controllers/concerns/bbb_errors.rb

@@ -40,6 +40,12 @@ def initialize
     end
   end
 
+  class UnsupportedContentType < BBBErrors::BBBError
+    def initialize
+      super('unsupportedContentType', 'POST request Content-Type is missing or unsupported')
+    end
+  end
+
   class InternalError < BBBError
     def initialize(error)
       super('internalError', error)

config/routes.rb

@@ -3,7 +3,10 @@
 Rails.application.routes.draw do
   # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html
 
-  scope 'bigbluebutton/api', as: 'bigbluebutton_api', format: false, defaults: { format: 'xml' } do
+  scope 'bigbluebutton/api', as: :bigbluebutton_api, format: false, defaults: { format: 'xml' } do
+    # See https://github.com/bigbluebutton/bigbluebutton/blob/main/bigbluebutton-web/grails-app/controllers/org/bigbluebutton/web/UrlMappings.groovy
+    # for the definitions of the routes in BigBlueButton itself. Note that both private (BBB internal) and public APIs are in that file.
+
     match '/', to: 'bigbluebutton_api#index', via: [:get, :post]
     match 'isMeetingRunning', to: 'bigbluebutton_api#is_meeting_running', as: :is_meeting_running, via: [:get, :post]
     match 'getMeetingInfo', to: 'bigbluebutton_api#get_meeting_info', as: :get_meeting_info, via: [:get, :post]
@@ -14,19 +17,19 @@
     end
     match 'create', to: 'bigbluebutton_api#create', via: [:get, :post]
     match 'end', to: 'bigbluebutton_api#end', via: [:get, :post]
-    match 'join', to: 'bigbluebutton_api#join', via: [:get, :post]
-    post 'analytics_callback', to: 'bigbluebutton_api#analytics_callback', as: :analytics_callback
-    post 'insertDocument', to: 'bigbluebutton_api#insert_document'
+    get 'join', to: 'bigbluebutton_api#join'
+    post 'analytics_callback', to: 'bigbluebutton_api#analytics_callback'
+    post 'insertDocument', to: 'bigbluebutton_api#insert_document', as: :insert_document
     if Rails.configuration.x.recording_disabled
       match('getRecordings', to: 'bigbluebutton_api#get_recordings_disabled', as: :get_recordings, via: [:get, :post])
-      match('publishRecordings', to: 'bigbluebutton_api#recordings_disabled', as: :publish_recordings, via: [:get, :post])
+      get('publishRecordings', to: 'bigbluebutton_api#recordings_disabled', as: :publish_recordings)
       match('updateRecordings', to: 'bigbluebutton_api#recordings_disabled', as: :update_recordings, via: [:get, :post])
-      match('deleteRecordings', to: 'bigbluebutton_api#recordings_disabled', as: :delete_recordings, via: [:get, :post])
+      get('deleteRecordings', to: 'bigbluebutton_api#recordings_disabled', as: :delete_recordings)
     else
       match('getRecordings', to: 'bigbluebutton_api#get_recordings', as: :get_recordings, via: [:get, :post])
-      match('publishRecordings', to: 'bigbluebutton_api#publish_recordings', as: :publish_recordings, via: [:get, :post])
+      get('publishRecordings', to: 'bigbluebutton_api#publish_recordings', as: :publish_recordings)
       match('updateRecordings', to: 'bigbluebutton_api#update_recordings', as: :update_recordings, via: [:get, :post])
-      match('deleteRecordings', to: 'bigbluebutton_api#delete_recordings', as: :delete_recordings, via: [:get, :post])
+      get('deleteRecordings', to: 'bigbluebutton_api#delete_recordings', as: :delete_recordings)
     end
   end
 

spec/controllers/concerns/api_helper_spec.rb

@@ -55,6 +55,7 @@
   describe '.verify_checksum' do
     let(:query_string) { 'querystring' }
     let(:action_name) { 'index' }
+    let(:method) { :get }
     let(:check_string) { action_name + query_string }
     let(:checksum_algo) { nil } # To be defined down the scope
     let(:secret) { 'IAmSecret' }
@@ -63,10 +64,19 @@
     before do
       controller.action_name = action_name
       allow(request).to receive(:query_string).and_return(query_string)
+      allow(request).to receive(:request_method_symbol).and_return(method)
       Rails.configuration.x.loadbalancer_secrets = [secret]
+      if hash.present?
+        case method
+        when :get then request.query_parameters[:checksum] = hash
+        when :post then request.request_parameters[:checksum] = hash
+        end
+      end
     end
 
     context 'without params[:checksum]' do
+      let(:hash) { nil }
+
       it 'throws an error' do
         expect {
           verify_checksum
@@ -78,39 +88,24 @@
       context 'with SHA1' do
         let(:checksum_algo) { 'SHA1' }
 
-        before do
-          params[:checksum] = hash
-        end
-
         include_examples 'proper verify_checksum behavior'
       end
 
       context 'with SHA256' do
         let(:checksum_algo) { 'SHA256' }
 
-        before do
-          params[:checksum] = hash
-        end
-
         include_examples 'proper verify_checksum behavior'
       end
 
       context 'with SHA512' do
         let(:checksum_algo) { 'SHA512' }
 
-        before do
-          params[:checksum] = hash
-        end
-
         include_examples 'proper verify_checksum behavior'
       end
 
       context 'with incorrect checksum' do
         let(:checksum_algo) { 'MD5' }
-
-        before do
-          params[:checksum] = 'totallyNotAHash'
-        end
+        let(:hash) { 'totallyNotAHash' }
 
         it 'throws an error' do
           expect {