noVNC contains the session token in URL and insecurely sets the session cookie

[Jason Hullinger]

The VNC Console connection in Nova works by having the user connect to the API which returns a URL such as: https://example.com:443/?token=abc Where the token has a TTL which is then used to create a session from a WebSocket. However, URL's should not contain sensitive information such as session tokens with a TTL since URL's can be leaked through proxy logs or other types of attacks such as Cross-Site Scripting. Additionally, due to the session cookie being set with JavaScript it cannot securely be set to HttpOnly nor is it set with the Secure flag making it further susceptible to Cross-Site Scripting attacks or leakage through a non-SSL connection. To limit the exposure of the token being leaked through the URL the returned token from the API should be of a one-time use and only used as an authentication token in order to obtain a session. The session cookie should be set by a Web Service instead of the client in order to securely set the cookie with the HttpOnly flag to be set in addition to setting the Secure flag.

[Loganathan Parthipan (parthipan)]

A possible design to address this without having to change noVNC:

1. Generate the URL as usual.
2. Upon the first use of the URL:
        2.1 replace the token within nova to a new token (effectively invalidating the original token)
        2.2 Set this new token as a session cookie (secure=True, httponly=True) in the browser
3. Subsequent Authentications (for html and js resources) will use this session cookie
4. Upon switching to websockets, invalidate the session cookie both from the browser and the token from nova.

I don't think if step 4 is a good approach. A better way would be to track the state of the authentication in the server side so that no new auth can be done with this token but leave the existing connection use it as necessary. But this would require more changes.

[Thierry Carrez (ttx)]

That sounds like strengthening steps rather than a practical vulnerability fix. Those steps should definitely be implemented, but I see no reason to keep this bug private while we fix it or issue a security alert about it. Thoughts ?

[Jeremy Stanley (fungi)]

I agree this is at most a contributing risk and doesn't sound exploitable except when combined with more significant vulnerabilities. The design already assumes HTTPS with server certificate validated by the browser, which means that any MitM (logging proxy or otherwise) indicates an already-compromised system. Similarly, without any corresponding XSS vulnerability with which to leverage this weakness, it seems excessive to work on a hardening patch for this in private.

[Thierry Carrez (ttx)]

Agreed, will open the bug publicly tomorrow, unless someone complains.

[Jason Hullinger (jason-hullinger)]

Agreed on opening it up and that this is hardening but it is fundamental security practice to securely set session cookies.

[Loganathan Parthipan (parthipan)]

Michael: attached a patch that was made against a Diablo derived codebase with the hope that this might be useful.

[Michael H Wilson (geekinutah)]

@parthipan

Much appreciated.

[Michael H Wilson (geekinutah)]

@parthipan and Paul Murray

Do you guys have a newer version of this patch? I keep starting to try and apply this to trunk and then getting lost. If you have it around it would be awesome to see it submitted or if you want attach it here and I will put it in.

[Sean Dague (sdague)]

Moving to opinion state. From nova IRC:

<PhilD> sdague: No i don't think we cut a patch for more recent cloud - since there was general agreeement that this wasn't exploitable

[David Ibarra (dibarra)]

Attached is a quick and dirty hack for Juno, it just deletes the token whenever it's verified once.

[Daniel Berrange (berrange)]

SPICE displays involve multiple socket connections (as many as 10 or more). So I don't think you simply delete the token on first usage - it would prevent the other SPICE connections from succeeeding.

[David Ibarra (dibarra)]

I just loaded up VNC and it seems to work fine for me, the ctrl+alt+delete function works and everything.

It is a hack, but it at least protects against a MITM/XSS situation where someone can grab the token.

[Matthew Edmonds (edmondsw)]

@sdague, It is a well-accepted security statement that you should never pass secrets in query parameters. Maybe how to fix it is opinion, but not that there is a problem here.

You don't have to have a MitM or XSS to exploit this. Query parameters are stored in browser history, so launching a session from a shared computer also leads to exposure. Access to a web cache also equals exposure. Etc.

[Matthew Edmonds (edmondsw)]

This is what AppScan (a dynamic security scanning product) says about this kind of issue:

Query Parameter in SSL Request
Threat Classification: Information Leakage
Causes: Query parameters were passed over SSL, and may contain sensitive information
Security Risks: It may be possible to steal sensitive data such as credit card numbers, social security numbers etc. that are sent unencrypted
CWE: 598
X-Force: 52845

References:
Financial Privacy: The Gramm-Leach Bliley Act, https://www.ftc.gov/tips-advice/business-center/privacy-and-security/gramm-leach-bliley-act
Health Insurance Portability and Accountability Act (HIPAA), http://www.hhs.gov/hipaa/index.html
Sarbanes-Oxley Act, https://www.sec.gov/spotlight/sarbanes-oxley.htm
California SB1386, http://info.sen.ca.gov/pub/01-02/bill/sen/sb_1351-1400/sb_1386_bill_20020926_chaptered.html

Technical Description:
During the application test, it was detected that a request, which was sent over SSL, contained parameters that were transmitted in the Query part of an HTTP request.
When sending requests, the browser's history can be used to reveal the URLs, which contain the query parameter names and values.
Due to the sensitivity of encrypted requests, it is suggested to use HTTP POST (without parameters in the URL string) when possible, in order to avoid the disclosure of URLs and parameter values to others.

[Jeremy Stanley (fungi)]

I still maintain it's a security hardening opportunity. Someone would need access to your browser history _and_ an opportunity to use it before the token expires, so not a gaping hole but still potentially worth fixing in a future release.

Nova devs however seem to feel this isn't worth fixing at all, hence the "opinion" state on their bugtask. You could try submitting a fix to Gerrit for it, but there's no guarantee it'll be met with much urgency from them (or ever even reviewed at all).

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/678234

[OpenStack Infra (hudson-openstack)]

Change abandoned by Balazs Gibizer (<email address hidden>) on branch: master
Review: https://review.opendev.org/678234
Reason: @Matt: thanks for the pointer. I will work on https://review.opendev.org/#/c/220622 instead.