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

Critical Security Fix Required: You disclose the correct signature with each SignatureVerificationException... #61

Closed
ststeiger opened this issue Sep 29, 2016 · 11 comments

Comments

@ststeiger
Copy link

ststeiger commented Sep 29, 2016

I just tested the result after including this library in one of our projects.
Whatever it is that you are smoking - it must be good...
You are outputting the correct signature with each SignatureVerificationException...
If I were an attacker, I would silently say "thank you" when I saw this - and wonder what drug you're on...

private static void Verify(string decodedCrypto, string decodedSignature, string payloadJson)
{
    if (decodedCrypto != decodedSignature)
    {
        // My oh my - please don't donate the correct signature to a wannabe-attacker...
        // throw new SignatureVerificationException(string.Format("Invalid signature. Expected {0} got {1}", decodedCrypto, decodedSignature));
        throw new SignatureVerificationException(string.Format("Invalid signature. Expected {0} got {1}", "SEE_LOG", decodedSignature));
    }

signature

@ststeiger ststeiger changed the title Security Hole: You output the correct signature with each SignatureVerificationException... Critical Security Fix Required: You output the correct signature with each SignatureVerificationException... Sep 29, 2016
@ststeiger ststeiger changed the title Critical Security Fix Required: You output the correct signature with each SignatureVerificationException... Critical Security Fix Required: You disclose the correct signature with each SignatureVerificationException... Sep 29, 2016
@abatishchev
Copy link
Member

abatishchev commented Sep 29, 2016

  • Please watch your language, this is not a nightclub
  • Learn <customErrors>
  • This is open source, baby! Contribution is very welcome.

@abatishchev
Copy link
Member

This type of exceptions should be cough, swallows and logged accordingly. However, I agree this is not an ideal or nice default behavior.

@ststeiger
Copy link
Author

I actually am, just that my project is on
https://github.com/ststeiger/Jwt_Net20
I will fork the original project and issue a pull request once I'm finished.
If you want to take a look in the meantime - added RSA + started with ecdsa, and backported to .NET 2.0.
RSA key is still static.
Must still remove the extension methods, so there won't be any compiler warning.
But so far, it already works.
I am changing over to BouncyCastle because there's lots of NotImplemented in both mono, .NET 2.0 and .NET4.0 ecdsa.

On Learn :
I know it exists and why.
But we have errors enabled in production.
To be able to respond faster to problems.
While it's true that switching off error details is exactly for that - it shouldn't disclose such information by default.
After all, error-off should be to not disclose source-code + stack-traces (along with any possibly confidential info that might be in an error).
I don't want to know how many people copied this to production without realizing that.
You have just set it into public domain, you didn't limit liability, by the way - not a good idea either.

@abatishchev
Copy link
Member

abatishchev commented Sep 30, 2016

I agree it's not right, will research more once get some spare time. So far I think we need to save both values into Exception.Data dictionary.

I was thinking to fork this project too and call it Jwt2 but never did since we're using AAD and client certs at work.

Regarding authorship and licensing: I'm far from being the initial author, I've joined the project just 1 month ago as a open source community maintainer. So can't change the license, etc.

@rpetz
Copy link

rpetz commented Nov 29, 2016

has this been pulled in and resolved? seems like a pretty major security hole for a authentication library to throw back 'wrong token, heres the right one'...I know you can try catch and swallow it yourself or just hide it with custom errors off, but that is a pretty big loop hole for the second search result in NuGet for JWT with 284,000 downloads...

...this really should be patched up cause this can introduce a lot of security holes to a lot of people without them ever knowing it - it's not a complex fix and I would be happy to toss a pull request at it if it will get pushed to nuget with a quickness

@abatishchev
Copy link
Member

No, this hasn't been fixed (yet). Pull requests are very welcome!

@rpetz
Copy link

rpetz commented Nov 29, 2016

Pull request created - I think I'm just gonna pull the code into my current project directly instead of using NuGet but the package really should be updated in NuGet as well

@abatishchev
Copy link
Member

Closed in favor of the pr.

@maciejkoleda
Copy link

  • Please watch your language, this is not a nightclub

Nightclub is the place where your spree cant soul'd out to make some mess. Watch your language.

@abatishchev
Copy link
Member

What have we got here? Private Joker?

@ststeiger
Copy link
Author

ststeiger commented Oct 20, 2018

Bullshit! You didn't convince me!
Let me see your real war face!

Aaaaaaaaaaaaaaaaagh!

You didn't scare me!
Work on it!

OK, I will watch my language now ;)

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

No branches or pull requests

4 participants