SQL-injection possible in PGStore.prototype.quotedTable #151
Comments
|
Thanks, I'll take a look at this. A friendly note though: While this isn't a very critical security flaw (if someone is injecting vulnerable schema or table names, then one surely has other severe issues) it is recommended to contact the author through a private channel rather than disclose it publicly 😉 I'll add a note about that as well 🙂 |
|
You are correct, I should have done that, and I would have if it was more serious. I will soon make a new pull request with a minimal fix for this. The best way I have found so far is to use quote_ident, a sql-function
But that requires a query to the server, and if it isn't available at startup the table name can't be determinated. A quick fix is to double-quote duouble-quoted characters in Javascript. That's what I'm going to do in my pull request. |
|
I have a fix ready to push, just doing some final adjustments
ons 21 aug. 2019 kl. 22:53 skrev Robert Nilsson <notifications@github.com>:
… You are correct, I should have done that, and I would have if it was more
serious.
As you noticed, a project that is vulnerable to this probably have more
problems...
I will soon make a new pull request with a minimal fix for this.
The best way I have found so far is to use quote_ident
<https://www.postgresql.org/docs/10/functions-string.html>, a sql-function
quote_ident(string text)
Return the given string suitably quoted to be used as an identifier in an
SQL statement string. Quotes are added only if necessary (i.e., if the
string contains non-identifier characters or would be case-folded).
Embedded quotes are properly doubled.
But that requires a query to the server, and if it isn't available at
startup the table name can't be determinated.
A quick fix is to double-quote duouble-quoted characters in Javascript.
That's what I'm going to do in my pull request.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#151?email_source=notifications&email_token=AAAINGMYE26BWDYQKEMI663QFWTL7A5CNFSM4IOFI5L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD43C52Y#issuecomment-523644651>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAINGJNBYRD4FKDOJ6IQZLQFWTL7ANCNFSM4IOFI5LQ>
.
|
|
Fixed in |
The function PGStore.prototype.quotedTable was vulnerable since it
didn't correctly handle schema and table names with double quotes.
A specially crafted schema name could have deleted the content of
tables every time the prune process was running.
One way to solve this is to make a call to the server and let the
function quote_ident take care of the quoting. But since that requires a
call to the server at initialization time, and the server might not be
reachable, it quickly becomes complicated.
I solved it by adding two helper functions:
quoteIdent takes care of the double quoting by duplicating double quotes:
const quoteIdent = function (ident) {
return '"' + ident.replace(/"/g, '""') + '"';
};
qualifiedTable returns a quoted string with the schema name if needed.
const qualifiedTable = function (schema, table) {
return (schema ? quoteIdent(schema) + '.' : '') + quoteIdent(table);
};
Instead of concatenatening and quoting the schema and table name at
every query, I changed it to concetenate the table name at
initialization:
this.qualifiedTableName = qualifiedTable(this.schemaName, this.tableName);
and updated quotedTable to use the calculated value.
PGStore.prototype.quotedTable = function () {
return this.qualifiedTableName;
};
I did not remove quotedTable since it might break some third party code.
I also changed every instance where quotedTable() was called, to use
this.qualifiedTableName directly.
- this.query('DELETE FROM ' + this.quotedTable() + ' WHERE expire < to_timestamp($1)', [currentTimestamp()], function (err) {
+ this.query('DELETE FROM ' + this.qualifiedTableName + ' WHERE expire < to_timestamp($1)', [currentTimestamp()], function (err) {
- this.query('SELECT sess FROM ' + this.quotedTable() + ' WHERE sid = $1 AND expire >= to_timestamp($2)', [sid, currentTimestamp()], function (err, data) {
+ this.query('SELECT sess FROM ' + this.qualifiedTableName + ' WHERE sid = $1 AND expire >= to_timestamp($2)', [sid, currentTimestamp()], function (err, data) {
- const query = 'INSERT INTO ' + this.quotedTable() + ' (sess, expire, sid) SELECT $1, to_timestamp($2), $3 ON CONFLICT (sid) DO UPDATE SET sess=$1, expire=to_timestamp($2) RETURNING sid';
+ const query = 'INSERT INTO ' + this.qualifiedTableName + ' (sess, expire, sid) SELECT $1, to_timestamp($2), $3 ON CONFLICT (sid) DO UPDATE SET sess=$1, expire=to_timestamp($2) RETURNING sid';
- this.query('DELETE FROM ' + this.quotedTable() + ' WHERE sid = $1', [sid], function (err) {
+ this.query('DELETE FROM ' + this.qualifiedTableName + ' WHERE sid = $1', [sid], function (err) {
- 'UPDATE ' + this.quotedTable() + ' SET expire = to_timestamp($1) WHERE sid = $2 RETURNING sid',
+ 'UPDATE ' + this.qualifiedTableName + ' SET expire = to_timestamp($1) WHERE sid = $2 RETURNING sid',
|
An advisory has been published: GHSA-xqh8-5j36-4556 |
The function PGStore.prototype.quotedTable is vulnerable to SQL-injection, if the input has double quotes. If schemaName is set to 'web".session WHERE $1=$1;--' it will wipe the web.session table every time the prune process runs.
There is a function quote_ident that could be used:
Calling this function will require a call to the server and requires that the server is available before the table name can be resolved. This call could also get the version of the server, and warn the user if the server version is too old.
The text was updated successfully, but these errors were encountered: