[OSSA 2015-005] Websocket Hijacking Vulnerability in Nova VNC Server (CVE-2015-0259)

[Josh Kleinpeter]

OpenStack Vulnerability Team:

Brian Manifold (<email address hidden>) from Cisco has discovered a
vulnerability in the Nova VNC server implementation. We have a patch for
this vulnerability and consider this a very high risk.

Please email Dave McCowan (<email address hidden>) for more details on the attached patch.

Issue Details:

Horizon uses a VNC client which uses websockets to pass information. The
Nova VNC server does not validate the origin of the websocket request,
which allows an attacker to make a websocket request from another domain.
If the victim opens both an attacker's site and the VNC console
simultaneously, or if the victim has recently been using the VNC console
and then visits the attacker's site, the attacker can make a websocket
request to the Horizon domain and proxy the connection to another
destination.

This gives the attacker full read-write access to the VNC console of any
instance recently accessed by the victim.

Recommendation:
 Verify the origin field in request header on all websocket requests.

Threat:
      CWE-345
 * Insufficient Verification of Data Authenticity -- The software does not
sufficiently verify the origin or authenticity of data, in a way that
causes it to accept invalid data.

      CWE-346
 * Origin Validation Error -- The software does not properly verify that
the source of data or communication is valid.

      CWE-441
 * Unintended Proxy or Intermediary ('Confused Deputy') -- The software
receives a request, message, or directive from an upstream component, but
the software does not sufficiently preserve the original source of the
request before forwarding the request to an external actor that is outside
of the software's control sphere. This causes the software to appear to be
the source of the request, leading it to act as a proxy or other
intermediary between the upstream component and the external actor.

Steps to reproduce:
 1. Login to horizon
 2. Pick an instance, go to console/vnc tab, wait for console to be loaded
 3. In another browser tab or window, load a VNC console script from local
disk or remote site
 4. Point the newly loaded VNC console to the VNC server and a connection
is made
Result:
 The original connection has been been hijacked by the second connection

Root cause:
 Cross-Site WebSocket Hijacking is concept that has been written about in
various security blogs.
One of the recommended countermeasures is to check the Origin header of
the WebSocket handshake request.

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

[Tristan Cacqueray (tristan-cacqueray)]

The vulnerability reported seems exploitable. @Alaski can you please check nova impact and the proposed patch ?

[Andrew Laski (alaski)]

I am trying to understand this fully as I do not have a lot of prior experience with cross-site type exploits. When requesting console access from Nova it returns a URL with an auth token included as a GET query parameter. On its own there doesn't seem to be anything exploitable here. The problem seems to come in when that URL is passed into a browser and the token is stored in a cookie? And then subsequent attempts at accessing that URL from the same browser would pass the token in that cookie. It seems that using the token from the cookie is part of the security concern, but may not be something we can stop doing based on a comment in the code.

There does appear to be some risk here, but I do have concerns about the proposed patch. It is possible that some deployers are currently relying on the behavior explained here so there is a risk of breaking them. I would suggest amending the patch so that the origin checking is configurable, with the default being to check it.

[Tristan Cacqueray (tristan-cacqueray)]

@alaski: (if i understand this correctly) the problem is that a malicious website can initiate a connection directly to the websocket server on behalf of the user (thus using the previously stored token), and without a check on the origin this will effectively hijack the session.

For what it worth, I doubt there is a real use-case of having the connection origin different from the host. The proposed fix seems good to me.

[Josh Kleinpeter (jkleinpeter)]

@tristan-cacqueray: yes, that is correct. From there, you can do what you like with the instance.

[Andrew Laski (alaski)]

Assuming the user is logged in to the instance on the websocket session, yes it provides total access to the instance.

I'm okay with the patch as written as it would address the issue and if configurability is desired later it can be added in. I would prefer that a ValidationError be raised as opposed to creating a new exception class but that's my only nitpick.

[Tristan Cacqueray (tristan-cacqueray)]

Can the patch be re-formatted to include commit message ?

Also about affected version, I guess it goes back to stable/icehouse, thus backports for Juno and Icehouse are required as well...

Finally the report only mention VNC console, but SPICE seems affected as well right ?

Here is impact description draft #1:

Title: Nova console Cross-Site WebSocket hijacking
Reporter: Brian Manifold (Cisco)
Products: Nova
Versions: up to 2014.1.3 and 2014.2 versions up to 2014.2.1

Description:
Brian Manifold from Cisco reported a vulnerability in Nova console websocket. By tricking an authenticated user into clicking a malicious URL, a remote attacker may trigger a cross-site-scripting vulnerability resulting in potential hijack of consoles where the user is still logged in. Only Nova setups with vnc or spice enabled are affected.

[Dave McCowan (dave-mccowan)]

Patch for Kilo

[Dave McCowan (dave-mccowan)]

Patch for Juno

[Dave McCowan (dave-mccowan)]

Patch for Icehouse

[Dave McCowan (dave-mccowan)]

I've attached 3 patches that address the requests made by Andrew and Tristan (include commit message, use ValidationError exception, and provide patches for Icehouse and Juno).

[Thierry Carrez (ttx)]

nova-coresec please review proposed patches

[Tristan Cacqueray (tristan-cacqueray)]

Thanks for the patches, they passed ./run_tests.sh here. However flake8 failed for an undefined name 'null' in tests and some other minor warnings.

I also switched the OSSA task to 'In Progress' as an impact description have been proposed in comment #8.

[Dave McCowan (dave-mccowan)]

Update to stable-icehouse patch

[Dave McCowan (dave-mccowan)]

Update to stable-juno patch

[Dave McCowan (dave-mccowan)]

