openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.19k stars 914 forks source link

OAuth flow should show more information about the application requesting rights #4217

Open pietervdvn opened 1 year ago

pietervdvn commented 1 year ago

URL

https://www.openstreetmap.org/oauth2/authorize?client_id=sa1ngLJBJ8McmzHElN8NYtIDm5TZTYEYhq3-0snO4Qc&code_challenge=VgMbsKx3KZFEMM4ujihX5YORn4m8cUVzyjW41MAYjD0&code_challenge_method=S256&redirect_uri=http%3A%2F%2F127.0.0.1%3A1234%2Fland.html&response_type=code&scope=read_prefs+write_prefs&state=eX9ZWBxNWlQBou_OLWEPeNxtc_ojhO6XuagD4uVr-EU

How to reproduce the issue?

MapComplete recently had a security scan by Radically Open Security - thanks to the NlNet fund.

One of their recommendations was to start using OAuth 2.0 (which has now been done). However, they pointed out another weakness in the OAuth flow on the side of osm.org. I'm quoting @jacopojannone here from their research:

An attacker could still register their own application on OpenStreetMap, set up a malicious instance of MapComplete, and persuade users into using it to log into OpenStreetMap. Ideally, the OAuth authorization page on OpenStreetMap would clearly show which application the user is logging into, including the owner's name and the full application URL. This would make it evident to the user that they are not logging into a trusted instance of MapComplete. However, tests showed that this is not the case. When the OAuth 2.0 flow is used, the OpenStreetMap authorization page only shows the registered application name, with no other details, author names or URLs, as shown in the following figure.

image

This is handled a bit better in the current OAuth 1.0 flow, where the URL can be detected:

image

I propose that this flow is improved by:

  1. Showing the URL (or at least the host) verbatim next to the application
  2. Showing the maintainer of the application (even though that this might be a bit confusing for contributors)
mmd-osm commented 1 year ago

I remember we had a similar discussion about who's responsible for a particular OAuth 2.0 application a while ago: https://github.com/openstreetmap/openstreetmap-website/pull/3177#issuecomment-840054891

tomhughes commented 1 year ago

Unfortunately doorkeeper doesn't have anywhere to collect and store a URL for the application in the way the OAuth 1 library does which is why we don't show that.

I don't see that it helps anyway as surely your hypothetical attacker would just enter your URL in the same way they could enter MapComplete as an application name. Unless you're expecting us to start manually approving all applications collecting and displaying extra data isn't going to prove anything.

Equally showing the OSM username of the owner isn't going to help as an OSM username is unlikely to have any meaning to the person authorising the application and in any case there is nothing to stop somebody creating a user called "Map Complete Application" or whatever to own their fake MapComplete.

It would also be confusing because many existing official applications are probably owned by unlikely accounts - for example I have no idea who owns JOSM but I know that the embedded iD instance on osm.org is on my account!

pietervdvn commented 1 year ago

That is precisely what I mean.

About https://github.com/openstreetmap/openstreetmap-website/pull/3177#issuecomment-840490033 where Tomhughes writes:

I don't the OSM username of the person that created the application tells you much though - if you do that for iD or Potlatch then I think it points at me (tomhughes) for example!

I did register an account named MapComplete which is the 'owner' of the MapComplete client registration.

From a security perspective, one can still register the user names mapcomplete, mapComplete or Mapcomplete (different casing) - not to mention MapComplеte (with a cyrillic е instead of an e).

pietervdvn commented 1 year ago

I don't see that it helps anyway as surely your hypothetical attacker would just enter your URL in the same way they could enter MapComplete as an application name. Unless you're expecting us to start manually approving all applications collecting and displaying extra data isn't going to prove anything.

This is where the 'redirect URLs' come in. If a URL is entered, the redirect after authorization should be to only this entered URL, thus bringing the victim back to the trusted, actual application that they think they are authorizing.

tomhughes commented 1 year ago

Is that not already the case?

pietervdvn commented 1 year ago

Yes, the redirect URLS do already exist. Maybe the (hostname of) the redirect URL can be shown in the OAUth-authorize screen?

This URL can be loaded from the URL of the authorize-page itself as it is contained there.

tomhughes commented 1 year ago

We could do but the redirect is not always to a URL with a host and even if it is the host may not be the same as the application site that the user recognises so it needs some thought.

I don't think I've ever seen an OAuth flow which does that - they do usually show some information about the application but any URL is usually separate metadata not directly related to the redirect.

milan-cvetkovic commented 1 year ago

Perhaps we could ask for URLs to the app at the time when the app is registered and store it in the database. When we display "Authorization Required" we could display those links, if they are available.

pnorman commented 1 year ago

Perhaps we could ask for URLs to the app at the time when the app is registered and store it in the database. When we display "Authorization Required" we could display those links, if they are available.

If the concern is security, the application could fill in whatever they want for informational URLs.