Vulnerability write-up - "Dangerous assumptions"

TL;DR

Vulnerabilities emerge if software developers rely too much on the security of external components like open source libraries. A customer project led us into a rabbit hole finding multiple issues in third-party packages, resulting in critical vulnerabilities in our client's final product. Together with the DIVD we disclosed the findings to the open source communities, which resulted in fixes and 6 CVEs, found by our security experts Thomas Rinsma and Kevin Valk.

How vulnerabilities emerge: stacked dependencies

Modern applications depend heavily on other components, ranging from entire frameworks to small pieces of functionality. Such components in turn rely on similar dependencies. The case of NPM and the left-pad package is infamous, but the problem of dependency explosion is occurring in many modern software stacks.

Stacked dependencies introduced a new challenge to application security because the methodologies used to assess a standalone-product's security are no longer applicable. For several reasons, it is generally impossible to audit the entire dependency tree. The opposite approach of ignoring third-party code and solely focusing on the application code is in turn dangerous, and may leave several blind spots.

xkcd: Dependency

This is why we, at Codean, strive to find a good balance for our security evaluation work. Is a library blindly trusted to handle sensitive data? We will certainly look under the hood to shape an opinion about its overall security. Is it instead handling harmless functionalities? Then we can safely skip a thorough analysis. Our approach to stacked dependencies proved to be valuable and effective. In one of our continuous projects, it led us into a rabbit hole that revealed the path to several critical vulnerabilities in our client's final product, with multiple CVEs in open source packages as a result.

In this post, we’ll take you along in our explorative journey across the API boundaries of these packages, explaining the issues we found and how they could be exploited. We hope that this post will also serve as a reminder to not blindly trust third party components when it comes to critical functionality.

Vulnerabilities in Feathers and Sequelize

Our target application was written in Typescript and built on top of the Feathers web-framework, making use of the Sequelize ORM with MariaDB for storage. In a nutshell, this combination allows an application to easily define services with associated models.

Each service exposes CRUD-style operations over HTTP and optionally WebSockets, where these operations are converted into database operations by the ORM. For example, this request:

GET /messages?title=foo

will be handled by Feathers using Express and translated by Sequelize into an SQL query similar to:

SELECT * FROM `messages` WHERE `title` = 'foo'

of which the output is then returned to the client over JSON.

Special attributes

Sequelize, being an ORM, provides the query-construction logic. Feathers passes the entire query object from the URL as the where parameter to Sequelize Model.findAll() method.

Feathers however also supports special attribute names which can impact the query in various ways, including $sort, $limit and $select. Most of these are validated/filtered in some way, but the $select value is passed straight to the Sequelize `attributes` parameter, allowing clients to choose which SQL columns (i.e. model attributes) are included in the result.

(snippet: feathers-sequelize/lib/index.js)

_find (params = {}) {
    const { filters, query: where, paginate } = this.filterQuery(params);
    const order = utils.getOrder(filters.$sort);

    const q = Object.assign({
      where,
      order,
      limit: filters.$limit,
      offset: filters.$skip,
      raw: this.raw,
      distinct: true
    }, params.sequelize);

    if (filters.$select) {
      q.attributes = filters.$select;
    }

    const Model = this.applyScope(params);

    // ...

    return Model.findAll(q).catch(utils.errorHandler);
  }

Feathers developers likely assumed that Sequelize would accept only valid column names for attributes, or at least escape the values to prevent anything harmful from happening. However, it turns out that Sequelize assumes that these values are trusted, and even contains a special case where the escaping logic is skipped:

(snippet: sequelize/src/dialects/abstract/query-generator.js)

if (Array.isArray(attr)) {
  // ...

  if (attr[0] instanceof Utils.SequelizeMethod) {
    attr[0] = this.handleSequelizeMethod(attr[0]);
    addTable = false;
  } else if (!attr[0].includes('(') && !attr[0].includes(')')) {
    attr[0] = this.quoteIdentifier(attr[0]);
  } else {
    deprecations.noRawAttributes();
  }

  // ...
}

While a deprecation warning is triggered, control flow continues without the escaping logic! Apparently this was an intended feature to allow developers to use SQL functions like `COUNT()` in queries. In the case of our Feathers application, this allows for a full SQL injection by passing arbitrary SQL (e.g. `(1); DROP TABLE ...; --`, depending on the DB engine) as the first item of an array, within a $select value. A proof-of-concept with SQLite:

GET /messages?id=1&$select[0][]=(sqlite_version())&$select[0][1]=x

{"total":1,"limit":2,"skip":0,"data":[{"x":"3.34.0"}]}

This vulnerability was patched by Feathers (in v6.3.4) under CVE-2022-2422, and by Sequelize (in v6.29.0 and v7.0.0-alpha.20) under CVE-2023-22578 and CVE-2023-22580.

