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

Authentication problems to resolve #221

Open
rosshorne opened this issue Aug 9, 2023 · 13 comments
Open

Authentication problems to resolve #221

rosshorne opened this issue Aug 9, 2023 · 13 comments

Comments

@rosshorne
Copy link

Some authentication issues to find a resolution to in the Solid-OIDC and Solid protocol specs, in order of priority:

(Auth 1) Essential for Solid-OIDC: RFC 9207 should be adopted to avoid identity provider mixup (masquerading) and cross-site request forgery attacks. The measure described by the RFC it to simply add an iss field to the HTTP header of the response, as reported in RFC 9207. This applied to Solid-OIDC.
https://datatracker.ietf.org/doc/rfc9207/

(Auth 2) Editorial decision in the Solid Protocol Spec itself: WAC should be dropped entirely from the spec and ACP made normative. The property acp:client in the context graph is essential. It ensures that users who, of course, can use the same identity for multiple apps do not allow information intended for one app to be accessed by another app. This has been discussed previously (see below), but not resolved; yet I argue this is critical, so action should be taken.
solid/web-access-control-spec#81

(Auth 3) Optimisation. RFC 7636 (Proof Key for Code Exchange by OAuth Public Clients) is not effective, so can be safely dropped to simplify the Solid-OIDC protocol.

@acoburn
Copy link
Member

acoburn commented Aug 9, 2023

Auth 1

Adding support for RFC 9207 seems like a good idea, but I am not convinced that it should necessarily be required by Solid-OIDC. An OAuth 2.0 client library will already bind the relevant issuer to an OAuth session; an issuer mixup is theoretically possible, but it would require some significant implementation flaws in the given OAuth 2.0 client library. This is also currently mitigated in several other ways, including using DPoP's mechanism of binding an authorization code to a DPoP key. PKCE also assists with this.

Clarification: the iss parameter is not sent as a response header, rather it is a query parameter in the authorization response.

Auth 2

This is out of scope for Solid-OIDC and should be raised on the Solid Protocol repository.

Auth 3

RDF 7636 becomes required under the draft OAuth 2.1 specification. The best security practices for OAuth 2.0 describe PKCE as required for public clients. Dropping it from Solid-OIDC seems like an odd suggestion.

@rosshorne
Copy link
Author

rosshorne commented Aug 9, 2023

Hi Aaron, That's a classic authentication mistake to think that since a client thinks it's binding to a session that cannot be manipulated. I maintain that "Auth 1" is required. In short, the client can bind to one issuer, without knowing that another issuer is used to complete the session. Such an attack can allow an attacker to masquarade as an honest app with an honest user logged in. That makes this issue security critical.
I'm fine with your terminology r.e. iss.

Auth 2. Can you please link to where you think the correct channel is for raising this. Thank you.

For Auth 3. Indeed PKCE is not an effective measure in Solid-OIDC. That we can prove using the standard threat models for authentication. I am open to discussion about why RFC 7636 is required in OAuth 2.1, and the associated security implications.

@woutermont
Copy link
Contributor

Thanks for creating an issue on this, @rosshorne.

As I already indicated in private and public chat channels, I agree with you that issuer identification (RFC 9207) is a must and that as long as it is not a requirement of the OAuth version we depend on, Solid-OIDC should specify that requirement itself.

I do agree with @acoburn on the other two points. As we had to conclude in private communication, PKCE prevents actual attack vectors not covered fully by OAuth2.0 + RFC9207.

Issues surrounding the protocol in general can be raised in https://github.com/solid/specification/issues.

@rosshorne
Copy link
Author

Hi @woutermont thanks for reinforcing RFC 9207. I wonder what is the best concrete strategy to implement this in the specifications. Both the Solid OIDC primer and spec should be updated (and library developers informed).

For background @acoburn indeed Wouter already evaluated the evidence and agrees with the mix up/masquerading attack, thank you Wouter. Fortunately this can be handled independently of my secondary concern about RFC 7636.

