HTML Entity Encoding of received cookies can lead to invalid state errors

lochiiconnectivity commented 2 months ago

Checklist

[X] This can be reproduced in the quickstart sample application.
[X] I have reviewed the README.md and have not found a solution.
[X] I have reviewed the EXAMPLES.md and have not found a solution.
[X] I have searched previous issues and have not found a solution.
[X] I have searched the Auth0 Community and have not found a solution.
[X] I agree to adhere to the Auth0 General Contribution Guidelines.
[X] I agree to uphold the Auth0 Code of Conduct.

SDK Version

8.7

PHP Version

PHP 8.2

Description

HTML Entity Encoding of received cookies can lead to invalid state errors - some security frameworks will perform HTML Entity Encoding of the cookie data prior to being seen by the CookieStore, this can lead to failure to recover the values such as the state and 'invalid state' errors at token exchange time.

How can we reproduce this issue?

Deploy this SDK on a system which includes HTML Entity Encoding of received cookies, such as under various security frameworks.

shadowhand commented 2 months ago

Let's not be cryptic, what frameworks or packages do this?

lochiiconnectivity commented 2 months ago

No intention to be cryptic here this was observed in the wild in a third party platform which was HTML encoding all input including cookies, (the author of which has stated that this is a security framework, I do not know which but clearly it was configured to do this) , it was not obvious what led to the inability to retrieve the state from the cookie store - when debugging we saw that none of the keys of the cookie were valid JSON as they contained " as opposed to "

shadowhand commented 2 months ago

@lochiiconnectivity again, please name the package or platform. This is NOT a standard security practice and is extremely misguided. Discovering this would cause me to immediately drop said package or platform.

No one should be blindly applying htmlentities() on input, because the correct thing to do is only use htmlentities() on output. Auth0 should absolutely reject this patch because blindly calling html_entity_decode() on input which may not be encoded is also a potential security concern.

lochiiconnectivity commented 2 months ago

_ _Auth0 should absolutely reject this patch because blindly calling html_entity_decode() on input which may not be encoded is also a potential security concern__

You could also argue that we don’t expect URLEncoding or newlines either…

Worst case scenario , removing HEE which are introduced inappropriately and not expected could just lead to JSON corruption , this is happening now and we are trying to avoid it, even in the extremely unlikely situation that somebody corrupts the JSON in a targeted way, the worst case scenario is that the wrong data and/or IV is sent for decryption which will cause it to fail , given that the openSSL routines are called safely , that’s about the extent of the damage that could happen here.

Discovering this would cause me to immediately drop said package or platform

I’m glad you feel so idealistic about this, however we observed this in the wild and it led to failure of the SDK to operate, it’s something we have no control over and naming and shaming the platform won’t help. It doesn’t seem to be a huge concession on your part to acknowledge that this happens.

However given how many unhelpful “invalid state” answers exist out on community forums which don’t give users any ability to help themselves with it , I would have hoped you would have been more accommodating of this.

If you don’t believe that this patch adds value to the SDK or is safe/appropriate , and can’t add any further constructive comments on it , then feel free to close the M/R - I will instead raise an enterprise support ticket and ask the product team to review this.

shadowhand commented 2 months ago

You could also argue that we don’t expect URLEncoding [sic]

That would be incorrect, as URL encoding of data transmitted by a URL (such as state, etc) should be encoded per HTTP specifications. When people have trouble with state, in my experience it is often because they are double encoding and corrupting the state.

however we observed this in the wild and it led to failure of the SDK to operate

That is correct, because the SDK should not be trying to account for backwards security practices.

naming and shaming the platform won’t help

Hiding the identity simply means more people will not be aware of the problems of this platform. Honesty is always the best policy when it comes to security issues.

It doesn’t seem to be a huge concession on your part to acknowledge that this happens.

I am not denying that it happens. But the solution is not Auth0 accommodating a broken platform.

lochiiconnectivity commented 2 months ago

your opinions noted.

evansims commented 2 months ago

👋 Thanks for raising this @lochiiconnectivity, and for sharing your insight on this as well @shadowhand.

I sympathize with your situation on this @lochiiconnectivity, but this would simply be too destructive and potentially dangerous a change to apply broadly to the SDK. As @shadowhand noted, this opens up the potential for security concerns. At a minimum, it would be a significant and most likely breaking change that would invalidate sessions for users of all applications that implement the SDK.

I'm afraid this isn't a road we can take for the SDK. However, you could create your own storage class based on CookieStore that implements this, and simply have it live inside your app. Just configure the SDK at initialization time to use your storage class instead of the internal one.

I'll close this for now as it is not actionable for us, but feel free to keep the discussion going, if you have further questions.

lochiiconnectivity commented 2 months ago

However, you could create your own storage class based on CookieStore that implements this

Seems a lot of effort to go to , to maintain essentially a fork of the storage class for such simple change

We could instead allow the user to initialise the CookieStore with an optional encoding and decoding (user supplied) functions , in our case we would of course only use the decoding function, but other users could for example add encoding layers of their own

Since this wouldn’t be the default , you should have no security or session invalidation issues to worry about here , users would consciously choose to supply and maintain these functions.

evansims commented 2 months ago

I certainly understand where you're coming from. However, I feel this is really more appropriate for inclusion in the application layer, rather than the SDK. I can assure you the storage API we expose is stable, well-supported, and intended for precisely these sorts of cases.

lochiiconnectivity commented 2 months ago

At the application layer we only have two choices

One is to ingest , decode and manipulate the cookie data and then write that back to the shared environment , we have to do this each time before each method call.

The other is, as you said, fork the storage backend, and maintain this fork (including backporting any security fixes to it).

Neither is a great situation, we are talking about logic buried within the storage layer that isn’t easily accessible to the end user.

shadowhand commented 2 months ago

One is to ingest , decode and manipulate the cookie data and then write that back to the shared environment

Because of the broken nature of the provider/package (which I really think needs to be named for the benefit of the community, so that others can avoid it) this is probably the solution you will need to take. The first step to a solution will be accepting that said provider/package is doing things wrong.

auth0 / auth0-PHP