Flexible types

As mentioned above, the entire query object from Feathers is passed to the where parameter of Sequelize `Model.findAll()`. This seems to work fine in general, as this is normally a plain object with key-value pairs which are properly escaped and filtered by Sequelize. Even the documentation enforces this assumption. However, a closer look at the code shows that multiple other types are supported.

Within `Model.findAll()`, the `getWhereConditions` method is called to construct the WHERE part of SELECT and UPDATE queries. The `smth` parameter corresponds directly to the `where` parameter of the top-level query method (e.g. findAll).

(snippet: sequelize/src/dialects/abstract/query-generator.js)

getWhereConditions(smth, tableName, factory, options, prepend) {
    // ...

    if (smth && smth instanceof Utils.SequelizeMethod) { // Checking a property is cheaper than a lot of instanceof calls
      return this.handleSequelizeMethod(smth, tableName, factory, options, prepend);
    }

    if (_.isPlainObject(smth)) {
      return this.whereItemsQuery(smth, {
        model: factory,
        prefix: prepend && tableName,
        type: options.type,
      });
    }

    if (typeof smth === 'number') {
      let primaryKeys = factory ? Object.keys(factory.primaryKeys) : [];

      if (primaryKeys.length > 0) {
        // Since we're just a number, assume only the first key
        primaryKeys = primaryKeys[0];
      } else {
        primaryKeys = 'id';
      }

      where[primaryKeys] = smth;

      return this.whereItemsQuery(where, {
        model: factory,
        prefix: prepend && tableName,
      });
    }

    if (typeof smth === 'string') {
      return this.whereItemsQuery(smth, {
        model: factory,
        prefix: prepend && tableName,
      });
    }

    if (Buffer.isBuffer(smth)) {
      return this.escape(smth);
    }

    if (Array.isArray(smth)) {
      if (smth.length === 0 || smth.length > 0 && smth[0].length === 0) {
        return '1=1';
      }

      if (Utils.canTreatArrayAsAnd(smth)) {
        const _smth = { [Op.and]: smth };

        return this.getWhereConditions(_smth, tableName, factory, options, prepend);
      }

      throw new Error('Support for literal replacements in the `where` object has been removed.');
    }

    if (smth === null) {
      return this.whereItemsQuery(smth, {
        model: factory,
        prefix: prepend && tableName,
      });
    }

    return '1=1';
  }

Clearly, Sequelize developers were aiming for a more flexible system, where the caller can also pass functions, numbers, strings, buffers and arrays. We can see that the trivial 1=1 (i.e. true) value is returned in two instances:

When the smth value is an empty array, or an array with an empty array as its first element
As a fallback, when the type of smth is none of the expected types

In the case with Feathers, it is especially interesting to an attacker to construct a `WHERE 1=1` query, as this would allow them to bypass any constraints applied to the query by the application logic. This is normally done in a so-called before-hook, which is a callback that can modify the request.query object, among others. For example, a common pattern may be to enforce queries to only return data belonging to the logged-in user:

function constrainToUser(context) {
	if (!user_is_admin()) {
    // limit rows to those belonging to the user
    context.params.query.userId = session.loggedInUser;
  }
}

app.service('messages').hooks({
  before: {
    all: [constrainToUser] // Run `constrainToUser` before all requests on `/messages`
  }
})

This hook sets or overwrites the userId field of the query object, making sure that the condition is always added to the query. However, if an attacker passes an empty list for query, the following happens:

~$ node
> query = []
> query.userId = 42;
> Array.isArray(query)
true
> query.length
0

The hook will set the userId field on the array without any errors, while it is still a valid array with a length of zero. This triggers the first 1=1 scenario in Sequelize query logic as we saw above, allowing the attacker to effectively cancel out all constraints applied by before-hooks. In this case, returning the messages belonging to all users! This is pretty much what happened in our target application, causing a major loss of confidentiality.

Now the only question is: how to pass an array instead of an object for query? It turns out that this is not possible through the Feathers Express/HTTP interface (the URL query fragment is parsed by `qs`, which always returns a plain object), but it is possible through the WebSocket interface served by Socket.IO.

Normally, a Feathers query through a Socket.IO message would look like this:

42["find", "messages", {"title":"foo"}]

where the contents of query becomes the JSON-parsed version of {"title":"foo"}. To bypass the constraints as explained above we can send the following query instead:

42["find", "messages", []]

Resulting in a query without the hook-applied constraint:

SELECT * FROM `messages` WHERE 1=1

This vulnerability was patched by Feathers (in v6.3.4) under CVE-2022-29822, and by Sequelize (in v6.28.1 and v7.0.0-alpha.20) under CVE-2023-22579.

Pseudo-objects