Update to master-kilo patch

[Dave McCowan (dave-mccowan)]

Thanks Tristan. New patches uploaded fixing flake8 issues.

[Thierry Carrez (ttx)]

@nova-coresec please review proposed patches!

[Andrew Laski (alaski)]

The patches look good to me, and will address the issue described here.

[Jeremy Stanley (fungi)]

Tristan's impact description in comment #8 looks fine except that now it affects up to 2014.2.2 (released earlier today).

[John Garbutt (johngarbutt)]

I am +2 on these nova patches too, looks good. Sorry for the need to ping me quite so much.

I have slight reservations on deployment scenarios where this header check is too aggressive, but its an important bug to fix, so its probably a case of crossing the bridge when we come to it. It would be worse to make it configurable to start with.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks you for the review! Now that we have a CVE, proposed disclosure date:
2015-02-12, 1500UTC

[Paul McMillan (paul-mcmillan)]

(first a nickpick, this is CSRF or CSWF, definitely not XSS mentioned in the comment in the patch)

The patch improves things, but it doesn't completely solve the problem.

I'm sorry to bring these things up, since they're not going to be trivial to patch.

We need to verify that the url scheme (https or http) in the origin matches the expected scheme. Otherwise, a mitm can spoof an unencrypted page on the same domain, obtain an origin header with the same domain, and conduct the attack against an HTTPS cloud.

We need to maintain a whitelist of expected source domains, and compare against that rather than comparing against the host header. Without this, the current implementation is vulnerable to DNS rebinding.

[Jeremy Stanley (fungi)]

Thanks--we caught the XSS vs CSRF bit after the CVE was requested with the original wording and so stuck with it in the pre-OSSA.

As for "not going to be trivial to patch" do you suspect we'll be able to backport a more thorough solution to stable branches without violating our stable branch change policy? If not, we should probably continue with the original disclosure timeline to fix what can be trivially backported while publicly solving it in a more thorough fashion for the upcoming kilo release.

[Paul McMillan (paul-mcmillan)]

I don't know if Nova already has access somewhere to the necessary information to fix these issues correctly. Perhaps someone who's more familiar can help out.

In the first case, it needs to know if the system is being served behind https.

In the second case, it needs to know the hostname it is serving from (provided by the deployer, rather than intuited from the request).

[Dave McCowan (dave-mccowan)]

For the DNS binding concern, I've been told that it's important that customers can access their consoles through either https://foo.example.net:6080 or https://192.168.8.8:6080. If we restricted access to a white list of domains, it would break that use case. Does anyone have thoughts on that?

For the http/https case, the code is checking the entire Origin, not just the domain. So, a MITM would have to spoof http://foo.example.net:6080 in order to hijack https://foo.example.net:6080. Is this the case you want to protect from? It seems that would be straightforward addition to the code... just check that the origin is HTTPS, if current connection is HTTPS.

[Tristan Cacqueray (tristan-cacqueray)]

@Paul correct me if I'm wrong, but an attacker able to redirect a user to a DNS rebinded version of a cloud domain would likely have greater capabilities than just snooping on an eventual opened vnc/spice session... e.g., hijack the whole horizon session. Thus if it's the case, we should address this in another bug.

So far the fix seems to prevent the most trivial exploitation path and we should stick to the original timeline unless there is something wrong with the proposed patch.

[Paul McMillan (paul-mcmillan)]

@Dave for the http/https case, you are correct, if you're confident that nova reliably has access to whether or not the current connection is HTTPS on the client side. It's common to run TLS decryption at cloud boundaries so that secrets aren't spread across the cloud. Does nova properly support forwarded headers to determine this state? Does it correctly ignore them when it does not need them?

Re: DNS rebinding and host whitelisting: I see no reason you can't whitelist both IP addresses and the expected domain names if that is the expected customer use case. Browsers still send the HOST header with the IP address if that's what's in the url.

@Tristan: the DNS rebinding issue is correctly mitigated in Horizon because Horizon uses Django, which provides robust protection against this class of attacks. Nova's adhoc reinvention of these mechanisms is what got us in trouble here.

@Tristan: we need to fix the http/https thing more urgently than we need to fix the dns rebinding thing. Both issues will become public when this bug becomes public, but the http/https thing is trivially exploitable. If we keep this bug private, we'll need to issue a followup patch and CVE shortly - I'm ok with that, but not linking to the public launchpad bug will encourage people to look more closely at the patch.

The DNS rebinding issue is less critical, though I suspect you'll find people are happy to demonstrate an exploit the moment you claim something isn't a problem.

As the current patch stands, it's not wrong, just incomplete. If you'd prefer I open new issues for these things, I'll do that and we can treat them separately.

[Tristan Cacqueray (tristan-cacqueray)]

@Paul: can you please describe explicitly the attack scenario here ?

I'm not that experienced with CSRF or CSWF, could you explain how an attacker would redirect a user to an un-encrypted page from the same domain ?

IIUC, all three connections required to connect to a console are protected by TLS:

[1] user requests nova-api and it returns the url of novnc proxy with a token
[2] user connect to novnc proxy and it'll serve javascript to connect to websocket proxy
[3] user's browser connect to websocket proxy

Am I missing something here ?

[Paul McMillan (paul-mcmillan)]

@Tristan: Happy to describe in more detail, these things are extremely non-intuitive if you haven't spent a while thinking about them.

Our scenario involves a setup as follows:

Nova and horizon configured to serve from https. Nova is patched with the current proposed patch.

User is accessing the cloud through a man in the middle who controls all traffic to and from the user. [1]

user -> attacker -> cloud(https)

