From adeb7e7e2a854edad0f29fef952221cbc809a238 Mon Sep 17 00:00:00 2001 From: chris Date: Sat, 10 Jun 2023 23:08:05 +1000 Subject: [PATCH 1/2] feat(crypto)!: Deprecate calculateMac replaced by calculateServerMac or generateRequestMac --- lib/client.js | 11 ++++------- lib/crypto.js | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-- lib/server.js | 20 +++++++++---------- test/index.js | 36 ++++++++++++++++++++++++++++++++++ test/server.js | 46 ++++++++++++++++++++++++++++++++++++++++++- test/uri.js | 3 +-- 6 files changed, 147 insertions(+), 22 deletions(-) diff --git a/lib/client.js b/lib/client.js index 7bd383a..2b374d0 100755 --- a/lib/client.js +++ b/lib/client.js @@ -102,7 +102,7 @@ exports.header = function (uri, method, options) { artifacts.hash = Crypto.calculatePayloadHash(options.payload, credentials.algorithm, options.contentType); } - const mac = Crypto.calculateMac('header', credentials, artifacts); + const mac = Crypto.generateRequestMac('header', credentials, artifacts); // Construct header @@ -187,7 +187,7 @@ exports.authenticate = function (res, credentials, artifacts, options) { artifacts.ext = serverAuthAttributes.ext; artifacts.hash = serverAuthAttributes.hash; - const mac = Crypto.calculateMac('response', credentials, artifacts); + const mac = Crypto.generateRequestMac('response', credentials, artifacts); if (mac !== serverAuthAttributes.mac) { throw new Boom.Boom('Bad response mac', { decorate: result }); } @@ -276,7 +276,7 @@ exports.getBewit = function (uri, options) { // Calculate signature const exp = Math.floor(now / 1000) + options.ttlSec; - const mac = Crypto.calculateMac('bewit', credentials, { + const mac = Crypto.generateRequestMac('bewit', credentials, { ts: exp, nonce: '', method: 'GET', @@ -367,11 +367,8 @@ exports.message = function (host, port, message, options) { ts: artifacts.ts, nonce: artifacts.nonce, hash: artifacts.hash, - mac: Crypto.calculateMac('message', credentials, artifacts) + mac: Crypto.generateRequestMac('message', credentials, artifacts) }; return result; }; - - - diff --git a/lib/crypto.js b/lib/crypto.js index 97ba4bb..e4d168f 100755 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -19,7 +19,7 @@ exports.headerVersion = '1'; // Prevent comparison of mac exports.algorithms = ['sha1', 'sha256']; -// Calculate the request MAC +// Generates the request MAC /* type: 'header', // 'header', 'bewit', 'response' @@ -41,7 +41,7 @@ exports.algorithms = ['sha1', 'sha256']; } */ -exports.calculateMac = function (type, credentials, options) { +exports.generateRequestMac = function (type, credentials, options) { const normalized = exports.generateNormalizedString(type, options); @@ -51,6 +51,55 @@ exports.calculateMac = function (type, credentials, options) { }; +// Calculate the request MAC for verification + +/* + type: 'header', // 'header', 'bewit', 'response' + credentials: { + key: 'aoijedoaijsdlaksjdl', + algorithm: 'sha256' // 'sha1', 'sha256' + }, + options: { + method: 'GET', + resource: '/resource?a=1&b=2', + host: 'example.com', + port: 8080, + ts: 1357718381034, + nonce: 'd3d345f', + hash: 'U4MKKSmiVxk37JCCrAVIjV/OhB3y+NdwoCr6RShbVkE=', + ext: 'app-specific-data', + app: 'hf48hd83qwkj', // Application id (Oz) + dlg: 'd8djwekds9cj' // Delegated by application id (Oz), requires options.app + } +*/ + +exports.calculateServerMac = function (type, credentials, options, payload, contentType) { + + if (options.hash) { + if (!payload) { + console.log(`Security Warning: calculateServerMac was called trusting the client payload hash which provides no integrity checking and is insecure`); + } + else { + // never trust client provided hash, always calculate server side + options.hash = exports.calculatePayloadHash(payload, credentials.algorithm, contentType); + } + } + + const normalized = exports.generateNormalizedString(type, options); + + const hmac = Crypto.createHmac(credentials.algorithm, credentials.key).update(normalized); + const digest = hmac.digest('base64'); + return digest; +}; + + +exports.calculateMac = function (type, credentials, options) { + + console.log(`Deprecation Warning: calculateMac() is replaced by either calculateServerMac() or generateRequestMac()`); + return exports.generateRequestMac(type, credentials, options); +}; + + exports.generateNormalizedString = function (type, options) { let resource = options.resource || ''; diff --git a/lib/server.js b/lib/server.js index 30a85ca..1f43a5a 100755 --- a/lib/server.js +++ b/lib/server.js @@ -153,13 +153,6 @@ exports.authenticate = async function (req, credentialsFunc, options) { throw new Boom.Boom('Unknown algorithm', { decorate: result }); } - // Calculate MAC - - const mac = Crypto.calculateMac('header', credentials, artifacts); - if (!Cryptiles.fixedTimeComparison(mac, attributes.mac)) { - throw Object.assign(Utils.unauthorized('Bad mac'), result); - } - // Check payload hash if (options.payload || @@ -175,6 +168,13 @@ exports.authenticate = async function (req, credentialsFunc, options) { } } + // Calculate MAC + + const mac = Crypto.calculateServerMac('header', credentials, artifacts, options.payload, request.contentType); + if (!Cryptiles.fixedTimeComparison(mac, attributes.mac)) { + throw Object.assign(Utils.unauthorized('Bad mac'), result); + } + // Check nonce if (options.nonceFunc) { @@ -288,7 +288,7 @@ exports.header = function (credentials, artifacts, options) { artifacts.hash = Crypto.calculatePayloadHash(options.payload, credentials.algorithm, options.contentType); } - const mac = Crypto.calculateMac('response', credentials, artifacts); + const mac = Crypto.calculateServerMac('response', credentials, artifacts, options.payload, options.contentType); // Construct header @@ -429,7 +429,7 @@ exports.authenticateBewit = async function (req, credentialsFunc, options) { // Calculate MAC - const mac = Crypto.calculateMac('bewit', credentials, { + const mac = Crypto.generateRequestMac('bewit', credentials, { ts: bewit.exp, nonce: '', method: 'GET', @@ -508,7 +508,7 @@ exports.authenticateMessage = async function (host, port, message, authorization // Calculate MAC - const mac = Crypto.calculateMac('message', credentials, artifacts); + const mac = Crypto.generateRequestMac('message', credentials, artifacts); if (!Cryptiles.fixedTimeComparison(mac, authorization.mac)) { throw Object.assign(Utils.unauthorized('Bad mac'), result); } diff --git a/test/index.js b/test/index.js index 848f536..e90edbe 100755 --- a/test/index.js +++ b/test/index.js @@ -307,4 +307,40 @@ describe('Hawk', () => { const err = await expect(Hawk.server.authenticate(req, credentialsFunc)).to.reject(); expect(err.credentials).to.exist(); }); + + it('generates a header then fails to parse it (payload tampering)', async () => { + + const req = { + method: 'POST', + url: '/resource/4?filter=a', + headers: { + host: 'example.com:8080', + 'content-type': 'text/plain;x=y' + } + }; + + const payload = 'some not so random text'; + + const credentials1 = credentialsFunc('123456'); + + const reqHeader = Hawk.client.header('http://example.com:8080/resource/4?filter=a', req.method, { credentials: credentials1, ext: 'some-app-data', payload, contentType: req.headers['content-type'] }); + req.headers.authorization = reqHeader.header; + + const { credentials: credentials2, artifacts } = await Hawk.server.authenticate(req, credentialsFunc); + expect(credentials2.user).to.equal('steve'); + expect(artifacts.ext).to.equal('some-app-data'); + expect(() => Hawk.server.authenticatePayload('tampered text', credentials2, artifacts, req.headers['content-type'])).to.throw('Bad payload hash'); + + const res = { + headers: { + 'content-type': 'text/plain' + } + }; + + res.headers['server-authorization'] = Hawk.server.header(credentials2, artifacts, { payload: 'some reply', contentType: 'text/plain', ext: 'response-specific' }); + expect(res.headers['server-authorization']).to.exist(); + + expect(() => Hawk.client.authenticate(res, credentials2, artifacts, { payload: 'some reply' })).to.not.throw(); + }); + }); diff --git a/test/server.js b/test/server.js index 342ca7d..44ffbc7 100755 --- a/test/server.js +++ b/test/server.js @@ -1122,6 +1122,51 @@ describe('Server', () => { creds.algorithm = 'blah'; expect(() => Hawk.client.message('example.com', 8080, 'some message', { credentials: creds })).to.throw('Unknown algorithm'); }); + + it('coverage for calculateMac arguments to calculatePayloadHash', async () => { + + const credentials = credentialsFunc('123456'); + const payload = 'some not so random text'; + const req = { + method: 'POST', + url: '/resource/4?filter=a', + host: 'example.com', + port: 8080, + headers: { + host: 'example.com:8080', + 'content-type': 'text/plain' + } + }; + + const exp = Math.floor(Hawk.utils.now() / 1000) + 60; + const ext = 'some-app-data'; + const nonce = '1AwuJD'; + const hash = Hawk.crypto.calculatePayloadHash(payload, 'sha256', req.headers['content-type']); + const opts = { + ts: exp, + nonce, + method: req.method, + resource: req.url, + host: req.host, + port: req.port, + hash, + ext + }; + const mac = Hawk.crypto.calculateServerMac('header', credentials, opts, payload, req.headers['content-type']); + const header = 'Hawk id="' + credentials.id + + '", ts="' + exp + + '", nonce="' + nonce + + '", hash="' + hash + + '", ext="' + ext + + '", mac="' + mac + '"'; + + req.headers.authorization = header; + // missing contentType + Hawk.crypto.calculateServerMac('header', credentials, opts, payload); + Hawk.crypto.calculateMac('header', credentials, opts); + await expect(Hawk.server.authenticate(req, credentialsFunc)).to.not.reject(); + }); + }); describe('authenticatePayloadHash()', () => { @@ -1133,4 +1178,3 @@ describe('Server', () => { }); }); }); - diff --git a/test/uri.js b/test/uri.js index cdd5b07..f298224 100755 --- a/test/uri.js +++ b/test/uri.js @@ -149,7 +149,7 @@ describe('Uri', () => { const exp = Math.floor(Hawk.utils.now() / 1000) + 60; const ext = 'some-app-data'; - const mac = Hawk.crypto.calculateMac('bewit', credentials, { + const mac = Hawk.crypto.generateRequestMac('bewit', credentials, { ts: exp, nonce: '', method: req.method, @@ -515,4 +515,3 @@ describe('Uri', () => { }); }); }); - From 8a344af0b4858e752d99bc77d050c019083d0cc3 Mon Sep 17 00:00:00 2001 From: Yarik Date: Fri, 7 Jul 2023 13:37:35 +0200 Subject: [PATCH 2/2] Apply suggestions from code review --- lib/crypto.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/crypto.js b/lib/crypto.js index e4d168f..0da1833 100755 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -77,7 +77,7 @@ exports.calculateServerMac = function (type, credentials, options, payload, cont if (options.hash) { if (!payload) { - console.log(`Security Warning: calculateServerMac was called trusting the client payload hash which provides no integrity checking and is insecure`); + console.warn(`Security Warning: calculateServerMac was called trusting the client payload hash which provides no integrity checking and is insecure`); } else { // never trust client provided hash, always calculate server side @@ -95,7 +95,7 @@ exports.calculateServerMac = function (type, credentials, options, payload, cont exports.calculateMac = function (type, credentials, options) { - console.log(`Deprecation Warning: calculateMac() is replaced by either calculateServerMac() or generateRequestMac()`); + console.warn(`Deprecation Warning: calculateMac() is replaced by either calculateServerMac() or generateRequestMac()`); return exports.generateRequestMac(type, credentials, options); };