search

sinatra / rack-protection

NOTE: This project has been merged upstream to sinatra/sinatra
https://github.com/sinatra/sinatra/tree/master/rack-protection
818 stars 58 forks source link

Use secure_compare when checking CSRF token #98

Closed jeltz closed 8 years ago

jeltz commented 9 years ago

Since string comparisions may return early we want to use a constant time comparsion function to protect the CSRF token against timing attacks. Rack::Utils provides a such function.

jeltz commented 9 years ago

Rack::Utils.secure_compare was added in Rack 1.6.0, should I add an own implementation of secure_compare for old Rack versions, or is it ok to depend on 1.6.0?

kytrinyx commented 9 years ago

Sorry, I'm only just catching up on this issue. My guess is that we have to stay backwards-compatible with older versions of rack.

@zzak Thoughts?

zzak commented 9 years ago

@kytrinyx Good question, maybe we should provide our own shim for this method (if possible).

kytrinyx commented 9 years ago

Yeah, that might be the way to go.

jeltz commented 8 years ago

I have updated the pull request with the method copied from Rack::Utils so it will work with older versions of Rack too. Does this solution seem ok?

zzak commented 8 years ago

@jeltz The next release of rack-protection will only target Rack 2.0+ so we don't need to worry about the shim.

I've merged d8068e872b0f19ef9de25265552cb1b835270901, thank you!!

ghost commented 6 years ago

I have assigned CVE-2018-1000119 for this issue.

koffeinfrei commented 6 years ago

@kseifriedredhat The fix was picked into 1.5.5 as well: https://github.com/sinatra/rack-protection/commit/06f1b5d1bb00d81ebbad25414fb74f5bb9397c2f.

https://nvd.nist.gov/vuln/detail/CVE-2018-1000119 doesn't reflect that.

bobvanderlinden commented 6 years ago

Who should be notified to update nvd.nist.gov/vuln/detail/CVE-2018-1000119 ?

ghost commented 6 years ago

NVD pulls from MITRE's CVE Database (e.g. https://cve.mitre.org / https://github.com/cveproject/cvelist/) so it's up to NVD to notice and process this. You are free to contact the NVD and ask them to hurry up.

bobvanderlinden commented 6 years ago

Ah, then I guess this needs fixing: https://github.com/CVEProject/cvelist/blob/master/2018/1000xxx/CVE-2018-1000119.json I looked around a bit, but I don't exactly know yet how the version_data section works yet. Will look into this later.

ghost commented 6 years ago

I already updated it.

https://github.com/CVEProject/cvelist/pull/342

bobvanderlinden commented 6 years ago

Thanks! I guess the change hasn't propagated to GitHub yet as it is still notifying me about this vulnerability. I'll just wait for that to happen.

ghost commented 6 years ago

CVE processes the data, NVD then pulls and processes the data (this is opaque to me) and then GitHUB pulls and processes the data (this is opaque to me) so it can definitely take a while.

bobvanderlinden commented 6 years ago

Ah, that certainly explains the delay. Thanks for the info!