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

#483 Jwt Cookie set with SameSite=Strict #486

Merged
merged 2 commits into from
Apr 2, 2021

Conversation

symphony-hong
Copy link
Contributor

@symphony-hong symphony-hong commented Apr 2, 2021

Ticket

Closes #483

Description

The request to /bdk/v1/app/auth appears to be vulnerable
to cross-site request forgery (CSRF) attacks against authenticated users.

The request can be issued cross-domain.
The BDK relies solely on HTTP cookies to identify the user that issued the request.
The request performs some privileged action within the application, which returns a token.
The attacker can determine all the parameters required to construct a request that
performs the action. If the request contains any values that the attacker cannot
determine or predict, then it is not vulnerable.

Setting the SameSite=Strict will prevent sending the cookie when the request is done from another domain.

Dependencies

List the other pull requests that should be merged before/along this one.

Checklist

  • Referenced a ticket in the PR title and in the corresponding section
  • Filled properly the description and dependencies, if any
  • Unit tests updated or added
  • Javadoc added or updated
  • Updated the documentation in docs folder

@symphony-hong symphony-hong added the Mend: dependency security vulnerability Security vulnerability detected by WhiteSource label Apr 2, 2021
@symphony-hong symphony-hong requested a review from thibauult April 2, 2021 08:08
@symphony-hong symphony-hong requested a review from a team as a code owner April 2, 2021 08:08
The request to /bdk/v1/app/auth appears to be vulnerable
to cross-site request forgery (CSRF) attacks against authenticated users.

The request can be issued cross-domain.
The BDK relies solely on HTTP cookies to identify the user that issued the request.
The request performs some privileged action within the application, which returns a token.
The attacker can determine all the parameters required to construct a request that
performs the action. If the request contains any values that the attacker cannot
determine or predict, then it is not vulnerable.

Setting the SameSite=Strict will prevent sending the cookie when the request is done from another domain.
Copy link
Member

@thibauult thibauult left a comment

Choose a reason for hiding this comment

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

LGTM !

@symphony-youri
Copy link
Contributor

Isn't it going to break https://perzoinc.atlassian.net/browse/APP-3271 ?

@symphony-hong
Copy link
Contributor Author

IMHO, what we did in the APP-3271 is not the right way to perform the cross-site call in security sense.
Maybe, we can change the SameSite default to Strict and make it configurable for someone who want to do it without caring about the security issue?

@thibauult thibauult changed the title EIS-2766: Jwt Cookie set with SameSite=Strict #483 Jwt Cookie set with SameSite=Strict Apr 2, 2021
Copy link
Contributor

@symphony-elias symphony-elias left a comment

Choose a reason for hiding this comment

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

LGTM

@symphony-youri
Copy link
Contributor

Does someone remember why we set the userJwt cookie in the first place by the way?

Copy link
Member

@thibauult thibauult left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution :)

@thibauult
Copy link
Member

Does someone remember why we set the userJwt cookie in the first place by the way?

For "backward compatibility" I'd say. User session could also be stored in local storage though, however cookies are stored automatically and therefore doesn't require any additional code on client side.

@symphony-hong symphony-hong merged commit c513ae1 into finos:main Apr 2, 2021
@symphony-hong symphony-hong deleted the EIS-2766 branch April 2, 2021 14:00
@thibauult
Copy link
Member

@symphony-hong, sorry I forgot to tell you, but could you backport this change to 2.1-rc branch?

symphony-hong added a commit to symphony-hong/symphony-bdk-java that referenced this pull request Apr 5, 2021
* EIS-2766: Jwt Cookie set with SameSite=Strict

The request to /bdk/v1/app/auth appears to be vulnerable
to cross-site request forgery (CSRF) attacks against authenticated users.

The request can be issued cross-domain.
The BDK relies solely on HTTP cookies to identify the user that issued the request.
The request performs some privileged action within the application, which returns a token.
The attacker can determine all the parameters required to construct a request that
performs the action. If the request contains any values that the attacker cannot
determine or predict, then it is not vulnerable.

Setting the SameSite=Strict will prevent sending the cookie when the request is done from another domain.

* Make sameSite attribute configurable with default value = Strict
symphony-hong added a commit that referenced this pull request Apr 6, 2021
* EIS-2766: Jwt Cookie set with SameSite=Strict

The request to /bdk/v1/app/auth appears to be vulnerable
to cross-site request forgery (CSRF) attacks against authenticated users.

The request can be issued cross-domain.
The BDK relies solely on HTTP cookies to identify the user that issued the request.
The request performs some privileged action within the application, which returns a token.
The attacker can determine all the parameters required to construct a request that
performs the action. If the request contains any values that the attacker cannot
determine or predict, then it is not vulnerable.

Setting the SameSite=Strict will prevent sending the cookie when the request is done from another domain.

* Make sameSite attribute configurable with default value = Strict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mend: dependency security vulnerability Security vulnerability detected by WhiteSource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie set without SameSite=Strict
4 participants