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

Encoded token stored as KC_RESTART cookie uses weak algorithm- HS256 #13080

Closed
Shiva19908 opened this issue Jul 13, 2022 · 18 comments · Fixed by #26957
Closed

Encoded token stored as KC_RESTART cookie uses weak algorithm- HS256 #13080

Shiva19908 opened this issue Jul 13, 2022 · 18 comments · Fixed by #26957
Assignees
Labels
area/authentication Indicates an issue on Authentication area area/core kind/bug Categorizes a PR related to a bug kind/enhancement Categorizes a PR related to an enhancement kind/weakness Issues identified as a security hardening issue that we can improve into the code release/24.0.0 team/core-clients
Milestone

Comments

@Shiva19908
Copy link

Shiva19908 commented Jul 13, 2022

Describe the bug

This cookie is an internal cookie that is used by Keycloak. This cookie is used by Keycloak in the authentication flow to begin a new authentication flow if something happens and the flow needs to be restarted (https://github.com/keycloak/keycloak-community/pull/16/files/4c9b955990190344a3f1ed98c0a0a1dd62d1e8ec).

This cookie is classified as an internal cookie in Keycloak code. Here we can see the implementation for this cookie -> https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java

On line 184, it can be seen that this cookie's category is set as 'INTERNAL'.

Keycloak has a token manager and this class is used to specify which algorithm should be used for each cookie type (https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java#L192).

The issue we have is that because the KC_RESTART token has a category of 'INTERNAL', in the token manager class on line 192, it can be seen that it is hardcoded in Keycloak to always use HS256 algorithm for this type of token. HS256 is a weak algorithm.

Version

17.0.1

Expected behavior

Either the algorithm should be changed to a secure one or there should be an option to change the algorithm.

Actual behavior

The token stored as KC_RESTART cookie uses weak algorithm HS256

How to Reproduce?

Decode the JWT token stored as KC_RESTART cookie using a Burp Suite Extender extension such as JSON Web Tokens to identify that the algorithm in use 'alg' value is 'HS256'

Anything else?

No response

@Shiva19908
Copy link
Author

Hi @abstractj could you please comment on this issue ,for example, how severe is this issue? This is very important for us as one of our customers is waiting for the probable delivery date for the fix.
Kind regards,
Shiva

@abstractj
Copy link
Contributor

@Shiva19908 let me quote the analysis from @JoshuaMulliken

This is really not a security issue at all. HS256 is only considered insecure at low key sizes. 512 bits is a strong and recommended minimum key length. Luckily that is our default key length so I am not worried (64 bytes is 512 bits).

The only thing that I might recommend is that we remove the values 16 and 24 from the secret size options as they are both under 256 and the JWT spec states that the key size MUST be 256 bits or larger [1].

Thanks,

Josh Mulliken

[1] https://datatracker.ietf.org/doc/html/rfc7518#section-3.2

@Shiva19908 even though we acknowledge that we should be strengthening our security, there's nothing to worry about. But if you identify any ways to exploit HS256 in the way how it is implemented into our codebase, please reach us out in the keycloak-security mailing list.

@Shiva19908
Copy link
Author

Hi @abstractj ,
Thank you very much for the quick response! Good to know that there is nothing to worry about. Sorry, one more question, did I understand right that Keycloak would still consider doing the secret size changes as recommended by @JoshuaMulliken? If so, would there be a ticket to follow and when it might be delivered?
Kind regards,
Shiva

@abstractj
Copy link
Contributor

Hi @Shiva19908 for now we don't have any estimate for this improvement, but if this is something that you would be willing to contribute, please let us know.

@stianst stianst added area/authentication Indicates an issue on Authentication area area/core labels Sep 28, 2022
@mposolda
Copy link
Contributor

@abstractj Should the "triage" label be removed from this one?

@abstractj abstractj removed their assignment Nov 24, 2022
@abstractj abstractj added kind/enhancement Categorizes a PR related to an enhancement kind/weakness Issues identified as a security hardening issue that we can improve into the code and removed kind/bug Categorizes a PR related to a bug labels Nov 24, 2022
@abstractj
Copy link
Contributor

abstractj commented Nov 24, 2022

Thank you for bringing this to my attention @mposolda. I've updated the labels, but please do any changes that you would like.

@lukas0krupa
Copy link

Hi All,

I was trying for 2 days now to overcome this issue, specifying algorithm to RS256, but without any luck and still getting JWT signed with HS256 algorithm.

We got this issue flagged by penetration testers (medium severity), but it seems like it is possible to brute force HS256.

See:

Any chance you can help me (perhaps others) with workaround?

Thank you

@ericwout-overheid
Copy link

Hi,

I'm working on a project where we also have a pentest that notes that HS256 is too weak, unless we can prove that the key size is high. For us it's marked as HIGH severity. We have tried to find it in the code, but could not track it down. How can we verify which key size is used for the internal category?

Thanks!

@abstractj
Copy link
Contributor

@lukas0krupa as far as I'm aware, Keycloak does support JWT token signing with RS256 as mentioned in our documentation: https://www.keycloak.org/docs/latest/server_admin. If you're facing issues with it, please create a new issue providing more details about your configuration, logs, and steps to reproduce the issue for better understanding.

@ericwout-overheid Keycloak currently generates a 32-byte secret for the HS256 algorithm, based on security best practices. If you spot potential improvements or vulnerabilities, please create an enhancement request. For security vulnerabilities, follow the guidelines here: https://www.keycloak.org/security.

@keycloak/core please add your comments if I'm missing anything.

@ericwout-overheid
Copy link

@abstractj Thanks for your answer. We have been looking in the code/config to find where we can show this to our pentester but cannot find it. Can you give us a pointer?

@abstractj
Copy link
Contributor

@ericwout-overheid I'd suggest using https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/crypto/HS256SignatureProviderFactory.java as a starting point.

@abstractj abstractj added the kind/bug Categorizes a PR related to a bug label Jan 10, 2024
@rmartinc
Copy link
Contributor

rmartinc commented Feb 8, 2024

Hi!

The RestartLoginCookie is using the internal category which is always HS256. I suppose it was decided that way because it's only used by keycloak itself (hmac secret key is not shared). AFAIK the HS256 is considered secure as long as the HMac key is not weak (usually less than 48 bytes, the links shared in this issue seem to say exactly the same), the default key in keycloak is 64 bytes although you can create a new one with more bytes if necessary.

Nevertheless maybe we can add a new attribute to specify the default internal algorithm instead of hardcoded HS256. Similar to what we are doing for the Default Signature Algorithm for shared tokens.

@stianst
Copy link
Contributor

stianst commented Feb 8, 2024

This should not be configurable, but rather we should verify and consider updating what we use. The key we're using should be plenty strong, but we should maybe consider increasing it and also switching to HS512 perhaps?

@rmartinc
Copy link
Contributor

rmartinc commented Feb 8, 2024

Yep, we can do that too. Increasing the default size is easy (128 or 256 for example), but changing to HS512 requires to think what to do with migrated realms. Options that I see:

  1. Force them to HS512 with a migration task that adds a new HS512 key in all existing realms.
  2. Maintain HS256 for them, and the internal alg is HS256 or HS512 depending what key exists.

@stianst
Copy link
Contributor

stianst commented Feb 8, 2024

Don't think any migration is needed really; new HS512 keys are created automatically on-demand as needed already, and existing HS256 cookies would still be supported as the HS256 keys would still be there. This is all already supported with the key rotation support we have.

I think the only thing we need to consider is when/how to clean-up the HS256 keys, that may be something we just document though perhaps?

@rmartinc
Copy link
Contributor

rmartinc commented Feb 8, 2024

Ah! Yes, the fallback method, I never liked the name of the resulting key but it's automatically created. And yes, we can just document the change saying the old HS256 key can be removed some time after the upgrade (action tokens and so on so forth). New realms will automatically create a HS512 key instead of HS256 (the current hmac-generated name).

@rmartinc
Copy link
Contributor

rmartinc commented Feb 8, 2024

I'm taking the issue, I'll try to provide a fix in the next weeks.

@rmartinc rmartinc self-assigned this Feb 8, 2024
@rmartinc rmartinc added this to the 25.0.0 milestone Feb 8, 2024
@stianst
Copy link
Contributor

stianst commented Feb 8, 2024

Ah! Yes, the fallback method, I never liked the name of the resulting key but it's automatically created. And yes, we can just document the change saying the old HS256 key can be removed some time after the upgrade (action tokens and so on so forth). New realms will automatically create a HS512 key instead of HS256 (the current hmac-generated name).

Ah, true forgot about that. Yer, so may be we would just add a migration task then that adds a HS512 key, and also change the defaults for new realms.

rmartinc added a commit to rmartinc/keycloak that referenced this issue Feb 11, 2024
rmartinc added a commit to rmartinc/keycloak that referenced this issue Feb 13, 2024
rmartinc added a commit to rmartinc/keycloak that referenced this issue Feb 13, 2024
mposolda pushed a commit that referenced this issue Feb 15, 2024
Closes #13080

Signed-off-by: rmartinc <rmartinc@redhat.com>
@mposolda mposolda modified the milestones: 25.0.0, 24.0.0 Feb 15, 2024
ahus1 pushed a commit to ahus1/keycloak that referenced this issue Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/authentication Indicates an issue on Authentication area area/core kind/bug Categorizes a PR related to a bug kind/enhancement Categorizes a PR related to an enhancement kind/weakness Issues identified as a security hardening issue that we can improve into the code release/24.0.0 team/core-clients
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants