superjson throws on constructor property

[Janpot]

superjson recently started throwing on values that contain a constructor property:

superjson.stringify({ constructor: { name: 'hello' } })
// throws Error: Detected property constructor. This is a prototype pollution risk, please remove it from your object.

We're transferring values that contain jsonschemas that describe user generated typescript interfaces. Therefore we don't have control over which property names are in our objects, but we do know they are plain javascript objects.

Any way this restriction can be relaxed again?

[Skn0tt]

This error comes from here:

https://github.com/blitz-js/superjson/blob/0130a809f87df142b5e190e1b519be0c0280fb16/src/plainer.ts#L219-L227

In your case, there's no transform happening so I don't think there's a true risk for prototype pollution. Let me see if we can relax this ...

[Janpot]

Does this mean that the parsing side uses these properties to "guess" the original constructor without any other marker in the metadata? That feels very brittle to me.

[Skn0tt]

No, the parsing side reads the metadata to find the original prototype / constructor. The exception you're seeing was added in response to #267 - we're trying to catch invalid inputs on parsing, not on serialisation. Otherwise you'll store something in your database, only to realise that you can't retrieve it anymore when parsing.

[Janpot]

Otherwise you'll store something in your database, only to realise that you can't retrieve it anymore when parsing.

It's a bit counterintuitive, because I can perfectly parse superjson which it's unable to serialize again.

const parsed = superjson.parse(
    '{"json":{"schema":{"constructor":{"type":"string"}},"createdAt":"2023-12-06T10:49:28.911Z"},"meta":{"values":{"createdAt":["Date"]}}}'
  )

console.log(typeof parsed);
// 'object'
console.log(superjson.stringify(parsed));
// Error: Detected property constructor. This is a prototype pollution risk, please remove it from your object.

[Skn0tt]

This specific value should be stringifiable, I agree. I believe the open PR already addressed that, will add a test case 👍

[Skn0tt]

Test case is over here, and it's green: 5fa55b7

The important thing here is that constructor doesn't appear in meta, so it's not a prototype pollution risk. If you added e.g. a regex to the constructor object, it would appear in meta, and would become a risk.

[Janpot]

I may still be misunderstanding the attack vector fully but I'm reading this section. Do I understand that when assigning to nested properties of referentialEqualities, superjson doesn't discriminate between own properties and prototype properties? Wouldn't it then be possible to effectively eliminate the attack vector, while traversing the property path, to test whether each nested one is an own property before assigning to it. And error out in case it's not an "own property". Which would only happen on specially crafted serialized form, and never with something that was serialized with superjson.

In pseudo code

function getSafeNested(obj, [next, ...rest]) {
  if (!Object.prototype.hasOwnProperty.call(obj, next)) {
    throw new Error('checkmate hackers')
  }
  return rest.length === 0 ? obj[next] : getSafeNested(obj[next], rest)
}

function setSafeNested(obj, [next, ...rest], value) {
  if (!Object.prototype.hasOwnProperty.call(obj, next)) {
    throw new Error('checkmate hackers')
  }
  if (rest.length > 0) {
    setSafeNested(obj[next], rest, value)
  } else {
    obj[next] = value
  }
}

for (const [src, tgts] of Object.entries(serialized.meta.referentialEqualities)) {
  const srcValue = getSafeNested(serialized.json, src.split('.'))
  for (const tgt of tgts) {
    setSafeNested(serialized.json, tgt.split('.'), srcValue)
  }
}

Assuming you already considered this, what am I missing?

[Skn0tt]

I have not considered this yet, actually. If we can prevent prototype pollution with this, i'd be happy to! @Janpot would you be willing to work on a PR with this?

[Janpot]

@Janpot would you be willing to work on a PR with this?

I won't in the near future, but I may pick this up at some point if it doesn't get implemented.

[Janpot]

@Skn0tt To be honest, I'm having a bit of trouble replicating the original prototype pollution. Did they ever provide an actual payload that triggers it?

[Skn0tt]

Yes, there were some test cases added for it: 0d68cd5#diff-258035e5968f6bf645400d417f310218d7d9a9a10606a3c34e7f55db193f58f3

[Janpot]

Yep, I noticed. You'd think they'd fail if I remove the check for forbidden properties. But the prototype doesn't seem to get polluted in the test, this assert passes: https://github.com/blitz-js/superjson/pull/284/files#diff-258035e5968f6bf645400d417f310218d7d9a9a10606a3c34e7f55db193f58f3L1125

[Skn0tt]

I see you only left the test for __proto__ in - are you sure that it also doesn't get polluted when the prototype or constructor property is used? The exact property can differ between browser implementations, so I don't think we can blindly trust CI here.

[Janpot]

The exact property can differ between browser implementations, so I don't think we can blindly trust CI here.

I understood that the specific CVE makes use of prototype pollution in Node.js. I ran the tests in Node.js. The CI runs on Node.js as well so I don't see why it wouldn't be a proper target. In Node.js I can pollute the prototype manually with:

{}.__proto__.foo = 'hello';
console.log({}.foo)
// "hello"

I've been trying all kinds of ways to trigger a prototype pollution in superjson after removing the property validation, I can't seem to come up with something.

[Skn0tt]

I've tried to replicate one, but also couldnt' come up with something that successfully pollutes, but I think i'm very close. I pushed an example to your PR. When you attach a debugger, you'll see that once it gets to this line:

https://github.com/Janpot/superjson/blob/7c1b5f75e77a29204a815deb30ea087d460684d1/src/accessDeep.ts#L82

it will have the Object prototype in the parent variable. In the lines below, it would write onto parent[lastKey], which would pollute the Object prototype. The only reason it doesn't is that Object doesn't satisfy any of the type checks below, like isPlainObject - but my gut feeling is that I just haven't found the right payload yet.

[Janpot]

The isPlainObject blocks traversing into Object.prototype because it rejects Object.prototype as a plain object and only accepts objects that have Object.prototype as a prototype or are created with Object.create(null). It seems to work to block prototype pollution as suggested by the test payload and anything I can come up with, but really this should just use Object.hasOwnProperty.