Skip to content

Commit

Permalink
Use brcrypt.compare for password validation instead of compareSync (#…
Browse files Browse the repository at this point in the history
…7612)

Signed-off-by: Vinit Sarvade <vinit.sarvade.08@gmail.com>
  • Loading branch information
VinitSarvade committed Sep 1, 2020
1 parent 77be3ea commit dcd5254
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports = async (ctx, next) => {
`${strapi.config.server.url}${strapi.plugins.documentation.config['x-strapi-config'].path}/login${querystring}`
);
}
const isValid = strapi.plugins['users-permissions'].services.user.validatePassword(
const isValid = await strapi.plugins['users-permissions'].services.user.validatePassword(
ctx.session.documentation,
config.password
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ module.exports = {
})
.get();

const isValid = strapi.plugins['users-permissions'].services.user.validatePassword(
const isValid = await strapi.plugins['users-permissions'].services.user.validatePassword(
password,
storedPassword
);
Expand Down
7 changes: 3 additions & 4 deletions packages/strapi-plugin-users-permissions/controllers/Auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,9 @@ module.exports = {
);
}

const validPassword = strapi.plugins['users-permissions'].services.user.validatePassword(
params.password,
user.password
);
const validPassword = await strapi.plugins[
'users-permissions'
].services.user.validatePassword(params.password, user.password);

if (!validPassword) {
return ctx.badRequest(
Expand Down
2 changes: 1 addition & 1 deletion packages/strapi-plugin-users-permissions/services/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,6 @@ module.exports = {
},

validatePassword(password, hash) {
return bcrypt.compareSync(password, hash);
return bcrypt.compare(password, hash);
},
};

3 comments on commit dcd5254

@mattf96s
Copy link

Choose a reason for hiding this comment

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

Hi @lauriejim and @alexandrebodin

Forgetting the await in the const isValid = strapi.plugins['users-permissions'].services.user.validatePassword is a massive security issue. It means the code doesn't wait for a return and the passwords aren't compared. So any password is valid.

Even though this is patched, I think you should declare it for anyone on a version where this is a problem.

Thanks
Matt

@alexandrebodin
Copy link
Member

Choose a reason for hiding this comment

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

Hi @lauriejim and @alexandrebodin

Forgetting the await in the const isValid = strapi.plugins['users-permissions'].services.user.validatePassword is a massive security issue. It means the code doesn't wait for a return and the passwords aren't compared. So any password is valid.

Even though this is patched, I think you should declare it for anyone on a version where this is a problem.

Thanks
Matt

The code was using compareSync so it didn't need an await. there was no security issues. we only moved from sync to async

@davidemorra
Copy link

Choose a reason for hiding this comment

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

Well not..

It's a security issue if in the code anyone use this method, if not present this change must be inserted in a migration guide.

Please sign in to comment.