Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Commit

Permalink
Merge pull request #290 from chrisdlangton/sec-fix-payload-verification
Browse files Browse the repository at this point in the history 
Security fix
  • Loading branch information
@lotas
lotas committed Jul 10, 2023
2 parents fe746ef + 8a344af commit b175ee2
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 22 deletions.
11 changes: 4 additions & 7 deletions lib/client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 });
}
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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;
};



53 changes: 51 additions & 2 deletions lib/crypto.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -41,7 +41,49 @@ exports.algorithms = ['sha1', 'sha256'];
}
*/

exports.calculateMac = function (type, credentials, options) {
exports.generateRequestMac = function (type, credentials, options) {

const normalized = exports.generateNormalizedString(type, options);

const hmac = Crypto.createHmac(credentials.algorithm, credentials.key).update(normalized);
const digest = hmac.digest('base64');
return digest;
};


// 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);

Expand All @@ -51,6 +93,13 @@ exports.calculateMac = function (type, credentials, options) {
};


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 || '';
Expand Down
20 changes: 10 additions & 10 deletions lib/server.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Expand All @@ -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) {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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);
}
Expand Down
36 changes: 36 additions & 0 deletions test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

});
46 changes: 45 additions & 1 deletion test/server.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()', () => {
Expand All @@ -1133,4 +1178,3 @@ describe('Server', () => {
});
});
});

3 changes: 1 addition & 2 deletions test/uri.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -533,4 +533,3 @@ describe('Uri', () => {
});
});
});

0 comments on commit b175ee2

Please sign in to comment.