search

wso2 / product-is

Welcome to the WSO2 Identity Server source code! For info on working with the WSO2 Identity Server repository and contributing code, click the link below.
http://wso2.github.io/
Apache License 2.0
730 stars 713 forks source link

Signing or encrypting of tokens gives unexpected result #15134

Open Thumimku opened 1 year ago

Thumimku commented 1 year ago

Hi Team, Upon enabling encryption/decryption in setCookie/getCookie adaptive authentication methods, the decrypted value is missing special characters and end characters of the string value. We analyzed this and found that when decoding the string value using the base64 decode method [1], special characters may be dropped before processing further. Moreover, a possible cause for dropping end characters may be due to the usage of no padding [2] (default algorithm is AES/GCM/NoPadding). When noPadding is specified, the set of bytes forming valid complete blocks will be encrypted as expected and the rest may be discarded (16 bytes for AES). Therefore, any input less than a block long may be silently discarded. Due to the above behaviour, when decrypting the cookie in the getCookie method [3], the decrypted value is missing the special characters and end characters.

Ex: Actual value passed in adaptive script: "{\"usr\" : \"" + subject.username + "\", \"str\" : \"" + subject.userStoreDomain + "\"}"; Decrypted value: usrvildljungbjornstrVASTTRAF

As a result, the user is unable to authenticate as expected when encryption/decryption is enabled in the cookie and the following error is thrown,

Caused by: jdk.nashorn.internal.runtime.ParserException: <json>:1:0 Expected json literal but found u
usrvildljungbjornstrVASTTRAF

Expected output after decryption to authenticate successfully: {"usr":"vildljungbjorn","str":"VASTTRAFIK"}

Your thoughts are highly appreciated regarding this matter.

[1] https://github.com/wso2-extensions/identity-conditional-auth-functions/blob/master/components/org.wso2.carbon.identity.conditional.auth.functions.http/src/main/java/org/wso2/carbon/identity/conditional/auth/functions/http/CookieFunctionImpl.java#L79 [2] https://github.com/wso2/carbon-kernel/blob/4.9.x/core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/CryptoUtil.java#L160 [3] https://github.com/wso2-extensions/identity-conditional-auth-functions/blob/master/components/org.wso2.carbon.identity.conditional.auth.functions.http/src/main/java/org/wso2/carbon/identity/conditional/auth/functions/http/CookieFunctionImpl.java#L153

After some analysis,

For cookie value, it is recommended to use string instead of JSON object [1]. So, suggesting to encode the
azusr = "{\"usr\" : \"" + subject.username + "\", \"str\" : \"" + subject.userStoreDomain + "\"}"; and use the encoded value as the cookie. And when trying to retrieve the username and user store domain, obtaining the cookie first and then decode themat adaptive script and then parse the JSON would help.

Reference: [1] https://is.docs.wso2.com/en/latest/references/adaptive-authentication-js-api-reference/

Upon further analysis

It was found that the setCookie method expects an encoded value to encrypt [1]. Is there a particular reason for this? Likewise, in the getCookie method, it returns an encoded value [2]. Although the setCookie method expects an encoded value, there are no already available functions to perform encoding/decoding in Adaptive authentication scripts. If the above additional encoding and decoding steps are removed since the entire cookie is anyways encoded [3], there will be no unexpected results as mentioned above. Please note that we got multiple cases regarding this matter. It's better to provide this in the product rather than suggesting custom solutions.

What do you think?

[1] https://github.com/wso2-extensions/identity-conditional-auth-functions/blob/master/components/org.wso2.carbon.identity.conditional.auth.functions.http/src/main/java/org/wso2/carbon/identity/conditional/auth/functions/http/CookieFunctionImpl.java#L79 [2] https://github.com/wso2-extensions/identity-conditional-auth-functions/blob/master/components/org.wso2.carbon.identity.conditional.auth.functions.http/src/main/java/org/wso2/carbon/identity/conditional/auth/functions/http/CookieFunctionImpl.java#L153 [3] https://github.com/wso2-extensions/identity-conditional-auth-functions/blob/master/components/org.wso2.carbon.identity.conditional.auth.functions.http/src/main/java/org/wso2/carbon/identity/conditional/auth/functions/http/CookieFunctionImpl.java#L92

Thumimku commented 1 year ago

Proposed Fix

We will introduce a new variable called "encode" as a sub-parameter inside the map (which is 4th parameter)

            setCookie(context.response, "sampleValue", sampleUser, {"max-age" : 4000, "encrypt" : true, "sign" : false, "encode" : true});

Encode Value: true Encode the sampleValue and proceed Value: false Don't Encode the sampleValue and proceed

Important If sampleValue consists of any special character it's mandatory to use encode as true.

This fix should include the doc fix also.