Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hgetall mishandles malicious keys (__proto__) #1267

Closed
davidje13 opened this issue Dec 27, 2020 · 2 comments · Fixed by #1416 · 4 remaining pull requests
Closed

hgetall mishandles malicious keys (__proto__) #1267

davidje13 opened this issue Dec 27, 2020 · 2 comments · Fixed by #1416 · 4 remaining pull requests
Labels

Comments

@davidje13
Copy link

davidje13 commented Dec 27, 2020

This isn't a global prototype pollution attack, but does mean it's possible to get inconsistent behaviour:

await client.hset('test_key', ['__proto__', 'hello']);
console.log('hget:', await client.hget('test_key', '__proto__'))); // "hello"
console.log('hgetall:', await client.hgetall('test_key'))); // does not include __proto__: hello

It's because the reply transformer which is applied does not check for special field names: https://github.com/luin/ioredis/blob/master/lib/command.ts#L404

Since this isn't doing a recursive set, it can't cause global prototype pollution, but it can cause unexpected inconsistent behaviour as shown above. Obviously this is mainly a concern if allowing user-provided field names (e.g. a database explorer UI). Since the database itself can handle this name, I think the client library should be able to handle it too.

The easiest fix would be something like:

    const obj = {};
    for (let i = 0; i < result.length; i += 2) {
      const key = result[i];
      const value = result[i + 1];
      if (obj[key]) { // can only be truthy if the property is special somehow, like '__proto__' or 'constructor'
        Object.defineProperty(obj, key, {
          value,
          configurable: true,
          enumerable: true,
          writable: true,
        });
      } else {
        obj[key] = value;
      }
    }
    return obj;

But a better fix would be to change the API to return a Map instead.

None of the other commands seem to have the same issue; hgetall is the only command which uses setReplyTransformer, and the ArgumentTransformers are fine (e.g. hset('foo', JSON.parse('{"__proto__":"yay"}')) works)


Relatedly, it would be nice to be able to call hgetall without the reply transformer (returning a raw key/value alternating list) for situations where the intent is to iterate through all fields anyway. It's interesting that the transformer has a condition checking for arrays; is it possible for the server to send a different data type? The main documentation says it will always be a list.

@kushagranigam
Copy link

kushagranigam commented Aug 11, 2021

Veracode reported the package for this vulnerability. From the issue description, I believe this wouldn't impact the applications that aren't directly allowing user-provided field names. Adding link to Veracode SCA finding page for list of vulnerable versions:

https://sca.analysiscenter.veracode.com/vulnerability-database/security/sca/vulnerability/sid-28827/summary

It would be great to see a fix as suggested by @davidje13 in the issue in the upcoming versions. Thanks.

luin added a commit that referenced this issue Aug 18, 2021
luin added a commit that referenced this issue Aug 18, 2021
ioredis-robot pushed a commit that referenced this issue Aug 18, 2021
## [4.27.8](v4.27.7...v4.27.8) (2021-08-18)

### Bug Fixes

* handle malicious keys for hgetall ([#1416](#1416)) ([7d73b9d](7d73b9d)), closes [#1267](#1267)
@ioredis-robot
Copy link
Collaborator

🎉 This issue has been resolved in version 4.27.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

rethab pushed a commit to rethab/probot that referenced this issue Sep 1, 2021
  * the previously used version of ioredis was vulnerable to a prototype
    pollution attack: redis/ioredis#1267
rethab pushed a commit to rethab/probot that referenced this issue Sep 1, 2021
  * the previously used version of ioredis was vulnerable to a prototype
    pollution attack: redis/ioredis#1267
rethab added a commit to rethab/probot that referenced this issue Sep 1, 2021
  * the previously used version of ioredis was vulnerable to a
    prototype pollution attack: redis/ioredis#1267
gr2m pushed a commit to probot/probot that referenced this issue Sep 1, 2021
the previously used version of ioredis was vulnerable to a prototype pollution attack: redis/ioredis#1267
janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this issue Mar 1, 2024
## [4.27.8](redis/ioredis@v4.27.7...v4.27.8) (2021-08-18)

### Bug Fixes

* handle malicious keys for hgetall ([#1416](redis/ioredis#1416)) ([7d73b9d](redis/ioredis@7d73b9d)), closes [#1267](redis/ioredis#1267)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment