rstudio / shiny

Easy interactive web applications with R
https://shiny.posit.co/
Other
5.38k stars 1.86k forks source link

Session ID/token exposed with downloadHandler #3748

Open leeroyaus opened 1 year ago

leeroyaus commented 1 year ago

Hello

When creating a link to download files using downloadHandler, the session ID/token is exposed in the link. This seems to be generally regarded as a serious security issue - see details at the link below:

https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/06-Session_Management_Testing/04-Testing_for_Exposed_Session_Variables

Is it possible to use a different token/ID for links using downloadHandler? Or could you provide more information about how this might not be a serious security issue?

jcheng5 commented 1 year ago

Hi, sorry for the delay in responding.

You're right to point out that leaking session tokens in the URL is generally regarded as serious. However, in the case of Shiny there are a couple of mitigating factors:

  1. Contrary to what session tokens are usually used for, Shiny doesn't use them for authentication/authorization; that is, it's not possible to steal a session token and use it to impersonate a logged-in user (on Connect, ShinyApps.io, or any other Shiny hosting server), access the apps they are able to access, etc. Cookies are used for that instead.
  2. Shiny's session tokens are tied to the lifetime of a particular page load. Once that page is navigated away from, or a tab is closed, or even on page reload, the session token is no longer valid. (Actually there's approximately a 15 second delay for the session token to become invalid, as it takes that long for Shiny to be convinced that you're not the victim of an unintended, temporary network interruption.)
  3. Connect and ShinyApps.io are both https-only, which prevents disclosure via packet sniffing (but doesn't prevent disclosure via log files or malicious proxies).

So the attack window is quite a bit narrower, and the effect of an attack much less harmful, than the general case as described in the OWASP link. This has been enough to allay other customers' concerns in the past (it's far from the first time this has come up).

However, thinking about this one more time made me realize there is one attack vector that we could easily close, which is the scenario where Alice and Bob are both authorized users of App X. If Bob gets ahold of one of Alice's download URLs while Alice is still on the page, he can access the same download file she would get if she clicked the link. In this case, it's easy for Shiny to realize that the owner of the session and the requester of the download are not the same person. I'll file a PR to close this hole.

The upshot is that, with this upcoming change, this should be a total non-issue for any app that requires users to log in, since the session ID will be scoped to the logged-in user.

jfunction commented 1 year ago

Also worth pointing out that ideally potential security issues should not be reported/discussed in a public forum. I see that Github has introduced a "Security" tab where policies on reporting can be set up for each repo. For specific websites the standard adopted by many is to expose a /security.txt endpoint.

tylerlittlefield commented 1 year ago

@jcheng5 This is a pretty niche problem I am having but I am wondering if the details you bring up about session IDs and downloadHandlers has anything to do with an issue I have been noticing with shiny + downloadHandlers + HPAs in a kubernetes cluster: https://community.rstudio.com/t/issues-with-shiny-downloadhandler-in-kubernetes-pods-using-hpas/174945

Based on your deep understanding of shiny, do you have any ideas why a downloadHandler might return 404 on a brand new pod but succeed after waiting an extra minute or two?

Note: I am using sticky sessions with the following annotations in my ingress:

nginx.ingress.kubernetes.io/affinity: "cookie"
nginx.ingress.kubernetes.io/session-cookie-name: "route"
nginx.ingress.kubernetes.io/session-cookie-expires: "172800"
nginx.ingress.kubernetes.io/session-cookie-max-age: "172800"
nginx.ingress.kubernetes.io/proxy-read-timeout: "3600"
nginx.ingress.kubernetes.io/proxy-body-size: "500m"

Edit: I think I may be getting closer, or at least found something that looks promising: https://github.com/kubernetes/ingress-nginx/issues/5944#issuecomment-734942740

I'm going to set this annotation in the shiny apps ingress and see if this resolves it. It sounding like what is happening is:

  1. User goes to app
  2. HPA scales up
  3. Second user goes to app, but their session is lost as a result of the default affinity mode "balanced"
  4. Second user attempts to download file but it returns 404 because the session ID in the GET request no longer exists

Edit: Okay, so I think I actually have this figured out as I was able to reliably reproduce the issue and resolve it by adding the following annotation to my ingress: nginx.ingress.kubernetes.io/affinity-mode: "persistent"

For future readers, here is how I was able to reproduce the issue:

  1. Open 4 different browsers
  2. Navigate to app in each browser
  3. Manually scale pods to 1
  4. Refresh page in all browsers (ideal state is for all refreshed browsers to hit the same pod after scaling to 1)
  5. Manually scale pods to 5
  6. Wait until scale completes
  7. Click the download button within each browser, 1 will likely fail

Basically, I think what happens is during a scale up event (addition of more pods) the session gets lost for all users that were already connected to the app (prior to scale up event) and this causes the download to 404. This happens when you have the default affinity-mode of "balanced" but can be resolved by setting it to persistent.