Skip to content
This repository was archived by the owner on May 16, 2021. It is now read-only.

Use secure_compare when checking CSRF token #98

Closed
wants to merge 2 commits into from

Conversation

jeltz
Copy link
Contributor

@jeltz jeltz commented May 25, 2015

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.

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
Copy link
Contributor Author

jeltz commented May 25, 2015

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
Copy link
Contributor

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
Copy link
Member

zzak commented Aug 13, 2015

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

@kytrinyx
Copy link
Contributor

Yeah, that might be the way to go.

@jeltz
Copy link
Contributor Author

jeltz commented Dec 15, 2015

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
Copy link
Member

zzak commented Jul 26, 2016

@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 d8068e8, thank you!!

@zzak zzak closed this Jul 26, 2016
zzak referenced this pull request in sinatra/sinatra Mar 7, 2018
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.
@ghost
Copy link

ghost commented Mar 7, 2018

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

@koffeinfrei
Copy link

@kseifriedredhat The fix was picked into 1.5.5 as well: 06f1b5d.

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

@bobvanderlinden
Copy link

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

@ghost
Copy link

ghost commented Mar 9, 2018

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
Copy link

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
Copy link

ghost commented Mar 9, 2018

I already updated it.

CVEProject/cvelist#342

@bobvanderlinden
Copy link

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
Copy link

ghost commented Mar 12, 2018

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
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants