Add more SASL validation and fix tests

packages/pg/lib/sasl.js

@@ -20,19 +20,27 @@ function continueSession(session, password, serverData) {
   if (session.message !== 'SASLInitialResponse') {
     throw new Error('SASL: Last message was not SASLInitialResponse')
   }
+  if (typeof password !== 'string') {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a string')
+  }
+  if (typeof serverData !== 'string') {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: serverData must be a string')
+  }
 
-  const sv = extractVariablesFromFirstServerMessage(serverData)
+  const sv = parseServerFirstMessage(serverData)
 
   if (!sv.nonce.startsWith(session.clientNonce)) {
     throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: server nonce does not start with client nonce')
+  } else if (sv.nonce.length === session.clientNonce.length) {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: server nonce is too short')
   }
 
   var saltBytes = Buffer.from(sv.salt, 'base64')
 
   var saltedPassword = Hi(password, saltBytes, sv.iteration)
 
-  var clientKey = createHMAC(saltedPassword, 'Client Key')
-  var storedKey = crypto.createHash('sha256').update(clientKey).digest()
+  var clientKey = hmacSha256(saltedPassword, 'Client Key')
+  var storedKey = sha256(clientKey)
 
   var clientFirstMessageBare = 'n=*,r=' + session.clientNonce
   var serverFirstMessage = 'r=' + sv.nonce + ',s=' + sv.salt + ',i=' + sv.iteration
@@ -41,12 +49,12 @@ function continueSession(session, password, serverData) {
 
   var authMessage = clientFirstMessageBare + ',' + serverFirstMessage + ',' + clientFinalMessageWithoutProof
 
-  var clientSignature = createHMAC(storedKey, authMessage)
+  var clientSignature = hmacSha256(storedKey, authMessage)
   var clientProofBytes = xorBuffers(clientKey, clientSignature)
   var clientProof = clientProofBytes.toString('base64')
 
-  var serverKey = createHMAC(saltedPassword, 'Server Key')
-  var serverSignatureBytes = createHMAC(serverKey, authMessage)
+  var serverKey = hmacSha256(saltedPassword, 'Server Key')
+  var serverSignatureBytes = hmacSha256(serverKey, authMessage)
 
   session.message = 'SASLResponse'
   session.serverSignature = serverSignatureBytes.toString('base64')
@@ -57,54 +65,87 @@ function finalizeSession(session, serverData) {
   if (session.message !== 'SASLResponse') {
     throw new Error('SASL: Last message was not SASLResponse')
   }
+  if (typeof serverData !== 'string') {
+    throw new Error('SASL: SCRAM-SERVER-FINAL-MESSAGE: serverData must be a string')
+  }
 
-  var serverSignature
-
-  String(serverData)
-    .split(',')
-    .forEach(function (part) {
-      switch (part[0]) {
-        case 'v':
-          serverSignature = part.substr(2)
-          break
-      }
-    })
+  const { serverSignature } = parseServerFinalMessage(serverData)
 
   if (serverSignature !== session.serverSignature) {
     throw new Error('SASL: SCRAM-SERVER-FINAL-MESSAGE: server signature does not match')
   }
 }
 
-function extractVariablesFromFirstServerMessage(data) {
-  var nonce, salt, iteration
-
-  String(data)
-    .split(',')
-    .forEach(function (part) {
-      switch (part[0]) {
-        case 'r':
-          nonce = part.substr(2)
-          break
-        case 's':
-          salt = part.substr(2)
-          break
-        case 'i':
-          iteration = parseInt(part.substr(2), 10)
-          break
+/**
+ * printable       = %x21-2B / %x2D-7E
+ *                   ;; Printable ASCII except ",".
+ *                   ;; Note that any "printable" is also
+ *                   ;; a valid "value".
+ */
+function isPrintableChars(text) {
+  if (typeof text !== 'string') {
+    throw new TypeError('SASL: text must be a string')
+  }
+  return text
+    .split('')
+    .map((_, i) => text.charCodeAt(i))
+    .every((c) => (c >= 0x21 && c <= 0x2b) || (c >= 0x2d && c <= 0x7e))
+}
+
+/**
+ * base64-char     = ALPHA / DIGIT / "/" / "+"
+ *
+ * base64-4        = 4base64-char
+ *
+ * base64-3        = 3base64-char "="
+ *
+ * base64-2        = 2base64-char "=="
+ *
+ * base64          = *base64-4 [base64-3 / base64-2]
+ */
+function isBase64(text) {
+  return /^(?:[a-zA-Z0-9+/]{4})*(?:[a-zA-Z0-9+/]{2}==|[a-zA-Z0-9+/]{3}=)?$/.test(text)
+}
+
+function parseAttributePairs(text) {
+  if (typeof text !== 'string') {
+    throw new TypeError('SASL: attribute pairs text must be a string')
+  }
+
+  return new Map(
+    text.split(',').map((attrValue) => {
+      if (!/^.=/.test(attrValue)) {
+        throw new Error('SASL: Invalid attribute pair entry')
       }
+      const name = attrValue[0]
+      const value = attrValue.substring(2)
+      return [name, value]
     })
+  )
+}
 
+function parseServerFirstMessage(data) {
+  const attrPairs = parseAttributePairs(data)
+
+  const nonce = attrPairs.get('r')
   if (!nonce) {
     throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: nonce missing')
+  } else if (!isPrintableChars(nonce)) {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: nonce must only contain printable characters')
   }
-
+  const salt = attrPairs.get('s')
   if (!salt) {
     throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: salt missing')
+  } else if (!isBase64(salt)) {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: salt must be base64')
   }
-
-  if (!iteration) {
+  const iterationText = attrPairs.get('i')
+  if (!iterationText) {
     throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: iteration missing')
+  } else if (!/^[1-9][0-9]*$/.test(iterationText)) {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: invalid iteration count')
   }
+  const iteration = parseInt(iterationText, 10)
 
   return {
     nonce,
@@ -113,31 +154,48 @@ function extractVariablesFromFirstServerMessage(data) {
   }
 }
 
+function parseServerFinalMessage(serverData) {
+  const attrPairs = parseAttributePairs(serverData)
+  const serverSignature = attrPairs.get('v')
+  if (!serverSignature) {
+    throw new Error('SASL: SCRAM-SERVER-FINAL-MESSAGE: server signature is missing')
+  } else if (!isBase64(serverSignature)) {
+    throw new Error('SASL: SCRAM-SERVER-FINAL-MESSAGE: server signature must be base64')
+  }
+  return {
+    serverSignature,
+  }
+}
+
 function xorBuffers(a, b) {
-  if (!Buffer.isBuffer(a)) a = Buffer.from(a)
-  if (!Buffer.isBuffer(b)) b = Buffer.from(b)
-  var res = []
-  if (a.length > b.length) {
-    for (var i = 0; i < b.length; i++) {
-      res.push(a[i] ^ b[i])
-    }
-  } else {
-    for (var j = 0; j < a.length; j++) {
-      res.push(a[j] ^ b[j])
-    }
-  }
-  return Buffer.from(res)
+  if (!Buffer.isBuffer(a)) {
+    throw new TypeError('first argument must be a Buffer')
+  }
+  if (!Buffer.isBuffer(b)) {
+    throw new TypeError('second argument must be a Buffer')
+  }
+  if (a.length !== b.length) {
+    throw new Error('Buffer lengths must match')
+  }
+  if (a.length === 0) {
+    throw new Error('Buffers cannot be empty')
+  }
+  return Buffer.from(a.map((_, i) => a[i] ^ b[i]))
+}
+
+function sha256(text) {
+  return crypto.createHash('sha256').update(text).digest()
 }
 
-function createHMAC(key, msg) {
+function hmacSha256(key, msg) {
   return crypto.createHmac('sha256', key).update(msg).digest()
 }
 
 function Hi(password, saltBytes, iterations) {
-  var ui1 = createHMAC(password, Buffer.concat([saltBytes, Buffer.from([0, 0, 0, 1])]))
+  var ui1 = hmacSha256(password, Buffer.concat([saltBytes, Buffer.from([0, 0, 0, 1])]))
   var ui = ui1
   for (var i = 0; i < iterations - 1; i++) {
-    ui1 = createHMAC(password, ui1)
+    ui1 = hmacSha256(password, ui1)
     ui = xorBuffers(ui, ui1)
   }
 

packages/pg/test/test-helper.js

@@ -111,16 +111,6 @@ assert.success = function (callback) {
   }
 }
 
-assert.throws = function (offender) {
-  try {
-    offender()
-  } catch (e) {
-    assert.ok(e instanceof Error, 'Expected ' + offender + ' to throw instances of Error')
-    return
-  }
-  assert.ok(false, 'Expected ' + offender + ' to throw exception')
-}
-
 assert.lengthIs = function (actual, expectedLength) {
   assert.equal(actual.length, expectedLength)
 }

packages/pg/test/unit/client/sasl-scram-tests.js

@@ -38,7 +38,7 @@ test('sasl/scram', function () {
     test('fails when last session message was not SASLInitialResponse', function () {
       assert.throws(
         function () {
-          sasl.continueSession({})
+          sasl.continueSession({}, '', '')
         },
         {
           message: 'SASL: Last message was not SASLInitialResponse',
@@ -53,6 +53,7 @@ test('sasl/scram', function () {
             {
               message: 'SASLInitialResponse',
             },
+            'bad-password',
             's=1,i=1'
           )
         },
@@ -69,6 +70,7 @@ test('sasl/scram', function () {
             {
               message: 'SASLInitialResponse',
             },
+            'bad-password',
             'r=1,i=1'
           )
         },
@@ -85,7 +87,8 @@ test('sasl/scram', function () {
             {
               message: 'SASLInitialResponse',
             },
-            'r=1,s=1'
+            'bad-password',
+            'r=1,s=abcd'
           )
         },
         {
@@ -102,7 +105,8 @@ test('sasl/scram', function () {
               message: 'SASLInitialResponse',
               clientNonce: '2',
             },
-            'r=1,s=1,i=1'
+            'bad-password',
+            'r=1,s=abcd,i=1'
           )
         },
         {
@@ -117,12 +121,12 @@ test('sasl/scram', function () {
         clientNonce: 'a',
       }
 
-      sasl.continueSession(session, 'password', 'r=ab,s=x,i=1')
+      sasl.continueSession(session, 'password', 'r=ab,s=abcd,i=1')
 
       assert.equal(session.message, 'SASLResponse')
-      assert.equal(session.serverSignature, 'TtywIrpWDJ0tCSXM2mjkyiaa8iGZsZG7HllQxr8fYAo=')
+      assert.equal(session.serverSignature, 'jwt97IHWFn7FEqHykPTxsoQrKGOMXJl/PJyJ1JXTBKc=')
 
-      assert.equal(session.response, 'c=biws,r=ab,p=KAEPBUTjjofB0IM5UWcZApK1dSzFE0o5vnbWjBbvFHA=')
+      assert.equal(session.response, 'c=biws,r=ab,p=mU8grLfTjDrJer9ITsdHk0igMRDejG10EJPFbIBL3D0=')
     })
   })
 
@@ -138,15 +142,32 @@ test('sasl/scram', function () {
       )
     })
 
+    test('fails when server signature is not valid base64', function () {
+      assert.throws(
+        function () {
+          sasl.finalizeSession(
+            {
+              message: 'SASLResponse',
+              serverSignature: 'abcd',
+            },
+            'v=x1' // Purposefully invalid base64
+          )
+        },
+        {
+          message: 'SASL: SCRAM-SERVER-FINAL-MESSAGE: server signature must be base64',
+        }
+      )
+    })
+
     test('fails when server signature does not match', function () {
       assert.throws(
         function () {
           sasl.finalizeSession(
             {
               message: 'SASLResponse',
-              serverSignature: '3',
+              serverSignature: 'abcd',
             },
-            'v=4'
+            'v=xyzq'
           )
         },
         {
@@ -159,9 +180,9 @@ test('sasl/scram', function () {
       sasl.finalizeSession(
         {
           message: 'SASLResponse',
-          serverSignature: '5',
+          serverSignature: 'abcd',
         },
-        'v=5'
+        'v=abcd'
       )
     })
   })