R.e. RFC 7636: indeed there are claims in RFC 7636 itself that there are attack vectors addressed by PKCE. However, the standard attacker models for authentication can be used to prove that RFC 7636 is ineffective, at least without further features that do not appear in the Solid-OIDC specification. This is perhaps a symptom of a deeper problems that probably should be explored. There are several possibilities, one of which would be safe to discuss here (others should be on a private channel). It is possible that, some implementations perhaps deviate from the Solid-OIDC specification or use alternative flows that are not yet specified, in which case the spec should somehow better encompass those features. We'd probably need a dialogue to identify precisely what is missing (in the sense that a measure is in implementations but not in the spec).

Resolving (Auth 1) and discussing (Auth 3) can run in parallel, since they are independent and have different priorities.

@rosshorne
Copy link
Author

Hi Aaron, Wouter, Frederick?, Laurens?, and other OIDC experts.
We could schedule a special topics meeting on this. How about 24 October?
That seems to be the next slot in the calendar here:
solid/specification#555
Later slots are also good if there are clashes.

@woutermont
Copy link
Contributor

Sure, we could switch it up with the terminology topic, no issue. @csarven can you do that and add dates? Thansk!

@elf-pavlik
Copy link
Member

Today's special topic meeting will be focused on this issue: https://hackmd.io/Wyk_rMoZS3iqppUyCNw9Wg

@elf-pavlik
Copy link
Member

For the record, I see a related issue in inrupt/solid-client-authn-js#2985 , which in turn links to other related issues including one in CSS CommunitySolidServer/CommunitySolidServer#1276

@elf-pavlik
Copy link
Member

We have next meeting scheduled for Nov 7th solid/specification#555

@rosshorne
Copy link
Author

Thank you @elf-pavlik. Here are some key references from last time that are safe to post, since they are in public domain and covered by RFC 9207.

The original work by Fett on OAuth:
https://doi.org/10.1145/2976749.2978385

A lifting of the attack vector discussed to Solid OIDC:
https://doi.org/10.3390/info14070411

mixup-attack.pdf

@elf-pavlik
Copy link
Member

Today only @acoburn and I joined the scheduled meeting. We decided that we are going to:

  • add RFC 9207 as SHOULD
  • add security considerations explaining that it should be a MUST for parties in the open ecosystem (needs further details)

@rosshorne
Copy link
Author

Hi @elf-pavlik @acoburn excuse me. I must have mixed up time zones (I'm available now).
I think your resolution is good OK. I can contribute a short explanation when writing.

@rosshorne
Copy link
Author

rosshorne commented Nov 7, 2023

Already having RFC 9207 as MUST (most places) plugs most Cross-Site Request Forgery (CSRF) attacks I was worried about, so that's great.

However, for the record, I was also wanting to discuss whether some session state information should be made explicit. There's an outlier threat model, explained in the PKCE RFC (RFC 7636), where the code is leaked by an app for whatever reason (this was pointed out by @woutermont ). PKCE is supposed to address that threat (not covered by RFC 9207). However, it only does so if there is shared state information between the first message from the app to the browser and the message containing the code sent from the browser to the app. HTTPS is not stateful unless you make it so, e.g., by passing, along with the code and iss, the h(pkce) (or a nonce passed from the app to the browser along with the h(pkce)) and having the browser check the h(pkce) (or the nonce) before passing the code on to the app. [Alternatively, some header information may make the browser aware of when two HTTPS operations involve the same TLS handshake and checking that is the case before forwarding the code to the app.] Therefore, PKCE doesn't appear to be effective without such an additional measure, which appears to be under-the-hood in the current spec (but may well be in already addressed in implementations).

This threat related to RFC 7636 is definitely secondary to the one discussed previously regarding RFC 9207. Therefore we should first update the spec, as you both suggest, r.e., RFC 9207.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

4 participants