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

Resolve critical vulnerability allowing arbitrary tokens to pass as matching #60

Merged
merged 2 commits into from Aug 30, 2020

Conversation

aeneasr
Copy link
Contributor

@aeneasr aeneasr commented Aug 17, 2020

Before applying the patch to token.go, VerifyToken would incorrectly allow the following pairs to appear as equal:

    token_test.go:82: VerifyToken returned a false positive for: [foo bar]
    token_test.go:82: VerifyToken returned a false positive for: [foo ]
    token_test.go:82: VerifyToken returned a false positive for: [ bar]
    token_test.go:82: VerifyToken returned a false positive for: [ ]

This opens up attack vectors when developers rely on VerifyToken in cases where the attacker could trick the first parameter (realToken) into being an arbitrary value. Given that VerifyToken does not rely on r *http.Request to extract the token, this could happen when misusing the API or when other exploits are found to the way nosurf.Token(r) works.

This patch handles empty values and base64 decoding errors as verification failures and allows the failing test cases to pass.

@aeneasr
Copy link
Contributor Author

aeneasr commented Aug 17, 2020

@justinas I strongly recommend publishing a CVE for this so that people using - for example - nancy have a chance to detect and patch this.

@justinas
Copy link
Owner

Looks bad. And obvious in hindsight 😅 I scrolled through what usages I could find on GitHub an did not notice any obvious misuses as in this PoC.

Let me literally sleep on it (getting late here) and perhaps proceed with the CVE and this PR tomorrow.

@aeneasr
Copy link
Contributor Author

aeneasr commented Aug 17, 2020

Awesome, thank you for the quick reply! As far as I can tell, unless you're not using nosurf.Token(r) or overriding the HTTP context it should be ok. Given how ServeHTTP() sets a valid base64 token I don't think this can be abused if used within the examples given in the docs. I think the most critical case is where someone would pass an empty string as the real token due to a code bug:

realToken := nosurf.Token(r)
if whatever {
  realToken = ""
}

nosurf.VerifyToken(realToken, ..)

I found this because we're mocking nosurf.Token(r) in our tests to avoid having to use cookies everywhere.

token.go Outdated
if len(s) == 2*tokenLength {
s = unmaskToken(s)
}
return subtle.ConstantTimeCompare(r, s) == 1
return subtle.ConstantTimeCompare(r, s) == 1 && len(r) > 0 && len(s) > 0
Copy link
Owner

Choose a reason for hiding this comment

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

Would a more restrictive check make sense, i.e. len(r) == tokenLength && len(s) == tokenLength? I think we can also move the length checks before slice comparison. ConstantTimeCompare is already non-constant if slices have differing lengths.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, it would probably be best to just delegate to verifyToken here after unmasking realToken (verifyToken expects one masked, the other unmasked 🍝 ). It already has such checks and there are less opportunities for footguns having one implementation rather than two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would a more restrictive check make sense, i.e. len(r) == tokenLength && len(s) == tokenLength? I think we can also move the length checks before slice comparison. ConstantTimeCompare is already non-constant if slices have differing lengths.

Will do! I'm busy with workshops until Friday so I won't be able to fix this before but am happy to do so on Friday :)

Also, it would probably be best to just delegate to verifyToken here after unmasking realToken (verifyToken expects one masked, the other unmasked 🍝 ). It already has such checks and there are less opportunities for footguns having one implementation rather than two.

I'm not a 100% sure if I caught that but I have to say that I also don't 100% understand how masking/unmasking works and what they do. If you point me to the right areas I'd be happy to give it my best try but if you already have a concrete idea on how to do it feel free to go ahead :)

Enforced a single point of token equality check for all functions. Added a test case to demonstrate why the verifyToken can not just be used.
@aeneasr
Copy link
Contributor Author

aeneasr commented Aug 27, 2020

Thanks to @zepatrik your comments have been addressed :)

@aeneasr aeneasr requested a review from justinas August 28, 2020 08:55
@justinas
Copy link
Owner

Looks good. I will see into getting a CVE assigned.

@justinas justinas merged commit 4d86df7 into justinas:master Aug 30, 2020
1 check passed
@aeneasr
Copy link
Contributor Author

aeneasr commented Aug 30, 2020

Thank you :)

@justinas
Copy link
Owner

v1.1.1 has been tagged.

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

Successfully merging this pull request may close these issues.

None yet

3 participants