Is it possible to hit the other 1=1 case in the Sequelize query-building logic? For the contents of query we are limited to JSON primitives (null, strings, numbers, plain objects and arrays), so it would seem impossible. However, there is always a trick! It turns out that Feathers performs a deep-copy of the query object as part of its filtering logic in cleanQuery(). Hence, we can use the `__proto__` key to perform a variant of prototype pollution where we confuse the lodash isPlainObject method into thinking our object is an array. This allows us to skip both branches in Sequelize logic and hit the fall-through case.

let _ = require('lodash'); // the library that provides `isPlainObject`

// Object as parsed from JSON
// Still a normal object, `__proto__` is not interpreted
> query = JSON.parse('{"__proto__":[]}')
> _.isPlainObject(query)
true
> Array.isArray(query)
false

// Simulating cleanQuery's assignment
// The object is now neither a "plain object" according to lodash, nor an array!
> query = {}
> query["__proto__"] = []
> _.isPlainObject(query)
false
> Array.isArray(query)
false

Thus, with the following query, we hit Sequelize fall-through case, again bypassing the constraint:

42["find", "messages", {"__proto__":[]}]

This vulnerability was patched by Feathers (in v6.3.4) under CVE-2022-29822 (as above).

More vulnerabilities: Socket.IO

We went a bit deeper into the rabbit hole and started looking at how Socket.IO parses incoming messages. We investigated whether it was possible to supply non-JSON types somehow. This led us to yet another vulnerability: a lack of type validation in Socket.IO attachment functionality.

In short, Socket.IO allows the sender to include placeholder values in the JSON payload, which will then get replaced with the contents of a binary payload from a second message, encoded in a Node Buffer object. As an example, take the following two messages:

451-0["create", "messages", {"file":{"_placeholder":true,"num":0}}]
b4<base64 data here>

Here, Socket.IO will find the inner object with the _placeholder value and replace it with attachment of index specified in num. This allows for the use of multiple numbered attachments.

(snippet: socketio/socket.io-parser/lib/binary.ts)

function _reconstructPacket(data, buffers) {
  if (!data) return data;

  if (data && data._placeholder) {
    return buffers[data.num]; // appropriate buffer (should be natural order anyway)
  } else if (Array.isArray(data)) {
    for (let i = 0; i < data.length; i++) {
      data[i] = _reconstructPacket(data[i], buffers);
    }
  } else if (typeof data === "object") {
    for (const key in data) {
      if (Object.prototype.hasOwnProperty.call(data, key)) {
        data[key] = _reconstructPacket(data[key], buffers);
      }
    }
  }

  return data;
}

To achieve this, the value of the num key is directly used as an index into the `buffers` array, without any type validation. Hence, because array indexing and method/property access are the same operation in Javascript, this allows an attacker to send a package as follows:

451-0["find", "messages", {"_placeholder":true,"num":"reverse"}]
b4

Which will result in a query object with a Function type:

> buffers = []
> buffers["reverse"]
[Function: reverse]

As such an object is not explicitly handled by Sequelize, it again triggers the fallback case of 1=1, bypassing any constraints applied in Feathers before-hooks.

This vulnerability was patched in socket.io-parser (in v4.2.1) under CVE-2022-2421.

Summary

All in all, together with DIVD we were able to classify and distinguish six interrelated issues, resulting in the following CVEs:

CVE-2022-29822 - Feathers - Improper parameter filtering in the Feathers js library, which may ultimately lead to SQL injection
CVE-2022-2422 - Feathers - SQL injection via attribute aliases
CVE-2023-22580 - Sequelize - Bad query filtering leading to SQL errors
CVE-2023-22579 - Sequelize - Unsafe fall-through in getWhereConditions
CVE-2023-22578 - Sequelize - Default support for “raw attributes” when using parentheses
CVE-2022-2421 - Socket.IO - Improper type validation in attachment parsing

Through this journey we learned to pay even closer attention to the boundaries between packages and APIs, as trust may often be misplaced or poorly communicated. We recommend developers to be explicit in their documentation and handling of input data. Especially in dynamically-typed languages like Javascript: don’t just assume, but verify input data(types).

Acknowledgements

We would like to thank DIVD for handling the complex disclosure process between the different interrelating packages. Our gratitude also goes to the maintainers of Feathers, Sequelize and Socket.IO, for their commitment to implement fixes for vulnerabilities exceeding the scope of their work.

Timeline

2022-03-23 - Codean provides DIVD with vulnerability descriptions. DIVD starts coordinated disclosure process.
2022-04-13 - Feathers maintainers apply patches
2022-06-27 - Socket.IO maintainers apply patches
2022-12-02 - Sequelize maintainers apply patches

References

Vulnerability write-up - "Dangerous assumptions"

< Back