Here's what happens:
1) User logs into cloud securely via https://yourcloud.com/
2) User securely accesses a server via websocket VNC and logs in. User (optionally) closes this window.
3) User opens a new browser tab to an insecure site (it really can be any insecure site at all)
4) On receiving the request for the insecure site, the attacker fetches it from the source, and rewrites it to include an invisible attack iframe before serving it to the user. [2]
5) The attack iframe directs the user's browser to open http://yourcloud.com:6080 inside the hidden iframe (even if you don't serve that site insecurely)
6) When the user's browser requests http://yourcloud.com:6080, the attacker intercepts this request.
7) Rather than serving the expected content, the attacker serves javascript which instructs the user's browser to re-connect to the secure websocket that was previously used for VNC.
8) When the user's browser sends this request to your secure websocket, it sends the cookies associated with the secure domain, even though the javascript which triggered this request (and controls the resulting websocket) did not come from a secure domain. The attacker can't see these cookies, but the user's browser does send them.
9) The secure server inspects the origin header. Since the origin for this websocket request is http://yourcloud:6080, the current patch allows the request (since the origin domain and port match the HOST header). Then the server checks the cookies (which are correct, since they are the users legit cookies from the earlier session) and the server allows the request.
10) Now the attacker controls a websocket which has been opened with the user's credentials. If the user left the VNC session logged in (very common when you're fixing something like broken networking, since closing the VNC terminal doesn't log the user out of the terminal), the attacker has full control over the VM and will proceed to install a permanent backdoor as quickly as possible.

Without the current patch, the attacker doesn't have to bother with the MITM, and can just open the websocket from any webpage they control. It's counter intuitive, but browsers don't prevent this kind of connection and even when the socket is opened from a foreign page, it comes with the secret cookies related to the target page, granting access.

Hopefully that helps you understand how this attack works. It does require a different attack setup than the plain CSRF scenario, but it produces similar end results. I agree with you that the current patch is a major improvement, and if it's going to take a significant delay to implement a fix for this issue, we should put out the current patch and deal with this one separately.

-Paul

[1] As a practical aside, it is easy to b...

[Paul McMillan (paul-mcmillan)]

One point about the above explanation that I didn't make clearly is that during normal operation, when the user's browser makes a websocket request for VNC, it is to wss://yourcloud.com:6080/websockify, and it sends the token which chooses the instance as a cookie.

The websocket does not send the token as a get request (even though you see it as a get request parameter when you load the html which serves the javascript vnc client), so that cookie containing the instance token persists until the next time anyone (from anywhere on the internet) asks the user browser to request a websocket to wss://yourcloud.com:6080/websockify.

[Dave McCowan (dave-mccowan)]

@Paul : Thank you very much for you detailed comments; I have definitely learned a lot.
To summarize, this patch does protect against a simple click-bait attack, but does not protect against simultaneous click-bait and MITM attacks. NoVNC has other issues with MITM (https://bugs.launchpad.net/nova/+bug/1197459), in that if a MITM attacker can capture the token from the URL, then he can access the console directly.
As you point out, the white-list solution is not a simple fix. NoVNC will need information that it may not have, such as knowledge of proxies, load-balancers, and TLS unwrappers that may affect the connection information.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks for such a detailed explanation, it makes much more sense now. So basically if the attacker is a mitm, then the vulnerability is still exploitable.

Is Dave's proposed solution in comment #27 be enough then ? ( check that the origin is HTTPS, if current connection is HTTPS. ). Does NovaProxyRequestHandlerBase have access to that information ?

[Dave McCowan (dave-mccowan)]

@Tristan: Paul pointed out my error in #29. If the TLS connection is unwrapped by an intermediate (such as stunnel), then there would need to be coordination between Nova and the unwrapper. This coordination, such as an inserted HTTP header, would probably need configuration options to make sure it is done right for each deployment.

[Jeremy Stanley (fungi)]

I suspect (but would love to be proven wrong) that any fix involving adding configuration to handle these situations will be at odds with our stable branch change policy, and as such would not be covered by the security advisory since it would only be mitigated thoroughly in the master branch and next major release.

[Paul McMillan (paul-mcmillan)]

This is how Django handles and docs the coordination between a TLS unwrapper and the application, if necessary:
https://docs.djangoproject.com/en/1.7/ref/settings/#secure-proxy-ssl-header
I believe this approach and the documentation around it is correct and reasonably standard. A similar approach would probably be correct for nova.

@Dave I was aware of the "token in the GET parameter issue" raised in 1197459, and agree that part isn't usually exploitable in a proper https site (though it is pretty horrifying), but the "cookie isn't set securely" part is actually quite exploitable. I can open a separate private bug about that.

@Jeremy: Does the stable branch policy preclude us from adding new configuration values with reasonable defaults, and saying "Add these configs if you want to be secure"?

If we can't that, we could do something truly horrible like detecting if we ever see a https referer, and then permanently shifting to only accepting https referers.

@all do we agree that given a) the current patch improves things, and b) the fixes for the issues raised here are non-trivial, we should move these issues into separate tickets?

[Jeremy Stanley (fungi)]

Our stable branch guidelines are a little vague on this point, and mainly discourage introducing backward-incompatible configuration changes. https://wiki.openstack.org/wiki/StableBranch#Appropriate_Fixes The bigger concern is backporting a significant behavior change or a change in default values of existing configuration options, I think.

Prior art involving security advisories where configuration options were added in backports: bug 1336207, bug 1354208

[Tristan Cacqueray (tristan-cacqueray)]

So we are postponing the advisory in order to find a better workaround.

The ideal would be to find a backport-able fix. But in the case this is not feasible, maybe we could come up with a better than nothing interim solution like an option to make sure origin scheme is https ?

[Paul McMillan (paul-mcmillan)]

To the best of my knowledge, Nova does not currently have the necessary information to make a correct decision about this.

I would strongly recommend that we take the same approach Django has, and add support for specifying a header which tells nova whether the request is secure or not. I wouldn't object to also supporting a setting which just flat out toggled HTTPS mode on or off, since that may be easier for many people to deploy without reconfiguring their load balancers and proxies.

In either case, the new settings should have sane defaults which don't break people's installations if they don't configure them.

[Dave McCowan (dave-mccowan)]

I think the patch should be pushed as is. The more complete fix will have to default to no-check, which will result in the same behavior as the current patch. The complete solution will require an admin to change the configuration of Nova and change the configuration of his load-balancers and proxies, at the same time. I think it might be better to address that in the open so we can get more opinions on sane defaults and recommendations that will work in a majority of the architectures.

[Tristan Cacqueray (tristan-cacqueray)]

@Dave, the problem with current patch is: user have no protection against a man in the middle. And if we open the bug, then yes we fix the broader vulnerability, but we also disclose a 0day... On the other hand, the longer we wait, the longer stakeholder have knowledge of this bug while the rest of the community is not protected. Also, since the OSSA task status is 'Fix commited', we are now able to subscribe downstream stakeholder if they want to participate in this bug fix....

According to Paul comment #40, the proper fix would be to introduce a new Header X_FORWARDED_PROTO to give the proxy enough information to validate the origin. Will this also require modification on the noVNC code and what would be the roadblocks to implement this on Icehouse ?

I think we should also consider a more simpler fix for stable. considering most deployment should already be using ssl, a check to make sure the origin is using https would effictively mitigate the issue.

@all, while we could move these issues to other bugs, because all the detail are already posted here, we'll have to also open them on disclosure day.

[Andrew Laski (alaski)]

I'm of the same opinion as Dave, that this patch should move forward as is. It addresses the originally reported issue, but not the variation of the issue that Paul brought to light. And I agree that fixing the other issue in a backwards compatible way is going to be difficult unless it defaults to bypassing enforcement, which leaves it functionally the same as this patch.

[Paul McMillan (paul-mcmillan)]

As Tristan said, it's not responsible to disclose 0day without a patch and leave users unprotected, whatever the reason. I don't want to have that conversation with my customers who want to know why there's no patch, and I know none of you do either.

Fixing the issue in a backwards compatible way (defaulting the check to not running) is NOT functionally the same as publishing 0day without an available patch. Yes, administrators who apply the patch will have to reconfigure systems in order to gain full protection, but conscientious administrators will do that.

I agree with Tristan that for stable, a simple setting that indicates the required origin protocol is an appropriate backwards-compatible, easy to configure solution to the problem.

[Dave McCowan (dave-mccowan)]

I may have been over thinking it. Perhaps there is a fix that also checks the protocol in a clean way. Nova API builds the 'access_url' that includes the proto (http or https). The Origin header in the websocket handshake includes the used proto (http or https). These two need to match. websocketproxy.py has easy access to the latter. @nova-experts: any suggestions on how to pull 'access_url' from Nova API into NoVNC?

If they do match then I believe we have a complete solution. @Paul?

[Paul McMillan (paul-mcmillan)]

If we're comfortable with how nova figures out the 'access_url' value , then that sounds like an excellent solution.

[Tristan Cacqueray (tristan-cacqueray)]

@Dave I'm not sure NoVNC needs any change... Beside, I don't think NoVNC is a supported project, it does not show up in the official programs list and does not follow the stable releases.

IIUC it is the websocketproxy that lacks the check on the proto, and an out of band solution (e.g. no additional headers) that can reconstruct the 'access_url' would be great! Can you amend this change to the initial patch in order to have a single patch for this issue ?

About the OSSA task, as the CVE have not been disclosed and the new attack exploit the same vulnerability, we may keep the same number with this updated impact description:

Title: Nova console Cross-Site WebSocket hijacking
Reporter: Brian Manifold (Cisco), Paul McMillan (Nebula)
Products: Nova
Versions: up to 2014.1.3 and 2014.2 versions up to 2014.2.2

Description:
Brian Manifold from Cisco and Paul McMillan from Nebula reported a vulnerability in Nova console websocket. By tricking an authenticated user into visiting a malicious URL, a remote attacker or a man in the middle may exploit a cross-site-websocket-hijacking vulnerability resulting in potential hijack of consoles where the user is still logged in. Only Nova setups with vnc or spice enabled are affected.

[Dave McCowan (dave-mccowan)]

@tristan: yes to both. when I said "noVNC" I meant "websocketproxy". i will work to amend the patch to check the proto. I am hoping a nova guru will help me design the fix to give websockproxy access to the access_url.

[Andrew Laski (alaski)]

access_url = '%s?token=%s' % (CONF.novncproxy_base_url, token), so really what you need is the CONF.novncproxy_base_url setting.

You can add "from oslo_config import cfg" and then set "CONF = cfg.CONF" near where LOG is set. There are plenty of examples of how that is done in other modules. That should provide you with access to the setting you need. A caveat is that it requires that the config file on the websocketproxy server has that option set, which is not guaranteed since that option was not necessary on those hosts before.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks alaski for the quick guidance!
In order to not force a new requirement, we can try to load that configuration and use it when possible... Then we can amend the OSSA with a note saying that the websocketproxy server needs access to nova.conf in order to have full protection...

And just to be sure, xvpvncproxy_base_url and html5proxy_base_url are out of scope here right ?

[Dave McCowan (dave-mccowan)]

I was hoping to pull the value that in generated since it can come from novncproxy_, or spice_, or maybe something else in the future.

[Andrew Laski (alaski)]

The generated value is not persisted anywhere. And if we wanted to look at persisting it it would need to be shared storage since it's generated on the compute host.

We could do something like min_protocol(novncproxy_, spice_, etc...) where http < https and enforce that the request matches at least the minimum set. Beyond that I'm out of ideas for the moment since communicating the access_url that was decided will require a communication channel between nova-compute and websocketproxy that we don't currently have.

[Dave McCowan (dave-mccowan)]

I've got a prototype working. I added 'access_url' to the dict that is generated by authorize_console() and returned by check_token(). so, websocketproxy has easy access to the 'access_url'. the change ripples through console/, compute/, and consoleauth/ but is pretty straightforward. i'll post a patch shortly for feedback.

[Dave McCowan (dave-mccowan)]

Example code verifying origin protocol vs. access_url protocol.

[Andrew Laski (alaski)]

This looks the same as the previous patch to me... Perhaps you did not commit the changes yet?

[Dave McCowan (dave-mccowan)]

Now with real changes. :-)

[Paul McMillan (paul-mcmillan)]

This approach looks correct to me. The only other thing I'd do is add an assertion that 'origin_parsed.netloc' is not empty - there are some edge cases that can confuse urlparse, and I'd prefer to categorically avoid them.

[Andrew Laski (alaski)]

Nice approach. There are some drawbacks with this that make backporting tricky. You've updated an rpc call here which means the version needs to be bumped. I will need to refresh myself on how this has been done on backports in the past. Additionally you can't assume that access_url is in connect_info as it's possible that a token was set before the upgrade, or that consoleauth has not been upgraded yet. So some additional error handling will be needed.

[Tristan Cacqueray (tristan-cacqueray)]

Some tests are failing with the last patch:
* AttributeError: No values given for arguments: access_url
nova.tests.unit.compute.test_compute.ComputeAPITestCase.test_*_console

* KeyError: 'access_url'
nova.tests.unit.console.test_websocketproxy.NovaProxyRequestHandlerBaseTestCase.test_new_websocket_client_py273_good_scheme

* TypeError: authorize_console() takes exactly 9 arguments (8 given)
nova.tests.unit.consoleauth.test_consoleauth.CellsConsoleauthTestCase.test_*

Finally a flake8 error:
./nova/console/websocketproxy.py:84:9: F841 local variable 'origin_scheme' is assigned to but never used

[Dave McCowan (dave-mccowan)]

This patch adds unit test and error checking and is off kilo/master.
@andrew, please let me know what you want to do with the RPC version.
I made the code in websocketproxy backwards compatible (silently skipping the check if access_url is not supplied).
I have icehouse and juno patches in progress.

[Jeremy Stanley (fungi)]

It's also necessary to determine whether increasing the RPC version is at all suitable in stable branches, or whether we just have to document the risk there and leave that part of the fix unimplemented.

[Paul McMillan (paul-mcmillan)]

We need to provide some way to fix the stable branches as well. If we can't increment the RPC version, adding an optional configuration would be an appropriate backwards-compatible choice.

[Dave McCowan (dave-mccowan)]

New patch set is complete, except for RPC version decision and update.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks Dave for the new patch set!

I've subscribed apevec and adam_g as Nova stable maintainer. So we are looking at a RPC version bump in order to have stable properly fixed.

