Fixed security issue that would allow unpermitted params to be passed through

[farhatahmad]

Description

Testing Steps

Screenshots (if appropriate):

[farhatahmad]

@defnull This should technically solve both issues - please take a look and let me know

[kepstin]

This also needs "integration" test cases which go through the rails request pipeline and include parameters in both the query string and body of the request. (I'd recommend writing these in Minitest rather than Rspec so you can use the Testing Rails Applications document for reference, and so I can review them for correctness.

See this SO reply: https://stackoverflow.com/a/58018336 which notes that the RSpec requests stuff cannot simulate a request with both query and body parameters, and demonstrates how to provide both using the QUERY_STRING parameter:

class CollectionsTest < ActionDispatch::IntegrationTest
  test 'foo' do
    post collections_path, { collection: { name: 'New Collection' } }, 
                           { "QUERY_STRING" => "api_key=my_api_key" }

    # this proves that the parameters are recognized separately in the controller
    # (you can test this in you controller as well as here in the test):
    puts request.POST.inspect
    # => {"collection"=>{"name"=>"New Collection"}}
    puts request.GET.inspect
    # => {"api_key"=>"my_api_key"}
  end
end

[kepstin]

This doesn't fix the problem. The request.query_parameters method (which is an alias to request.GET) still only returns query parameters, not parameters in a post body.
The use of the CGI.escape function might escape the parameters differently from how they were on the original request (for example, integrations may either use %20 vs + to replace a space), which will cause the checksum to fail in some cases.

[farhatahmad]

query_parameters does include parameters that are passed through the body.
It also reflects the same parameters that are passed to encode_bbb_uri through this method here

def pass_through_params(excluded_params)
    params.except(*(excluded_params + [:format, :controller, :action, :checksum]))
          .to_unsafe_hash
  end

It doesn't fully include the body but at the very least it makes the checksum check the same parameters that are passed through to BBB

[farhatahmad]

Yep - working on tests now

[defnull]

I do not think this would solve the reported issue at all:

• request.query_parameters is an alias for request.GET which includes query parameters only (as the name suggests) so the updated verify_checksum still only checks query parameters and ignores url-encoded form parameters transmitted in the request body. But request.params (which is a merged hash of both query and form parameters) is still used in other places. Nothing changed in this regard.
• request.query_parameters is a hash. I'm not sure if one can rely on the order of parameters, so using that may create a check_string with a different order than what was used to create the checksum, failing the check for legitimate requests that have more than a single parameter. Non-ASCII parameters may also be altered by the use of CGI.escape.

But since we are discussing this in the open now, may I suggest a different solution?

First, stop accepting POST requests for endpoints that should not respond to POST requests. Simply edit config/routes.rb and remove the :post keyword from all BBB API routes that do not need to support POST. This solves the reported security problem (issue 1) completely. Additionally, in verify_checksum, fail if there are parameters in both the query string and the request body. That should never happen, even for APIs that do support both GET and POST requests (e.g. /create).

The signature is only valid for the verbatim url-encoded raw string (minus the checksum parameter). Order and encoding is important. The simplest code that avoids user-controlled regular expressions (issue 2) may be:

    check_string = action_name.camelcase(:lower)
    check_string += request.query_string.split("&").reject(|p| p.starts_with "checksum=").join("&")

(untested).

[farhatahmad]

Actually looks like you guys are right - I could've sworn when I tested that query_parameters worked but now its not. Will try again with a different approach

[kepstin]

Note that the "join" API in BigBlueButton does support POST requests despite not being mentioned in the docs; removing support for :post from Scalelite technically breaks BBB API compatibility. Admittedly, this ability is only very rarely used.

[defnull]

Usually the written specification is what counts. Support for POST requests to e.g. /join should either be specified in the documentation, or removed from the implementation, as soon as possible (3.0?). Undefined behavior is dangerous, as we see here.

If POST support needs to be kept, then verify_checksum should either ensure that only one of the two locations (query string or post body) contains parameters, or check both independently (expecing both to have a separate checksum) so the merged data in request.params can be trusted. This currently does not happen. Only the query string is checked, but request.params is used in different places (which also contains unverified parameters from the request body). This leads to unverified parameters ending up in /join redirect URLs generated by Scalelite.

[defnull]

By the way: Currently /create forwards the POST body verbatim, which works for the usual case (XML body) but does not work for the case discussed here (parameters in the POST body) because verify_checksum would check the post-body checksum against an empty query string and fail. get_post_req also assumes that the POST body is always XML. So, if I'm not mistaken, then Scalelite currently does not correctly handle url-encoded parameters in the POST body at all, not even for the APIs that are documented to support it. Fixing verify_checksum may surface more bugs that are currently hidden behind the checksum-always-fails barrier.