Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of HMAC Based Token pattern #155

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion API.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ The following options are available when registering the plugin.
* `skip` - a function with the signature of `function (request, h) {}`, which when provided, is called for every request. If the provided function returns true, validation and generation of crumb is skipped. Defaults to `false`.
* `enforce` - defaults to true, using enforce with false will set the CSRF header cookie but won't execute the validation
* `logUnauthorized` - whether to add to the request log with tag 'crumb' and data 'validation failed' (defaults to false)

* `method` - The token generation method (and therefore how to validate), could take values `random` or `hmac`. Defaults to `random`, if set to `hmac` crumb will use the `Hmac Based Token pattern` [as described by OWASP here](https://github.com/OWASP/CheatSheetSeries/blob/083b4791c44ef28b105f66c82ffd83a86bf9fa16/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
* `method` - The token generation method (and therefore how to validate), could take values `random` or `hmac`. Defaults to `random`, if set to `hmac` crumb will use the `Hmac Based Token pattern` [as described by OWASP here](https://github.com/OWASP/CheatSheetSeries/blob/083b4791c44ef28b105f66c82ffd83a86bf9fa16/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md).
* `method` - The token generation and validation method, can be `'random'` or `'hmac'`. Defaults to `'random'`, if set to `'hmac'` crumb will use the `Hmac Based Token pattern` [as described by OWASP here](https://github.com/OWASP/CheatSheetSeries/blob/083b4791c44ef28b105f66c82ffd83a86bf9fa16/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md).

* `sessionKey` - Define which key of `auth.credentials` should be used during token creation if specified method is `hmac`
### Routes configuration

Additionally, some configuration can be passed on a per-route basis. Disable Crumb for a particular route by passing `false` instead of a configuration object.
Expand Down
56 changes: 56 additions & 0 deletions lib/hmac.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';

const Crypto = require('crypto');

const ALGO = 'sha256';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be configurable. sha256 is a reasonable default, but there's no good reason to not allow users to override it with their own choice

/**
* Returns base64-encoded ciphertext
* @param {string}userId The string to encrypt
* @param {string}key The secret
* @returns {string}token
*/
const encrypt = (userId, key) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: all instances of the variable userId should be sessionId since that more clearly indicates what the parameter is since it's not necessarily a user's id

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second nit: key variables should be renamed to secret, again to more clearly indicate what it's used for


const timestamp = Date.now().toString();
const digest = Crypto.createHmac(ALGO, key)
.update(userId, 'utf8')
.update(timestamp, 'utf8')
.digest('hex');

return `${digest}_${timestamp}`;
};

/**
* Validate a CSRF token generated with HMAC method. based on OWASP cheatsheet
* https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#hmac-based-token-pattern
* @param {string}enc
* @param {string}userId
* @param {string}key
*/
const validate = (enc, userId, key) => {

if (!enc || !userId || !key) {
// Validation fails is one of the params is null or undefined
return false;
}

const timestamp = Date.now().toString();
const [token_digest, token_timestamp] = enc.split('_'); //Split the encrypted string to retrieve the hmac digest and the timestamp

if (!token_digest || !token_timestamp) {
// Validation fails is one of the params is null or undefined
return false;
}

const digest = Crypto.createHmac(ALGO, key)
.update(userId, 'utf8')
.update(token_timestamp, 'utf8') // Timestamp used at token generation
.digest('hex');

return (digest === token_digest && token_timestamp <= timestamp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the comparison of token_timestamp and timestamp, i think it's more appropriate to convert the token_timestamp to a number after splitting the value out than it is to convert the timestamp to a string

};

module.exports = {
encrypt,
validate
};
47 changes: 40 additions & 7 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const Cryptiles = require('@hapi/cryptiles');
const Hoek = require('@hapi/hoek');
const Validate = require('@hapi/validate');

const Hmac = require('./hmac');

const internals = {
restfulValidatedMethods: ['post', 'put', 'patch', 'delete']
Expand All @@ -23,7 +24,16 @@ internals.schema = Validate.object().keys({
restful: Validate.boolean().optional(),
skip: Validate.func().optional(),
enforce: Validate.boolean().optional(),
logUnauthorized: Validate.boolean().optional()
logUnauthorized: Validate.boolean().optional(),
method: Validate.string().optional().valid('random', 'hmac'),
secret: Validate.string().when(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secret is here and required when using the hmac method, but needs to be added to the documentation changes

'method',
{ is: Validate.exist(), then: Validate.required(), otherwise: Validate.optional() }
),
sessionKey: Validate.string().when(
'method',
{ is: Validate.exist(), then: Validate.required(), otherwise: Validate.optional() }
)
});


Expand All @@ -39,7 +49,9 @@ internals.defaults = {
restful: false, // Set to true for custom header crumb validation. Disables payload/query validation
skip: false, // Set to a function which returns true when to skip crumb generation and validation,
enforce: true, // Set to true for setting the CSRF cookie while not performing validation
logUnauthorized: false // Set to true for crumb to write an event to the request log
logUnauthorized: false, // Set to true for crumb to write an event to the request log
method: 'random', // Define the token generation method (and therefore how to validate)
sessionKey: 'userId' // Define which key of auth.credentials should be used during token creation if method = hmac
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure about the name of this option, its default value, or how it functions.

the recommendation from OWASP is that this is a "session ID". nothing suggests that this is necessarily the user's ID, especially given that a single user could be signed in to multiple sessions and the crumb could be valid for only one of those.

my hunch (but this is very much worth discussing) is that this may be better expressed as a function that receives the request object and returns an identifier rather than requiring that every request use the same property from an authenticated user's credentials.

at the very least, i suspect 'userId' is unlikely to be the most common property used for this, 'id' seems more fitting as a default to me.

};


Expand Down Expand Up @@ -68,7 +80,8 @@ exports.plugin = {
const unauthorizedLogger = () => {

if (settings.logUnauthorized) {
request.log(['crumb', 'unauthorized'], 'validation failed');
const tags = ['crumb', 'unauthorized'];
request.log(tags, 'validation failed');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this change isn't really necessary. i find the original more readable myself

}
};

Expand Down Expand Up @@ -130,7 +143,6 @@ exports.plugin = {
}

// Validate crumb

const restful = request.route.settings.plugins._crumb ? request.route.settings.plugins._crumb.restful : settings.restful;
if (restful) {
if (!internals.restfulValidatedMethods.includes(request.method) || !request.route.settings.plugins._crumb) {
Expand All @@ -144,7 +156,13 @@ exports.plugin = {
throw Boom.forbidden();
}

if (header !== getCrumbValue()) {
if (settings.method !== 'hmac' && header !== getCrumbValue()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're setting a default of random to settings.method, why not make this condition if (settings.method === 'random' to be more explicit?

unauthorizedLogger();
throw Boom.forbidden();
}

if (settings.method === 'hmac' &&
!Hmac.validate(header, request.auth.credentials[settings.sessionKey], settings.secret)) {
unauthorizedLogger();
throw Boom.forbidden();
}
Expand All @@ -168,11 +186,17 @@ exports.plugin = {
throw Boom.forbidden();
}

if (content[request.route.settings.plugins._crumb.key] !== getCrumbValue()) {
if (settings.method !== 'hmac' && content[request.route.settings.plugins._crumb.key] !== getCrumbValue()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one can also be an explicit settings.method === 'random'

unauthorizedLogger();
throw Boom.forbidden();
}

if (settings.method === 'hmac' &&
!Hmac.validate(content[request.route.settings.plugins._crumb.key], request.auth.credentials[settings.sessionKey], settings.secret)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this line is super long, making it kind of hard to comprehend quickly. it might be best if we assigned the value of request.route.settings.plugins._crumb.key to a named variable and used that instead of the full reference.

) {
unauthorizedLogger();
throw Boom.forbidden();
}
// Remove crumb

delete request[request.route.settings.plugins._crumb.source][request.route.settings.plugins._crumb.key];
Expand Down Expand Up @@ -210,7 +234,16 @@ exports.plugin = {
const generate = function (request, h) {

if (!request.plugins.crumb) {
const crumb = Cryptiles.randomString(settings.size);
let crumb = null;

if (settings.method === 'random') {
crumb = Cryptiles.randomString(settings.size);
}

if (settings.method === 'hmac' && (request.auth.isAuthenticated)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra parens

Suggested change
if (settings.method === 'hmac' && (request.auth.isAuthenticated)) {
if (settings.method === 'hmac' && request.auth.isAuthenticated) {

crumb = Hmac.encrypt(request.auth.credentials[settings.sessionKey], settings.secret);
}

h.state(settings.key, crumb, settings.cookieOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned in another comment, it's possible at this point for crumb to be null. we need a test that shows what happens in this scenario

request.plugins.crumb = crumb;
}
Expand Down
66 changes: 66 additions & 0 deletions test/hmac.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
'use strict';

const Code = require('@hapi/code');
const Lab = require('@hapi/lab');
const Hmac = require('../lib/hmac');

const { describe, it } = exports.lab = Lab.script();
const { expect } = Code;

describe('Hmac', () => {

const mySecret = 'super secret!!! Do not share';
const userId = 'myUserId';

it('should export two methods', () => {

expect(Hmac).to.include('encrypt')
.and
.to.include('validate');

expect(Hmac.encrypt).to.be.a.function();
expect(Hmac.validate).to.be.a.function();
});

describe('encrypt', () => {

it('should return a digest and a timestamp concatenate with _', () => {

const result = Hmac.encrypt(userId, mySecret);
expect(result).to.be.a.string().and.to.include('_');
});
});

describe('validate', () => {

it('should return FALSE if the token is not in the right format', () => {

let encryptedStr = 'thisIsNotaGoodformat';
expect(Hmac.validate(encryptedStr, userId, mySecret)).to.be.a.false();

encryptedStr = '_thisIsNotaGoodformat';
expect(Hmac.validate(encryptedStr, userId, mySecret)).to.be.a.false();
});

it('should return FALSE if one of the parameters is null', () => {

const encryptedStr = 'thisIsNotaGoodformat';

expect(Hmac.validate(null, userId, mySecret)).to.be.a.false();
expect(Hmac.validate(encryptedStr, null, mySecret)).to.be.a.false();
expect(Hmac.validate(encryptedStr, userId, null)).to.be.a.false();
});

it('should return TRUE if the token is valid', () => {

const encryptedStr = Hmac.encrypt(userId, mySecret);
expect(Hmac.validate(encryptedStr, userId, mySecret)).to.be.a.true();
});

it('should return FALSE if digest do not match', () => {

const encryptedStr = `123456ABCD_${Date.now().toString()}`;
expect(Hmac.validate(encryptedStr, userId, mySecret)).to.be.a.false();
});
});
});
Loading