If this is not possible, we still have the option to triage the mitm case as a B1/B2 class of bug ( https://wiki.openstack.org/wiki/Vulnerability_Management#Incident_report_taxonomy ) and eventually provides a more simple patch that would make sure origin scheme is https.

[Tristan Cacqueray (tristan-cacqueray)]

@nova-coresec, can you please review the last proposed patch ?

@stable-maintainer: what are the odds of an RPC version increase to be accepted in order to support that new required parameter ?

Else any chance to support a more simple fix for stable only that would introduce a "enforce_ssl_origin" option to be activated by cloud administrator in order to protect stable deployment against the mitm hijack ?
This solution wouldn't need to be supported in master as the proposed fix auto discover that settings...

[Tony Breeds (o-tony)]

I'm not sure what the problem is but I keep getting 'Processing Failed' when I try to view any of the patches.

[Thierry Carrez (ttx)]

@Tony: looks like a transient Launchpad issue. Seems to work now.

[Tony Breeds (o-tony)]

@ttx something strange is happening. I removed the "patch" flag for 1409142-master-kilo.patch and I was able to view it.

[Dave McCowan (dave-mccowan)]

@alaski, @stable-maintainer: I need the answer to the question: Can we change the RPC versions of the consoleauth API in Icehouse and Juno? If yes, I'll update the patches with version number bumps. If no, I'll update the patches with a new config parameter.

[Andrew Laski (alaski)]

In general the answer seems to be no. https://wiki.openstack.org/wiki/StableBranch#Appropriate_Fixes

If the vulnerability was of a severe enough nature and that was the only way to address it we could work out a way to do it, but for this I think you'll need to go with a config parameter.

[Tony Breeds (o-tony)]

@Dave: I'm neither @alaski nor @stable-maintainer but I think the config option is better than an RPC bump.

It can be done is a backwards compatible way. The RPC bump is neater but I think it's off the table.

If you'd rather wait for @alaski, @stable-maintainer to reply then that's fine.

[Thierry Carrez (ttx)]

With my stable hat on: if there is a backwards compatible way, then it should be implemented that way for stable.

[Dave McCowan (dave-mccowan)]

Looking at the VNC configuration options, there is already an "ssl_only" configuration option to disallow non-encrypted connections. I can piggy-back on that command, and also disallow origin headers that are not HTTPS. That's an easy enough fix, that I'm tempted to use it for Kilo too. No RPC API changes for any release. Thoughts?

[Tony Breeds (o-tony)]

I think a new option would be better. I concede that the 2 concepts are related I think borrowing the existing option may cause surprise to users that have stab;e deployed and that option is active and yet don't care about matching origin headers.

I guess it boils down to principle of least surprise.

just my $0.02.

[Tristan Cacqueray (tristan-cacqueray)]

Using the "ssl_only" option sounds good to me, it is documented as 'Disallow non-encrypted connections' and could be used to make sure origin is using an encrypted connections. On the other hand, Tony is right and this may cause surprise...

@Paul if we go with the new option to enforce origin is https for all branch, would it also deter the dns rebinding issue ? And if not, in case we fix it in another patch, would this option be compatible or will it be rewritten to support a whitelist mechanism ? Just making sure we are looking at a long-term solution....

[Dave McCowan (dave-mccowan)]

Looking more, I don't think the ssl_only option is the best choice. Customers may not have set this, but still use HTTPS. I implemented @alaski's idea in #49, and decided to ignore my concerns in #51. Patches are attached. Changing the RPC API at this point seems overly complex for a value that can be gleaned from the config file. These patches will work in all cases, except for when someone configures both NoVNCProxy and Spice, but only one of them with HTTPS. (The insecure one will fail to validate.)

[Tristan Cacqueray (tristan-cacqueray)]

@Dave: patch for kilo fail to apply with current master, imports and tests needs a little bit of adjustment

[Tristan Cacqueray (tristan-cacqueray)]

And beside a bunch of flake8 errors, run_tests succeed on all patch!
@paul and @nova-coresec please review proposed solution...

[Dave McCowan (dave-mccowan)]

New patches fix all the flake8 errors.

[Paul McMillan (paul-mcmillan)]

Sorry for the slow reply, I've been away.

@Dave I don't see why the latest incarnation needs to check the host header. If the user puts in their expected base url, we can directly use that and incidentally also solve the dns rebinding issue at the same time, since the host is provided by the user.

That said, the attached backport patches do appear to be correct, and I will not block their release based on this issue.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks for the feedback Paul!

Well if there is an easy way to deter dns rebinding in the same patch, that would be ideal. Though let's not block on that since this is already overdue.

@nova-coresec please review the last proposed patch, if it's approved, we could send the follow up next monday with a new disclosure date at next thursday...

[Andrew Laski (alaski)]

I have one concern with the patches. It requires https if either base url uses https. So a legitimate http novnc request could be failed if the spice base url uses https. I think this needs to go the other way and require that the protocol matches or exceeds the lesser of the protocols where http < https.

[Paul McMillan (paul-mcmillan)]

@Andrew As long as there aren't default values that cause it to always default to http, I'm ok with that. I don't like it, but... this needs to get out the door.

[Andrew Laski (alaski)]

Unfortunately the defaults are all http://127.0.0.1. But I don't think it's reasonable to break the websocket proxy for someone because they chose to use http for one type and https for another. We could look at deprecating that usage, but it shouldn't just be broken outright.

[Dave McCowan (dave-mccowan)]

That works for me. I'll have new patches tested and uploaded late tonight (US EST). Can reviewers check in late tonight or tomorrow for comments/approvals to keep Tristan's suggested timeline?

[Dave McCowan (dave-mccowan)]

Good news: Content_type is part of the connect_info data that websocketproxy receives, so we can check the protocol for vnc and spice separately.

[Thierry Carrez (ttx)]

@nova-coresec, please give proposed patches a last round of in-bug review

[Andrew Laski (alaski)]

The master patch doesn't apply. It appears to be a patch to your previous patch.

The Icehouse and Juno patches look good. I wouldn't hold up the patch for it but we're trying to phase out the use of audit as a log level so that added log line would be better as a warning.

[Thomas Goirand (thomas-goirand)]

Hi. Since I already have a bug opened against the NoVNC package [1], the security team and the release team of Debian are getting nervous that this issue isn't addressed in Debian. Can I already upload the Nova package with the above patch? Does it requires the patch from NoVNC available here [2] ?

Please let me know ASAP,

Cheers,

Thomas Goirand (zigo)

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=778618

[2] https://github.com/kanaka/noVNC/commit/ad941faddead705cd611921730054767a0b32dcd

[Tristan Cacqueray (tristan-cacqueray)]

Thomas, the NoVNC fix is standalone and not dependent of this bug.

Please do not use the above patch as they have not been reviewed/approved yet. Once we are confident it fixes issues mentioned here, we'll issue a follow up on the pre-OSSA with a new disclosure date.

If it's good by Thursday, the new disclosure date would be:
2015-03-10, 1500UTC

Thanks in advance for your patience.

[Paul McMillan (paul-mcmillan)]

@Thomas: As Tristan said, and as I have reiterated multiple times on the debian bug, the issue with novnc is separate from this one. I've added you to this bug https://bugs.launchpad.net/nova/+bug/1420942 which tracked that separate, independent bug (it is a bug in novnc, it is not a nova bug, and not an openstack bug, but still private until we can get the OSSN sent out).

Thank you for proposing the fix for noVNC into Debian, it will be easier to work upstream from there.

[Dave McCowan (dave-mccowan)]

@andrew. I attached a working master-kilo patch. in this version I removed the LOG altogether. Please let me know the convention for should-not-occur exceptions for each release (no log, log warning, log audit, ...), and I'll update the patches as appropriate.

[Andrew Laski (alaski)]

I'm not seeing an updated patch...

There are no hard rules right now for what log level should be used, but we try to adhere to standard expectations for what should be logged at each level. Audit is something made up in Nova and is not a standard logging level so we've been moving away from it. But in this case the log level would be triggered by a user requesting a console type that isn't handled here, though I'm not sure the request would make it here if that actually happened. Since it could be considered a user error rather than a system error a warning would be more appropriate than an exception in my opinion.

[Dave McCowan (dave-mccowan)]

I uploaded the correct patch uploaded this time. I miss Gerrit. :-)
If we consider console-type as part of the request, then any error (bad-token, bad-headers, bad-access-path, or bad-conosle-type) should all behave the same. Based on the pre-existing code, in each case they should raise an exception and not make a separate logging call.

