brave / vault

Brave personal data store vault.
https://brave.com
Mozilla Public License 2.0
19 stars 18 forks source link

need CSRF protection for actions authenticated by cookie? #49

Closed diracdeltas closed 8 years ago

diracdeltas commented 8 years ago

AFAICT, the vault doesn't do any CSRF checks for actions that require cookie-based auth (devops only?). Reuven used https://github.com/hapijs/crumb for the website.

mrose17 commented 8 years ago

again, i seek your counsel: the use of authentication with the vault is basically for dev/devops management purposes (at present, for updating advertisement geometry).

if we want to add CSRF checking, then what what change should we make?

cc: @retzion

diracdeltas commented 8 years ago

any action that changes server state should have CSRF validation, so that includes updating ad geometry. You will need to add the https://github.com/hapijs/crumb plugin and use it on any request that is changing state.

Ex: forms should have a hidden input field like <input type="hidden" name="crumb" value="{{crumb}}"/> and the server should reject the request if the crumb value doesn't match the expected crumb for that user's session.

I am not familiar with hapijs-crumb otherwise, maybe @retzion can advise on how to use it.

retzion commented 8 years ago

I learned enough hapi + crumb to get the website working with it finally. https://github.com/brave/brave-website/blob/staging/server.js Let me know if I can help.

mrose17 commented 8 years ago

@retzion - thanks. could you look at https://github.com/brave/vault/commit/8ce0e97bd8aeb4c0b584a6b6d8d64ab6a549275d and counsel me as to what else needs to be done (i'm all for checking CSRF, but the docs for that repo are a bit on the thin side...) many thanks!

retzion commented 8 years ago

Ha. The docs? I didn't find any documentation to speak of. ;) I think I would try the clearInvalid var like this..

var crumbOptions = { 
    cookieOptions: { 
        clearInvalid: true,
        isSecure: false
    } 
}

server.register({ register: require('crumb'), options: crumbOptions }, (err) => {
    ...
}

And make sure you are only trying https connections if you're set to isSecure:true Check your requests and responses for the crumb. Let me know if you have any error messages to look at. Let me know if I can help further.

retzion commented 8 years ago

I would also review my changes here.. https://github.com/brave/brave-website/blob/staging/public/assets/js/forms/mailchimp.js I'm not sure how you are making your requests but with jQuery.ajax it wasn't until I added the crumb in the Form AND added the following to my call that it finally worked (along with the clearInvalid param on server)..

   xhrFields: {
      withCredentials: true
   },
diracdeltas commented 8 years ago

also, please set isSecure to true on the cookie

On Tue, Jan 26, 2016 at 9:42 AM, Reuven notifications@github.com wrote:

I would also review my changes here..

https://github.com/brave/brave-website/blob/staging/public/assets/js/forms/mailchimp.js I'm not sure how you are making your requests but with jQuery.ajax it wasn't until I added the crumb in the Form AND added the following to my call that it finally worked (along with the clearInvalid param on server)..

xhrFields: { withCredentials: true },

— Reply to this email directly or view it on GitHub https://github.com/brave/vault/issues/49#issuecomment-175137215.