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

Validate size of signature against algorithm considering JWE #2008

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

brentschmaltz
Copy link
Member

SymmetricSignatures are now ensuring the size of the signature matches the algorithm.
Authenticated Encryption needed special processing as only the first half of the signature is considered for Aes128CbcHmacSha256, Aes128CbcHmacSha384 and Aes128CbcHmacSha512.

@TimHannMSFT
Copy link
Contributor

Seems like this also addresses #1743, is that correct?

@brentschmaltz
Copy link
Member Author

@TimHannMSFT had not considered #1743

@TimHannMSFT
Copy link
Contributor

My mistake, I think I got the key size check and the signature size check confused.


In reply to: 1407145465

public class TamperedTokenTests
{
[Theory, MemberData(nameof(JwtSignatureTruncationTheoryData))]
public async Task JwtSignatureTruncation(ValidateTokenTheoryData theoryData)
Copy link
Contributor

Choose a reason for hiding this comment

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

JwtSignatureTruncation

Looks like we're doing some similar testing in SignatureProviderTests::SignatureTrunctation. What's the difference? Looks like that also did symmetric hmac.

Can you help me understand why that didn't catch the issue? Should that test case be updated in any way for protection on the RSA/ECDSA cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimHannMSFT internally RSA/ECDSA require the signature bytes to be the correct size, so any truncation will be caught by the .net runtimes.

Signature provider tests missed this because they were being told an incorrect expected size from the JsonWebTokenHandler.

Copy link
Contributor

@TimHannMSFT TimHannMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

7409355636

This comment was marked as spam.

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.

3 participants