Skip to content

Commit

Permalink
Fixed code execution vulnerability due to Object coercion
Browse files Browse the repository at this point in the history
refs GHSA-jqv5-7xpx-qj74
fixes TryGhost/Toolbox#491

- when you call `ToString()` on `Napi::Value`, it calls
  `napi_coerce_to_string` underneath, which has the ability to run
  arbitrary JS code if the passed in value is a crafted object
- both remote code execution or denial-of-service are possible via
  this vulnerability
- `toString()` on an Object returns `[object Object]` so instead of
  calling the function, we're going to hardcode it to prevent this
  issue

Credits: Dave McDaniel of Cisco Talos
  • Loading branch information
daniellockyer committed Mar 13, 2023
1 parent 3a48888 commit edb1934
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/statement.cc
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ template <class T> Values::Field*
return new Values::Float(pos, source.ToNumber().DoubleValue());
}
else if (source.IsObject()) {
Napi::String napiVal = source.ToString();
Napi::String napiVal = Napi::String::New(source.Env(), "[object Object]");
// Check whether toString returned a value that is not undefined.
if(napiVal.Type() == 0) {
return NULL;
Expand Down
16 changes: 16 additions & 0 deletions test/other_objects.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,20 @@ describe('data types', function() {
});
});

it('should ignore faulty toString in array', function(done) {
const faulty = [[{toString: null}], 1];
db.all('SELECT * FROM txt_table WHERE txt = ? LIMIT ?', faulty, function (err) {
assert.equal(err, null);
done();
});
});

it('should ignore faulty toString set to function', function(done) {
const faulty = [[{toString: function () {console.log('oh no');}}], 1];
db.all('SELECT * FROM txt_table WHERE txt = ? LIMIT ?', faulty, function (err) {
assert.equal(err, undefined);
done();
});
});

});

5 comments on commit edb1934

@cesarkohl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.

@shubhamp-sf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can anyone explain this change? It's breaking for some of our projects.
Data stored in json is being returned as [object Object].

cc: @daniellockyer

@shubhamp-sf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue: #1694

@mvduin
Copy link

@mvduin mvduin commented on edb1934 Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds completely nonsensical... exploiting this "vulnerability" requires the attacker to have the ability to provide node-sqlite3 with arguments that have a custom toString() containing the attacker's code. If an attacker can already introduce custom code into your application like that they already have all the access they could want and don't need node-sqlite3 for anything.

@mvduin
Copy link

@mvduin mvduin commented on edb1934 Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this "fix" is extremely inappropriate. Even if we were to pretend that implicitly stringifying objects is a security vulnerability, the correct thing to do would be to throw a TypeError, not silently corrupt data being inserted into a database. This is horrible.

Please sign in to comment.