Prototype Pollution of getToken and putToken when identityIdis __proto__

[amydevs]

Describe the bug

Due to JS reserving the use of the __proto__ key on objects, and getToken and setToken using an object to store the tokens for all connected identities for any given provider, prototype pollution will occur.

To Reproduce

testProp(
    'get, set and unset tokens',
    [identitiesTestUtils.identitiyIdArb, identitiesTestUtils.providerTokenArb],
    async (identityId, providerToken) => {
      const identitiesManager = await IdentitiesManager.createIdentitiesManager(
        {
          db,
          keyRing: dummyKeyRing,
          gestaltGraph: dummyGestaltGraph,
          sigchain: dummySigchain,
          logger,
          fresh: true,
        },
      );
      const providerId = 'test-provider' as ProviderId;
     // identityId = '__proto__';
      await identitiesManager.putToken(providerId, identityId, providerToken);
      const providerToken_ = await identitiesManager.getToken(
        providerId,
        identityId,
      );
      expect(providerToken).toStrictEqual(providerToken_);
      await identitiesManager.delToken(providerId, identityId);
      await identitiesManager.delToken(providerId, identityId);
      const providerToken__ = await identitiesManager.getToken(
        providerId,
        identityId,
      );
      expect(providerToken__).toBeUndefined();
      await identitiesManager.stop();
    }, { seed: 568365398 }
  );

Expected behavior

Using __proto__ as the identityId should correctly set the token.

Screenshots

  ● IdentitiesManager › get, set and unset tokens (with seed=568365398)

    Property failed after 5 tests
    { seed: 568365398, path: "4:84:84:84:84:84:84:84:84:84:84:84:84:84", endOnFailure: true }
    Counterexample: ["__proto__",{"accessToken":"          ","refreshToken":"","accessTokenExpiresIn":0,"refreshTokenExpiresIn":0}]
    Shrunk 13 time(s)
    Got error: expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 6

    - Object {}
    + Object {
    +   "accessToken": "          ",
    +   "accessTokenExpiresIn": 0,
    +   "refreshToken": "",
    +   "refreshTokenExpiresIn": 0,
    + }

      120 |         identityId,
      121 |       );
    > 122 |       expect(providerToken).toStrictEqual(providerToken_);
          |                             ^
      123 |       await identitiesManager.delToken(providerId, identityId);
      124 |       await identitiesManager.delToken(providerId, identityId);
      125 |       const providerToken__ = await identitiesManager.getToken(

      at toStrictEqual (tests/identities/IdentitiesManager.test.ts:122:29)
      at AsyncProperty.run (node_modules/fast-check/lib/check/property/AsyncProperty.generic.js:49:28)
      Hint: Enable verbose mode in order to have the list of all failing values encountered during the run
      at buildError (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:131:15)
      at asyncThrowIfFailed (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:148:11)

  ● IdentitiesManager › start and stop preserves state (with seed=568365398)

    Property failed after 5 tests
    { seed: 568365398, path: "4:84:84:84:84:84:84:84:84:84:84:84:84:84", endOnFailure: true }
    Counterexample: ["__proto__",{"accessToken":"          ","refreshToken":"","accessTokenExpiresIn":0,"refreshTokenExpiresIn":0}]
    Shrunk 13 time(s)
    Got error: expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 6

    - Object {}
    + Object {
    +   "accessToken": "          ",
    +   "accessTokenExpiresIn": 0,
    +   "refreshToken": "",
    +   "refreshTokenExpiresIn": 0,
    + }

      161 |         identityId,
      162 |       );
    > 163 |       expect(providerToken).toStrictEqual(providerToken_);
          |                             ^
      164 |       expect(identitiesManager.getProviders()).toStrictEqual({
      165 |         [testProvider.id]: testProvider,
      166 |       });

      at toStrictEqual (tests/identities/IdentitiesManager.test.ts:163:29)
      at AsyncProperty.run (node_modules/fast-check/lib/check/property/AsyncProperty.generic.js:49:28)
      Hint: Enable verbose mode in order to have the list of all failing values encountered during the run
      at buildError (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:131:15)
      at asyncThrowIfFailed (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:148:11)

Test Suites: 1 failed, 1 total
Tests:       2 failed, 5 passed, 7 total
Snapshots:   0 total
Time:        11.233 s
Ran all test suites matching /.\/tests\/identities\//i.
GLOBAL TEARDOWN
Destroying Global Data Dir: /tmp/polykey-test-global-X7tBig

Platform (please complete the following information)

• Device: Dell Precision 3470
• OS: NixOS
• Version: Node v20.5.1

Additional context

Notify maintainers

@amydevs

[CMCDragonkai]

Interesting:

> JSON.parse('{"__proto__": 123}')
{ ['__proto__']: 123 }
> JSON.stringify({'__proto__': 123})
'{}'

This seems like such a strange behaviour. But it's really that __proto__ can't actually be put into as a key.

[CMCDragonkai]

> o = {}
{}
> o['__proto__'] = 123
123
> o
{}

[CMCDragonkai]

But you can do it like this:

> o = {}
{}
> Object.defineProperty(o, '__proto__', { value: 123, writable: true, enumerable: true });
{ ['__proto__']: 123 }
> o
{ ['__proto__']: 123 }

[CMCDragonkai]

Then you can stringify:

> JSON.stringify(o)
'{"__proto__":123}'

[CMCDragonkai]

> JSON.parse(JSON.stringify(o))
{ ['__proto__']: 123 }
> o['__proto__']
123

[CMCDragonkai]

So this is how you're meant to define such a property:

> o = { a: 123 }
{ a: 123 }
> Object.getOwnPropertyDescriptor(o, 'a')
{ value: 123, writable: true, enumerable: true, configurable: true }
> Object.defineProperty(o, '__proto__', { value: 123, writable: true, enumerable: true, configurable: true });
{ a: 123, ['__proto__']: 123 }
> Object.getOwnPropertyDescriptor(o, '__proto__')
{ value: 123, writable: true, enumerable: true, configurable: true }

So the real fix is to ensure that our objects are set with properties using Object.defineProperty in the instance of receiving the IdentityId from a foreign source.

[CMCDragonkai]

Here's another way to do it:

> o = { ['__proto__']: 123 }
{ ['__proto__']: 123 }
> o['__proto__']
123

Or:

> identityId = '__proto__'
'__proto__'
> o = { [identityId]: 123 }
{ ['__proto__']: 123 }

But this doesn't work:

> o = {}
{}
> o[identityId] = 123
123
> o
{}
> o[[identityId]] = 123
123
> o
{}
> o[`${identityId}`] = 123
123
> o
{}

So if you have o already, you have to use Object.defineProperty with all the parameters. But if you can declare o, then you can use [identityId]: 123.