GitHub OAuth client vulnerable to client impersonation

[hickford]

Like any OAuth native app, Git Credential Manager's OAuth client credentials are necessarily stored in the application's source code, eg. for GitHub

git-credential-manager/src/shared/GitHub/GitHubConstants.cs

Lines 15 to 16 in dc1edca

	// [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="OAuth2 public client application 'secrets' are required and permitted to be public")]
	public const string OAuthClientSecret = "18867509d956965542b521a529a79bb883344c90";

OAuth authorization servers are supposed to distinguish between public clients and confidential clients:

The authorization server SHOULD NOT make assumptions about the client type.

However GitHub assumes all OAuth clients to be confidential clients:

The Client Secret should not be shared!

This leaves public clients vulnerable to attacks such as client impersonation. In particular:

the authorization server SHOULD NOT process authorization requests automatically without user consent or interaction, except when the identity of the client can be assured [ie. the client is a confidential client]. This includes the case where the user has previously approved an authorization request for a given client id

Git Credential Manager's OAuth credentials would be an attractive target because very many users have previously authorized it.

Fixing this issue requires upstream changes in GitHub.

See also hickford/git-credential-oauth#17 for further comparison of OAuth servers.

[hickford]

cc. @hpsin

[dscho]

@hickford when reporting potentially critical security issues, do consider to report them responsibly, i.e. non-publicly. See https://docs.github.com/en/code-security/security-advisories/guidance-on-reporting-and-writing/privately-reporting-a-security-vulnerability for better ways to report vulnerabilities.

[hpsin]

Hi @hickford , this is a good call out. I'll update the documentation here - we just updated them and some guidance was taken a bit stringently.

GitHub splits functionality into two categories:

1. Server-to-server (the client credentials flow) which is for GitHub apps only and relies on the application private key. That doesn't exist for the OAuth app here, and that's what must never ship in native code.
2. User-to-server (authorization code flow) which relies on the client secret. The client secret is only usable in conjunction with the authorization code gotten in the user's browser and sent to the registered redirect URI. The threat model here is the same with or without the client secret in the picture, so it's been low priority to support a "native app without a client secret" mode, although our device code flow does correctly not require the client secret.

Primarily, the reason we haven't prioritized a dedicated native client flow for OAuth apps is that it doesn't change anything meaningfully except for some dual native+web apps with redirect bugs.

We're focusing on making GitHub apps better so that they can ultimately replace Oauth apps. We will be fixing some of the valid issues you call out, such as automatic redirection to public clients - that's on my table for this quarter.

[hickford]

We will be fixing some of the valid issues you call out, such as automatic redirection to public clients - that's on my table for this quarter.

@hpsin Brilliant, I agree that's the most important issue. So OAuth client registration will include a choice of public/confidential client type?

[hickford]

@hickford when reporting potentially critical security issues, do consider to report them responsibly, i.e. non-publicly. See https://docs.github.com/en/code-security/security-advisories/guidance-on-reporting-and-writing/privately-reporting-a-security-vulnerability for better ways to report vulnerabilities.

@dscho Thanks, agreed. I didn't consider this a vulnerability discovered but "a reminder to follow the spec for the reasons discussed in the spec".

[hpsin]

We won't be adding a public/confidential option with this work - instead, we’ll be looking at redirect urls to determine what's being sent to a native client, so that this applies retroactively to all clients.

[hickford]

We won't be adding a public/confidential option

@hpsin Thanks for clarifying. I urge you to follow the spec closely https://datatracker.ietf.org/doc/html/rfc8252#section-8.4

Authorization servers MUST record the client type in the client registration details in order to identify and process requests accordingly.

You suggest:

we’ll be looking at redirect urls to determine what's being sent to a native client

I introduced (and later fixed) a similar assumption in https://github.com/go-gitea/gitea/ . A problem is
https://datatracker.ietf.org/doc/html/rfc8252#section-7.2 :

Some operating systems allow apps to claim "https" scheme URIs in the domains they control. When the browser encounters a claimed URI, instead of the page being loaded in the browser, the native app is launched with the URI supplied as a launch parameter.

Such URIs can be used as redirect URIs by native apps. They are indistinguishable to the authorization server from a regular web-based client redirect URI. An example is:

https://app.example.com/oauth2redirect/example-provider

As the redirect URI alone is not enough to distinguish public native app clients from confidential web clients, it is REQUIRED in Section 8.4 that the client type be recorded during client registration to enable the server to determine the client type andact accordingly.

[hpsin]

Yep, when I owned OAuth at Microsoft we had similar problems, and we spent a good bit of time talking to Google, Windows, and Apple about universal URIs. It's why Microsoft apps support per-URI distinction instead of per-client-ID. We have longer term plans to introduce client type distinctions for GitHub, but for now, it's a targeted fix using just the URI. OS-bound HTTP redirects offer a decent bit more security than native redirects - both Apple and Google let you leverage your DNS entries to restrict the apps that can take over an HTTP URI, so the danger there is reduced (the third section of 8.4).

Thanks for calling these out, and putting together the tables documenting compliance across various AS. I know I found that useful when discussing changes we should be making on our platforms.

[ldennington]

Closing as there is no work to be done on the GCM side to correct this.

[hickford]

We have longer term plans to introduce client type distinctions for GitHub

Brilliant, I look forward to that