From aee54d119d46c538ba8a25725e1a35be145c67cc Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Wed, 21 Jun 2017 09:37:52 -0400 Subject: [PATCH 1/3] Add failing test for dollarQuotedString() Add a test for dollarQuotedString() to showcase how it fails to handle the randomly chosen dollar quote tag appearing in the string to be quoted. The test works by repeatedly trying to quote $a$ until the string itself is the randomly chosen dollar quote tag. --- test/index.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/index.js b/test/index.js index 10baaa8..c67b45d 100644 --- a/test/index.js +++ b/test/index.js @@ -63,6 +63,17 @@ describe('escape.dollarQuotedString(val)', function() { escape.dollarQuotedString(15).should.match(/\$[a-z]{1}\$15\$[a-z]\$/); escape.dollarQuotedString('something').should.match(/\$[a-z]{1}\$something\$[a-z]\$/); }) + + it('should handle dollar quotes in the string being escaped without resorting to luck', function(){ + var sql = 'SELECT $a$test$a$'; + // The only occurrences of $a$ in the string should be from the string itself. + // The repeated test from the for loop is to catch $a$ randomly being chosen as the tag. + for(var i=0;i<1000;i++) { + var escapedSql = escape.dollarQuotedString(sql); + var count = escapedSql.match(/\$a\$/g).length; + assert(count === 2); + } + }); }) describe('escape.ident(val)', function(){ From c976727cbcc0dba938625164f7209300d102655a Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Wed, 21 Jun 2017 09:25:01 -0400 Subject: [PATCH 2/3] Fix dollarQuotedString() to handle dollar quotes in text Fixes a bug in dollarQuotedString() where it fails to account for the case where the string being quoted contains the randomly chosen dollar quote tag. Before the fix, if a string containing $a$ there is a one in 21 (number of values in randomTags) chance of it occurring. With the fix the function now checks if the chosen dollar quote tag is contained within the string to be quoted and if so chooses a new longer random tag. The tag initially starts as the empty string so the initial default choice for quoting becomes $$. Random additions to the tag were chosen, over incrementing a counter, to make it slightly more difficult for an attacker to supply an input string that would require a large number of iterations. As the new defualt is to use $$ for quoting, a number of the results for unit tests have been simplified to match on the exact text that would be returned. --- index.js | 19 +++++++++++++++++-- test/index.js | 14 ++++++++++---- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index 4c66afa..6696484 100644 --- a/index.js +++ b/index.js @@ -84,6 +84,9 @@ function random(start, end) { * Format as dollar quoted string. * see: http://www.postgresql.org/docs/8.3/interactive/sql-syntax-lexical.html#SQL-SYNTAX-DOLLAR-QUOTING * + * Initially tries to use a tagless $$ as the quote wrapper. If it appears in the string to be quoted, then + * successively longer random tags are chosen until one is found that is not contained within the string. + * * @param {Mixed} val * @return {String} * @api public @@ -91,8 +94,20 @@ function random(start, end) { exports.dollarQuotedString = function(val) { if (val === undefined || val === null || val === '') return ''; - var randomTag = '$'+ randomTags[ random(0, randomTags.length) ] +'$'; - return randomTag + val + randomTag; + // Ensure val is coerced to a string + val = '' + val; + // Start with an empty tag: $$ + var tag = ''; + while (true) { + var dollarQuote = '$'+ tag + '$'; + // Check if val contains our selected dollar quote tag + if (val.indexOf(dollarQuote) < 0) { + // Not contained so dollarQuote is safe to use + return dollarQuote + val + dollarQuote; + } + // Tag was contained within val so add random character to it + tag += randomTags[ random(0, randomTags.length) ]; + } } /** diff --git a/test/index.js b/test/index.js index c67b45d..1dfa491 100644 --- a/test/index.js +++ b/test/index.js @@ -42,7 +42,7 @@ describe('escape(fmt, ...)', function(){ describe('%Q', function(){ it('should format as a dollar quoted string', function(){ escape('%Q', "Tobi's") - .should.match(/\$[a-z]{1}\$Tobi's\$[a-z]\$/); + .should.match("$$Tobi's$$"); }) }) }) @@ -59,9 +59,15 @@ describe('escape.string(val)', function(){ describe('escape.dollarQuotedString(val)', function() { it('should coerce to a dollar quoted string', function(){ escape.dollarQuotedString().should.equal(''); - escape.dollarQuotedString(0).should.match(/\$[a-z]{1}\$0\$[a-z]\$/); - escape.dollarQuotedString(15).should.match(/\$[a-z]{1}\$15\$[a-z]\$/); - escape.dollarQuotedString('something').should.match(/\$[a-z]{1}\$something\$[a-z]\$/); + escape.dollarQuotedString(0).should.equal('$$0$$'); + escape.dollarQuotedString(15).should.equal('$$15$$'); + escape.dollarQuotedString('something').should.equal('$$something$$'); + }) +}) + +describe('escape.dollarQuotedString(val)', function() { + it('should handle quoting strings with dollar quotes in them', function(){ + escape.dollarQuotedString('$$').should.match(/\$[a-z]+\$\$\$\$[a-z]+\$/); }) it('should handle dollar quotes in the string being escaped without resorting to luck', function(){ From dcffe45e5f772c9b5569fe385e8b79867c015547 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Wed, 21 Jun 2017 09:50:49 -0400 Subject: [PATCH 3/3] Expand set of random tag chars for dollar quotes Change the set of characters used to generate random tags for dollar quotes to include both lower and upper case non-vowel characters and the digits two (2) through nine (9). The overall expanded set decreases the likelyhood of a randomly picked tag existing in the contained input. The exclusions are to ensure that generated tags do not randomly end up including naughty words. --- index.js | 12 ++++++++++-- test/index.js | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 6696484..0329aa3 100644 --- a/index.js +++ b/index.js @@ -63,8 +63,16 @@ exports.string = function(val){ * Dollar-Quoted String Constants */ -var randomTags = [ 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'g', 'j', 'k', - 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't']; +var randomTags = [ + // Upper case alpha sans vowels + 'B', 'C', 'D', 'F', 'G', 'H', 'J', 'K', 'L', 'M', + 'N', 'P', 'Q', 'R', 'S', 'T', 'V', 'W', 'X', 'Z', + // Lower case alpha sans vowels + 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', + 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'z', + // Numeric two through nine + '2', '3', '4', '5', '6', '7', '8', '9', +]; /** * produces a random number from a given range diff --git a/test/index.js b/test/index.js index 1dfa491..a5c6b58 100644 --- a/test/index.js +++ b/test/index.js @@ -67,7 +67,7 @@ describe('escape.dollarQuotedString(val)', function() { describe('escape.dollarQuotedString(val)', function() { it('should handle quoting strings with dollar quotes in them', function(){ - escape.dollarQuotedString('$$').should.match(/\$[a-z]+\$\$\$\$[a-z]+\$/); + escape.dollarQuotedString('$$').should.match(/\$[a-zA-Z0-9]+\$\$\$\$[a-zA-Z0-9]+\$/); }) it('should handle dollar quotes in the string being escaped without resorting to luck', function(){