Merge pull request #290 from chrisdlangton/sec-fix-payload-verification

Security fix
2023-07-10 10:24:10 +02:00 · 2023-07-10 10:24:10 +02:00 · b175ee27e4
--- 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;
 };
-
-
-
--- 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.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
+            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.warn(`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 || '';
--- 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

@ -423,7 +423,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);
    }
--- 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();
+    });
+
 });
--- 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', () => {
        });
    });
 });
-
--- 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,
@ -533,4 +533,3 @@ describe('Uri', () => {
        });
    });
 });
-