jupyterhub / ltiauthenticator

A JupyterHub authenticator for LTI
https://ltiauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
67 stars 54 forks source link

LTI13 - Ensure we can defend against replay attacks #127

Closed consideRatio closed 1 year ago

consideRatio commented 1 year ago

Understanding that the LTI13Authenticator was broken anyhow, makes the merged security patch of not validating JWTs less time critical to release.

To make sure that we don't cut a release with something that does work, but is insecure, I'd like to see this TODO note understood either to be acceptable or resolved.

https://github.com/jupyterhub/ltiauthenticator/blob/bad1285b2d890fdb97e2412c008738d01d1ee8ea/ltiauthenticator/lti13/handlers.py#L241-L255

I understand this currently, as that we would be suspecptible to a "replay attack".

A nonce can be used in an authentication protocol as a method of preventing replay attacks by ensuring that old communications are not being reused. The nonce helps to prove that the message received was sent by the intended sender and was not intercepted and resent by a bad actor.

I'm not sure how this typically is handled though, is state stored in memory or on disk about already handled requests or similar?

consideRatio commented 1 year ago

@minrk I understand from @martinclaus that 1.3.0 didn't include a functional LTI13Authenticator, so the security vulnerability probably wasn't exploited. If we release ltiauthenticator before we have fixed this, we may introduce a exploitable vulnerability though.

I'm very low of maintainer time capacity, but don't dare make a release until this issue has been considered as it is now. Do you have input about this possible vulnerability?

manics commented 1 year ago

To take advantage of this an attacker will need to record the request in the first place.

If LTI13 has never worked, then given the limited maintenance and expertise we could drop it from this repo, but offer it up for the community to maintain as a separate project outside JupyterHub?

consideRatio commented 1 year ago

To take advantage of this an attacker will need to record the request in the first place.

I figure recording the request as a HTTPS encrypted request is far easier than as a non-encrypted request. Do you understand this a vulnerability also if the request is recorded as a HTTPS encrypted request, or just non-encrypted?


@manics can you open an issue in jupyterhub/team-compass about dropping or relocating this repo from the jupyterhub github org?

martinclaus commented 1 year ago

According to the LTI security specs it is optional to check if a nonce has already been received.

minrk commented 1 year ago

On further re-reading, it looks like LTI13 is making some choices we probably shouldn't be doing in an authenticator and I think it may be right to remove it and start again if we don't have folks with capacity to maintain it and get it working. These are fixable, so if someone wants to finish the implementation and commit to maintaining it, that would still be welcome.

The use of response_type: id_token to authorize requests via implicit-grant flow isn't really what should be done. It should be following the authorization code flow like all other OAuth authenticators do, and issuing an access token (as opposed to an identity token, so response_type: 'code').

Implementing (and validating!) nonce is important for the implicit flow, which has this replay issue. authorization_code doesn't generally use a nonce, though it can.

I think part of this relates to implementing OIDC in oauthenticator - I think defining a specific subclass in oauthenticator that adds the OIDC features would help us implement something standard and reliable which could be used for things like LTI13.

martinclaus commented 1 year ago

The use of response_type: id_token to authorize requests via implicit-grant flow isn't really what should be done. It should be following the authorization code flow like all other OAuth authenticators do, and issuing an access token (as opposed to an identity token, so response_type: 'code').

I figure that LTI 1.3 relies on OpenID Connect implicit flow since an LTI 1.3 conformant tool (relying party) may be some JS running in a browser. Also, the tool needs to obtain some user profile information to be able to send back e.g. grading information to the platform.

minrk commented 1 year ago

That makes sense, if there's a reason, thanks!

We definitely need someone who knows the LTI idiosyncrasies who can take responsibility for this package, because we don't currently have the ability to do any development or testing.

It does seem odd that LTI wouldn't support authorization code grants, which would be the normal way to auth with an openid provider.

martinclaus commented 1 year ago

I can help out maintaining this package. I would need some help and guidance, though.

minrk commented 1 year ago

That would be wonderful! I can't really offer any guidance for the LTI side, but I can definitely help from the JupyterHub side.

I think these might be the right steps, given the fact that apparently our LTI 1.3 as-released hasn't worked yet, if I've understood correctly:

  1. make a release removing LTI13
  2. open a PR reverting that change, where we can work on developing a solid implementation with a bit less pressure
martinclaus commented 1 year ago

I'm not sure how this typically is handled though, is state stored in memory or on disk about already handled requests or similar?

To pick up the original intent of the issue: For LTI 1.1, replay attacks are mitigated by storing used nonces in a class attribute.

martinclaus commented 1 year ago

The OpenID specs suggest to store a random string as a session cookie and use a hash of that as nonce.

martinclaus commented 1 year ago

Above added in 85c1901.