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

[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.