[Andrew Laski (alaski)]

Somehow you have a change in nova/scheduler/filter_scheduler.py which got pulled in. But that's already on master so it wouldn't show up if pushed up to gerrit, or would cause a conflict. Outside of that though, I'm +2 on these.

[Tristan Cacqueray (tristan-cacqueray)]

@alaski I'm afraid I can't find that filter_scheduler.py change in the proposed patches. The backlog being quite long here doesn't help... , can you please confirm the three current patchs (proposed in comment #97, #98 and #108) are good ?

[Andrew Laski (alaski)]

Weird... I thought I applied that patch to a clean master branch but there was an extraneous scheduler change after it applied, but I'm not seeing it here.

I'm +2 on the three patches.

[Paul McMillan (paul-mcmillan)]

Just so we're clear here, in order to make this work properly, I'm going to have to specify both a novnc base url and a spice base url, even if I don't use one or the other?

[Paul McMillan (paul-mcmillan)]

Ah no, I didn't finish reading the latest patch. I like this approach better.

[Dave McCowan (dave-mccowan)]

@Paul, yes happily you're right at #115, we ended up with a clean solution. You we're also right at #114, we did briefly talk about a solution that would have had that horrible requirement. :-)

[Tristan Cacqueray (tristan-cacqueray)]

Last batch of patches fixes flake8 errors. I guess we are good to go now!

Proposed disclosure date:
2015-03-10, 1500UTC

[Tony Breeds (o-tony)]

The patches look good to me. +1

[Paul McMillan (paul-mcmillan)]

I applied this patch today to our system, and ran into trouble. Apparently the console type is 'novnc' instead of the 'vnc' that the patch looks for. I got this error:

 94: handler exception: Invalid Console Type for WebSocketProxy: 'novnc'
 94: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/websockify/websocket.py", line 711, in top_new_client
    self.new_client()
  File "/usr/lib/python2.7/dist-packages/nova/console/websocketproxy.py", line 94, in new_client
    origin.scheme):
  File "/usr/lib/python2.7/dist-packages/nova/console/websocketproxy.py", line 60, in verify_origin_proto
    raise exception.ValidationError(detail=detail)
ValidationError: Invalid Console Type for WebSocketProxy: 'novnc'

and applying the attached diff to the patched installation fixed it for me. We probably need to update the patches we sent out - can someone else verify this end-to-end with their novnc system to confirm it?

[Tristan Cacqueray (tristan-cacqueray)]

I confirm, vnc connections are broken in Icehouse and Juno.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks Dave,

Can we please get yet another round of approval on the last patch ?
Note that it just change console_type 'vnc' into 'novnc'.

[Paul McMillan (paul-mcmillan)]

This latest round of patches looks correct to me

[Tristan Cacqueray (tristan-cacqueray)]

Manual VNC connection on Icehouse and Juno worked correctly here on devstacks. Can someone else confirm the last round patch works properly before we send another follow-up ?

Also, does someone have an objection if we keep the last proposed disclosure date (2015-03-10, 1500UTC) ?

[Thierry Carrez (ttx)]

Fine with keeping same date

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/163033

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/163034

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/163035

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/163604

[Tristan Cacqueray (tristan-cacqueray)]

So two other issues have been found with the proposed patch:

1/ out of browser client that does not set the Origin header won't work. So we are removing the origin check if origin is not set, assuming all vulnerable web-based request would include the Origin header.

2/ Serial console access is also broken for Juno and Kilo.

Dave is working on another version.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Dave McCowan (<email address hidden>) on branch: master
Review: https://review.openstack.org/163604
Reason: Duplicate of https://review.openstack.org/#/c/163033/

[Paul McMillan (paul-mcmillan)]

Since the Origin header is required with a MUST by the RFC for browsers, I think I'm ok with this, absent any evidence that it's possible to convince browsers to leave the origin header off.

https://tools.ietf.org/html/rfc6455#section-4.1

   8. The request MUST include a header field with the name |Origin|
        [RFC6454] if the request is coming from a browser client. If
        the connection is from a non-browser client, the request MAY
        include this header field if the semantics of that client match
        the use-case described here for browser clients. The value of
        this header field is the ASCII serialization of origin of the
        context in which the code establishing the connection is
        running. See [RFC6454] for the details of how this header field
        value is constructed.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/163033
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=fdb73a2d445971c6158a80692c6f74094fd4193a
Submitter: Jenkins
Branch: master

commit fdb73a2d445971c6158a80692c6f74094fd4193a
Author: Dave McCowan <email address hidden>
Date: Mon Mar 2 15:00:22 2015 -0500

    Websocket Proxy should verify Origin header

    If the Origin HTTP header passed in the WebSocket handshake does
    not match the host, this could indicate an attempt at a
    cross-site attack. This commit adds a check to verify
    the origin matches the host.

    SecurityImpact

    Change-Id: Ica6ec23d6f69a236657d5ba0c3f51b693c633649
    Closes-Bug: 1409142

