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

Nonce security when the tokenMaxAge is set to false #175

Open
priyachawla11 opened this issue Jan 18, 2023 · 11 comments
Open

Nonce security when the tokenMaxAge is set to false #175

priyachawla11 opened this issue Jan 18, 2023 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@priyachawla11
Copy link

Security Query Description

If tokenMaxAge is disabled, then the idtoken will be valid till the expiry time mentioned in the LTI payload. But nonce has a TTL of 10s, so it will be deleted from the Database after 10sec. So what is the security if the same launch request is replayed after 10sec within the window of idtoken validity?

Should Nonce TTL be equal to the idtoken expiry time to mitigate the replay attacks between the window of idtoken validity (max observed 60min)?

or

Are we relying on state param for such a case? But if the 'savedState' is false no error is thrown i.e if the state is not present in Database.

(By default, as tokenMaxAge is 10s and Nonce TTL is 10s, it remains in sync. The request is not entertained because of tokenMaxAge validation, so nonce auto-deletion is not affecting.)

LTIJS Security understanding

  1. State is created, inserted in the Database, and in the cookie header with maxAge as 1 min.
  2. The nonce is created while processing the login request.
  3. Nonce and state is sent in the payload of the OIDC request to the Platform.
  4. On receiving the respective launch request, the nonce is checked in Database.
  5. If the nonce is not present in Database, then it is inserted in Database and the launch request is processed.
  6. If the nonce is present in the Database, then the 'NONCE_ALREADY_RECEIVED' error is thrown to mitigate replay attacks.
  7. The LTI payload is verified, which throws TokenExpiredError if the current timestamp is exceeding the LTI payload exp claim.
  8. Then tokenMaxAge validation is checked. If more than tokenMaxAge has passed from iat, then the LTI payload is considered expired.
  9. State is checked in the Database, and is removed from the Database if present.
  10. State is removed from the cookie header.

LTI1.3 Spec for id token expiry

  1. The exp Claim MUST be an absolute expiry time for the message, typically five minutes after the iat timestamp.
  2. The Consumer MUST honor this expiry time.
  3. Though Consumer MAY also choose to expire the JWT at an earlier time (but no earlier than the iat value) = LTIJS TokenMaxAge

LTI1.3 Spec for state
An opaque value is used to maintain the state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie.

LTI1.3 Spec for nonce
A string value is used to associate a Client session with an ID Token, and to mitigate replay attacks. The value is passed through unmodified from the Authentication Request to the ID Token.

Observed Expiry times of idtokens for Platforms

Deployment Type Expiry Time [exp-iat]
Blackboard 60 min
Moodle 1 min
Canvas 60 min
Brightspace 30 min

We believe:

Nonce should be saved in Database while sending the login request and deleted on receiving the launch request, so that replay attacks won't be entertained as nonce cannot be reused in replay attacks after deletion

If the nonce request is received again or if the arbitrary nonce value is received, then it will not be present in Database and an error will be thrown.

@priyachawla11 priyachawla11 added the bug Something isn't working label Jan 18, 2023
@priyachawla11
Copy link
Author

@Cvmcosta Please help with the above query

@Cvmcosta
Copy link
Owner

@priyachawla11 Hello! Sorry for the late reply, i have been unable to find the time to work on this library recently. If i understand your question correctly, you are saying Ltijs should store the nonce during the login request instead of when receiving a launch request? And then throw the error if the nonce is not in the database during the launch request processing?

I like this idea, it's an interesting approach. As i said i don't have a lot of time at the moment, i will try to implement this as soon as i am more available. In the meantime, if you want to create an PR with this implementation, it would be a massive help, since reviewing takes a lot less time than actually implementing.

@priyachawla11
Copy link
Author

@Cvmcosta Can you please confirm the following too before the improvement changes:
If tokenMaxAge is disabled, then the idtoken will be valid till the expiry time mentioned in the LTI payload. But nonce has a TTL of 10s, so it will be deleted from the Database after 10sec. So for security (without nonce improvement), if the same launch request is replayed between the window of idtoken validity (max observed 60min) - Nonce TTL be equal to the idtoken expiry time?

@Cvmcosta
Copy link
Owner

Cvmcosta commented Feb 16, 2023

@priyachawla11 Yes, you are correct

@priyachawla11
Copy link
Author

@Cvmcosta Thanks for resolving query 😊

@priyachawla11
Copy link
Author

@Cvmcosta I am unable to push my branch (nonce-security) to the repository for PR due to the following error -

remote: Permission to Cvmcosta/ltijs.git denied to priyachawla11.
unable to access 'https://github.com/Cvmcosta/ltijs.git/': The requested URL returned error: 403

Please help.

@priyachawla11
Copy link
Author

@Cvmcosta A gentle reminder to please help me with the above, I am unable to push my branch to the repo.

@Cvmcosta
Copy link
Owner

Cvmcosta commented Apr 4, 2023

Hello @priyachawla11! How are you trying to push? Did you create a fork of ltijs before creating a new branch? You cant push directly to my repo.

@priyachawla11
Copy link
Author

@Cvmcosta Thanks, was able to create PR after forking.
#185

@priyachawla11
Copy link
Author

@Cvmcosta A gentle reminder to please review the PR (#185).

@Cvmcosta
Copy link
Owner

Cvmcosta commented May 4, 2023

@priyachawla11 I am really sorry i haven't had the time to review your PR yet. I will do my best to find time and review it in the next few days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants