A vulnerability in ObjectID()

[xiaofen9]

We found that ObjectID() allows an attacker to generate a malformed objectid by inserting an additional property to his user-input.

The vulnerable code is as follows: bson-objectid will return early if it detects _bsontype == ObjectID in user-input object. As a result, objects in arbitrary forms can bypass formatting if they have a valid bsontype.

https://github.com/williamkapke/bson-objectid/blob/eaaf20d828974c1a2b8e07b522b1ebdc86672f41/objectid.js#L29-L32

Unlike bson-objectid, the official implementation (shown in the following code) enforces a stricter early return condition, i.e., only if the constructor of the user-input object is ObjectID.
https://github.com/mongodb/js-bson/blob/cea186763527e4d3749a8234d335e9c395effe5b/lib/objectid.js#L59-L61

Reproduce script

var ObjectID = require("bson-objectid");

var json = {
    "mal_formkey": {
        "payload": "xxxx"
    },
    "_bsontype" : "ObjectID"
};

console.log(ObjectID(json));
console.log(ObjectID.isValid(ObjectID(json)));

[williamkapke]

I follow what you're saying. MongoDB might reject it though...? Have you tried inserting a malformed one?

The arg._bsontype==="ObjectID" statement was added for those that have both js-bson (likely via mongo) AND this library installed for whatever need.... and mingle the ObjectIDs between the modules. I suppose we could remove it and just cause it to generate a new instance based off the input? Meaning, if an instance from js-bson was passed in, it will create a new one since instanceof will fail.

[williamkapke]

Ideally, it'd be best if Mongo would break ObjectID in to it's own module!

Perhaps after all these years, they'll reconsider now that this module has gained > 30,000 weekly downloads & 237 public NPM defendants (2100 uses according to Github)! :)

[xiaofen9]

Thanks for the quick response.
I think removing _bsontype == "ObjectID" will work and I agree with you that it is better if ObjectID can be a standalone module for flexibility and consistency consideration.

[snoopysecurity]

Hi, i noticed that this issue has a CVE now (CVE-2019-19729), any plans on removing _bsontype == "ObjectID" and pushing a new release?

[williamkapke]

From what I've found so far-

The payload needs to have an id property (getter at minimum) or it will cause an error and insert fails.

Error: object [{"mal_formkey":{"payload":"xxxx"},"_bsontype":"ObjectID"}] is not a valid ObjectId at serializeObjectId ([...]/node_modules/mongodb/node_modules/bson/lib/bson/parser/serializer.js:287:11)

When it is converted to BSON, the BSON serializer looks at the _bsontype and converts it to the binary representation based on that type. So, even if there is extra data, there isn't anywhere for it to be serialized to.

Therefor, the risk of this being persisted to a MongoDB seems really low.

BUT, that's only for MongoDB. ObjectIDs are now used in many different places. (e.g.: Redis, SQL, flat files...) So, I can't gauge the risk to those unknown possibilities and it seems wise to just be sure there isn't a problem.

bson's ObjectID now uses a Buffer for the id property. This module was created when ObectID used a String to store the data. A benefit of using a Buffer is that it cannot be created from JSON. I think this module should follow suit with a major version bump. It has always used a Buffer under the hood anyhow.

I can remove _bsontype === "ObjectID" but there will be a performance impact (albeit only observed by high load codebases). With id being a buffer- I could, alternatively, add a Buffer.isBuffer check of the id property.

I've likely missed a point of view- so I wanted to throw this out there first. Thoughts?

[xiaofen9]

Thanks for disclosing the potential risk of this issue.
For your performance concern, I think we might still be able to keep the"short cut" of objectid identification. However, a more reliable metric should be adapted. For example, instead of detecting bsontype, we can detect some build-in function inheriting from ObjectID (if any). This is more reliable since attackers can't send function instance via json.

[williamkapke]

Indeed! Maybe could use toHexString() since it exists on their prototypes.

[xiaofen9]

Sound good!

[niftylettuce]

Can anyone submit a PR for this?

[niftylettuce]

Closing until PR submitted.

[huntr-helper]

‎‍🛠️ A fix has been provided for this issue. Please reference: 418sec#2

🔥 This fix has been provided through the https://huntr.dev/ bug bounty platform.