[Paul McMillan (paul-mcmillan)]

The fix merged to latest looks good to me.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/163035
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=0ff674217024a59a79bf80ab752a5cbf7e94642c
Submitter: Jenkins
Branch: stable/icehouse

commit 0ff674217024a59a79bf80ab752a5cbf7e94642c
Author: Dave McCowan <email address hidden>
Date: Tue Feb 24 21:33:58 2015 -0500

    Websocket Proxy should verify Origin header

    If the Origin HTTP header passed in the WebSocket handshake does
    not match the host, this could indicate an attempt at a
    cross-site attack. This commit adds a check to verify
    the origin matches the host.

    Change-Id: Ica6ec23d6f69a236657d5ba0c3f51b693c633649
    Closes-Bug: 1409142

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/163034
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=676ba7bbc788a528b0fe4c87c1c4bf94b4bb6eb1
Submitter: Jenkins
Branch: stable/juno

commit 676ba7bbc788a528b0fe4c87c1c4bf94b4bb6eb1
Author: Dave McCowan <email address hidden>
Date: Tue Feb 24 21:35:48 2015 -0500

    Websocket Proxy should verify Origin header

    If the Origin HTTP header passed in the WebSocket handshake does
    not match the host, this could indicate an attempt at a
    cross-site attack. This commit adds a check to verify
    the origin matches the host.

    Change-Id: Ica6ec23d6f69a236657d5ba0c3f51b693c633649
    Closes-Bug: 1409142

[Yukihiro KAWADA (warp-kawada)]

Is this patch valid for console_type = 'serial'?

novaconsole kwd_vm_123
this cli fails.

origin:http expected:ws

[Lars Kellogg-Stedman (larsks)]

You will find yourself here if your formerly-working console connections have suddenly started failing with "ValidationError: Origin header protocol does not match this host". This is because the changes introduced to resolve this issue make the value of "novncproxy_base_url" important on the host running nova-novncproxy, whereas previously this value was unused except by nova-compute.

The default value for novncproxy_base_url uses an "http://" url, which means that any https:// connections to your console will be rejected until you update this configuration option and restart nova-novncproxy.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/169752

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/169753

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/169752
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=2ffcf18d00eff6fb0777769469c4aa5ac7bbb6c9
Submitter: Jenkins
Branch: master

commit 2ffcf18d00eff6fb0777769469c4aa5ac7bbb6c9
Author: Nikola Dipanov <email address hidden>
Date: Wed Apr 1 14:35:13 2015 +0100

    consoleauth: Store access_url on token authorization

    Related-bug: 1409142

    As part of the fix for the related bug - we've added protocol checking
    to mitigate MITM attacks, however we base protocol checking on a config
    option that is normally only intended for compute hosts.

    This is quite user hostile, as it is now important that all nodes
    running compute and proxy services have this option in sync.

    We can do better than that - we can persist the URL the client is
    expected to use, and once we get it back on token validation, we can
    make sure that the request is using the intended protocol, mitigating
    the MITM injected script attacks.

    This patch makes sure that the access_url is persisted with the token -
    the follow-up patch makes consoles use that info.

    Change-Id: I02a377f54de46536ca35413b615d3298967afc33

[Nikola Đipanov (ndipanov)]

It is also worth noting here for future reference that the above 2 patches:

https://review.openstack.org/169752
https://review.openstack.org/169753

are relevant to Kilo only (they will not be backported to previous releases). In Kilo it will no longer be needed to keep the novncproxy_base_url options in sync between compute and novncproxy services. This is because the URL a token was authorized for will be kept alongside other authorization info.

However there is a caveat to the way upgrades need to be done for Juno to Kilo (it copied here from the original commit message):

UpgradeImpact: Websocket proxies need to be upgraded in a lockstep
with the API nodes up to this commit (or when upgrading to Kilo),
as older API nodes will not be sending the access_url when authorizing
console access, and newer proxy services (this commit and onward) would
fail to authorize such requests.

[Rui Yuan Dou (ry-dou)]

I tried the Juno patch in https://review.openstack.org/#/c/163034/ , and it did not really resolved this problem. I can still include the VNC page in a different domain.
The patch try to read a http header "Origin" and compare it with the page url, but it doesen't exist in the http headers of get request, so it wont actually block the Cross-Site visit. Can anyone verify it?

[Grant Murphy (gmurphy)]

I think this might be mentioned in a comment of:

https://review.openstack.org/#/c/163034/3/nova/console/websocketproxy.py

".. the Origin header is set by browser and it is set to the location of the script that makes the request. It's a mandatory field for browser but not relevant for command line tools. More information here: https://tools.ietf.org/html/rfc6455#section-10.2"

How are you making the GET request?

[Rui Yuan Dou (ry-dou)]

I just include the web console url in a iframe item of my localhost web server page, and the console still can be shown in that page, and there's no such "Origin" header exists.

[Abel Lopez (al592b)]

This is still present after this patch. In our testing, we're able to get consoles for the same instance on different hosts.

[Grant Murphy (gmurphy)]

I've opened https://bugs.launchpad.net/ossa/+bug/1511541 to track this as an incomplete fix. @al592b, @rydoc if you can attach your test case to that bug it might help expedite the process.

[Dave McCowan (dave-mccowan)]

@rydou: What browser did you use that did not include an Origin header? @Abel: Getting consoles for the same instance from different hosts is not the vulnerability. The vulnerability is getting access to the same instance from the same browser, but using websocket code downloaded from a different server (hijacking the existing connection). I'll follow up with you offline.