User gets access to VNC console of an instance of another tenant

[Rohit Karajgi]

A user is able to access the VNC console of an instance of another tenant in the following scenario:

1. User(U1) of Tenant(T1) creates an instance(I1), which uses port 5900 for VNC.
2. User(U1) gets the URL for accessing his instance's(I1) VNC console.
3. User(U1) terminates his VM (libvirt frees port 5900).

4. User(U2) of Tenant(T2) creates an instance(I2), which also gets free port 5900 for VNC.
5. Now, User(U1) attempts to access his instance's(I1) VNC console using his URL (from Step2).

Expected result: User(U1) should not be able to access VNC Console and should see "Failed to connect to server (code: 1006)"

Actual result: User(U1) is able to access VNC Console of instance(I2) of User(U2), belonging to Tenant(T2) as it runs on the same VNC port of the same compute host.

This issue is reproducible within the token time-to-live of 600 seconds(default).

[Unmesh Gurjar (unmesh-gurjar)]

Gerrit review: https://review.openstack.org/#/c/13828/ addresses the issue (for Nova's memorycache implementation).

[Unmesh Gurjar (unmesh-gurjar)]

The above fix fetches all the tokens (from nova.common.memorycache) based on the 'host' and 'port' parameters (which are essentially values of token key).
However, it won't work for memcache based storage backend, since there is no mechanism/API to retrieve tokens based on value.
IMO we need to extend the 'token' storage strategy, by maintaining a list of tokens for each instance (in addition to the token key).

For example:
Current implementation:
'token1': {'token': 'token1', 'host': 'localhost', 'port': 1234, ...},
'token2': {'token': 'token2', 'host': 'localhost', 'port': 1234, ...},
'token3': {'token': 'token3', 'host': 'localhost', 'port': 5678, ...}

Proposed implementation
'token1': {'token': 'token1', 'host': 'localhost', 'port': 1234, ...},
'token2': {'token': 'token2', 'host': 'localhost', 'port': 1234, ...},
'token3': {'token': 'token3', 'host': 'localhost', 'port': 5678, ...},
#add the following instance-token store
'ins001': ['token1', 'token2'],
'ins002': ['token3']

This list would facilitate fetching tokens to be deleted (independent of the backend being used).
If this solution looks good, I will propose it in another patch set.

[Unmesh Gurjar (unmesh-gurjar)]

On reaching the TTL, memcache would delete that particular token, however, the token would still exist in instance-specific-tokens list.
Due to this, there is a possibility of having a dangling token in the instance-specific-tokens list (which is a drawback of the above proposed solution).

[Thierry Carrez (ttx)]

@Unmesh: would be good to embargo this fix so we can coordinate the announcement -- could you remove the public review at this point ? (abandon, or turn to draft). You can attach the patch to this bug and we'll review it here.

[Thierry Carrez (ttx)]

Adding nova-core as we'll need fast help now that the beans have almost been spilled.

[Unmesh Gurjar (unmesh-gurjar)]

@Thierry: Sorry for causing this issue. I have abandoned the review. Attaching the patch set that fixes the issue for Nova's memorycache backend. However, as pointed out in comments #2 and #3 above, need to implement a solution that works for any backend.

[Thierry Carrez (ttx)]

@Unmesh: are you working on a patch that would address all backends ?

[Unmesh Gurjar (unmesh-gurjar)]

@Thierry: I am not working on this patch right now.
I am waiting for comments/response to the design proposed in #2 above. In #3, I have mentioned a drawback of this (#2) approach.

[Thierry Carrez (ttx)]

@Nova-Core: please comment on design proposed in #2 above.

[Vish Ishaya (vishvananda)]

#2 looks good to me. I added anthony young as he was the original author to see if he thinks it is reasonable.

[Anthony Young (sleepsonthefloor)]

comment #2 seems reasonable to me as well.

[Thierry Carrez (ttx)]

@Unmesh: looks like comment #2 approach is OK, please propose another patch set (or let us know if you need help in that)

[Unmesh Gurjar (unmesh-gurjar)]

Attaching the patch that implements the above solution.

[Thierry Carrez (ttx)]

@Nova-Core please review proposed patch and comment on this bug

[Vish Ishaya (vishvananda)]

+1 I feel like this should ultimately be in done in compute.api instead of compute.manager, but we can move it up along with other stuff later.

[Thierry Carrez (ttx)]

* Another +1 needed on the patch at comment #13
* Essex is obviously affected, but for Diablo didn't use the same code... Anyone with an opinion on that ?
* Also please review proposed impact description:

Title: Potential unauthorized VNC console access
Reporter: Rohit Karajgi (NTT Data)
Products: Nova
Affects: Essex, Folsom, Grizzly [?]

Description:
Rohit Karajgi from NTT Data reported a vulnerability in Nova's authorization of VNC console access. Due to an issue in console token invalidation on instance deletion or migration, an authenticated user may retain access to a VNC port that was reallocated to another user, potentially allowing him to access another instance VNC console. Only setups allowing access to instances through VNC consoles are affected.

[Vish Ishaya (vishvananda)]

impact report looks good

[Thierry Carrez (ttx)]

@Nova-core please review proposed patch (comment #13)

[Russell Bryant (russellb)]

This seems like an odd use of a decorator. Why not just a function that gets called by the two methods that need it? It's not really wrapping anything. It's just running some code before calling the real function.

[Thierry Carrez (ttx)]

Hmm, we really need some progress here. Unmesh, could you address Russell's comments ?

[Unmesh Gurjar (unmesh-gurjar)]

Attached new patch set addressing the review comments.

[Unmesh Gurjar (unmesh-gurjar)]

I have fixed an issue in the previous patch set (error while deleting tokens for an instance for which there are no tokens associated).
Please review this new patch.

[Thierry Carrez (ttx)]

nova-core please review

[Russell Bryant (russellb)]

so close ...in 1058077_patch3.diff you have:

+ def delete_invalid_token(self, ctxt, instance_uuid):
+ topic = self.make_msg('delete_invalid_token',
+ instance_uuid=instance_uuid)
+ return self.call(ctxt, topic, version='1.1')

I don't think topic makes sense as the variable name, because it's the message being sent, not the topic. If you change the variable name to something else (msg works for me), then I'm +2 on it.

[Russell Bryant (russellb)]

I would post an updated patch, but the patch isn't in the form that makes it trivial to import and retain all meta-data ... when you post an update, please export the patch using "git format-patch".

[Unmesh Gurjar (unmesh-gurjar)]

New patch addressing above review comments. Please review this patch.

[Thierry Carrez (ttx)]

@nova-core: new review round!

[Vish Ishaya (vishvananda)]

+1

[Russell Bryant (russellb)]

Patch looks good to me, as well.

[Russell Bryant (russellb)]

Added openstack-stable-maint so that they know this is coming down the pipe.

[Michael Still (mikal)]

If I am reading this correctly, 0001-Delete-VNC-token-after-server-deletion.patch is the only patch needed to resolve this problem on Folsom? Unfortunately, that patch doesn't apply cleanly to master for me.

$ patch -p 1 < ~/openstack/vuln/1058077-folsom-patch
patching file nova/common/memorycache.py
patching file nova/compute/api.py
Hunk #1 succeeded at 1924 (offset 30 lines).
patching file nova/compute/manager.py
Hunk #1 FAILED at 54.
Hunk #2 FAILED at 300.
Hunk #3 succeeded at 1060 (offset 30 lines).
Hunk #4 succeeded at 2536 (offset 43 lines).
2 out of 4 hunks FAILED -- saving rejects to file nova/compute/manager.py.rej
patching file nova/consoleauth/manager.py
Hunk #1 succeeded at 45 (offset -1 lines).
Hunk #2 succeeded at 58 (offset -1 lines).
Hunk #3 succeeded at 68 with fuzz 2 (offset -1 lines).
Hunk #4 succeeded at 87 (offset -1 lines).
patching file nova/consoleauth/rpcapi.py
Hunk #1 succeeded at 30 (offset -1 lines).
Hunk #2 succeeded at 50 (offset -1 lines).

I'll take a look at this.

[Michael Still (mikal)]

The forward port wasn't too bad it turns out. There's the new folsom patch. I'd appreciate it if someone could eyeball it for errors.

[Michael Still (mikal)]

Sigh, ignore me. That patch above is for grizzly. Here are some new ones.

[Michael Still (mikal)]

And Folsom.

[Michael Still (mikal)]

@Unmesh -- I observe some test failures with these patches. I'd appreciate it if you could take a look. Thanks.

[Unmesh Gurjar (unmesh-gurjar)]

@Michael: I have fixed the failing unit tests, attaching the patch set of unit tests for grizzly.

[Thierry Carrez (ttx)]

@Mikal: does the new patch work for you ?

[Michael Still (mikal)]

Sorry, I was out of the office for a few days. Yep, the grizzly patch + unit tests works for me. Here's a patch of those two rolled together...

[Michael Still (mikal)]

Here's a backport to Folsom which also passes unit tests.

[Michael Still (mikal)]

Essex will require further backporting work, and I don't have time right now. I will take another look tomorrow.

[Unmesh Gurjar (unmesh-gurjar)]

Is any action required at my end? Should I submit a Gerrit review request for this fix?

[Thierry Carrez (ttx)]

@Unmesh: no further action. We now just need to get all the patches lined up and ready, communicate them to downstream stakeholders and prepare an advisory.

FYI we follow the process described at http://wiki.openstack.org/VulnerabilityManagement
We are currently waiting for a patch backport on Essex.

[Thierry Carrez (ttx)]

We need a volunteer to take on the Essex patch again. Adding the stable-essex crew for help.

[Unmesh Gurjar (unmesh-gurjar)]

Attaching the patch set for stable/essex.

[Thierry Carrez (ttx)]

Thanks Unmesh ! Nova-core, please review.

[Thierry Carrez (ttx)]

Nova-core, please review proposed patches !

[Mark McLoughlin (markmc)]

Don't tokens need to be deleted on resize too? How about hard reboot, can the port number change with that?

[Tushar Patil (tpatil)]

Yes, the port number might change as they are managed by libvirt (Refer to any libvirt.xml of an instance, you will notice autoport is set to yes in the graphics xml element.
For example,
 <graphics type="vnc" autoport="yes" keymap="en-us" listen="127.0.0.1"/> )

IMO, after you call reboot/resize/live migration the tokens referenced by the instance should be deleted otherwise we are back again to the square one.

[Rohan (kanaderohan)]

Attached patch for grizzly after implementing issues pointed out by Tushar Patil https://bugs.launchpad.net/nova/+bug/1058077/comments/49

Now tokens will be deleted for an instance whenever it is rebooted, resized, revert-resize, confirm-resize, stop-instance, _delete_evacuated_instance, terminate_instance, live_migration, rebuild instance, soft delete instance.

[Rohan (kanaderohan)]

Attached patch for stable/folsom with unit tests after implementing issues pointed out by Tushar Patil https://bugs.launchpad.net/nova/+bug/1058077/comments/49

Now tokens will be deleted for an instance whenever it is rebooted, resized, revert-resize, confirm-resize, stop-instance, terminate_instance, live_migration, rebuild instance.

[Rohan (kanaderohan)]

Attached patch for stable/essex with unit tests after implementing issues pointed out by Tushar Patil https://bugs.launchpad.net/nova/+bug/1058077/comments/49

Now tokens will be deleted for an instance whenever it is rebooted, resized, revert-resize, confirm-resize, stop-instance, terminate_instance, live_migration, rebuild instance.

[Rohan (kanaderohan)]

Also, can someone with know how about spice console confirm if the token/port collision doesnt give unauthorized access to the instance's spice console?

[Vish Ishaya (vishvananda)]

added daniel berrange to comment on spice.

[Daniel Berrange (berrange)]

The SPICE port allocation & client authorization works in exactly the same way as the VNC port allocation / authorization. So any vulnerability Nova has wrt to VNC access, will equally apply to SPICE. Likewise any approach to fixing it for VNC will also apply to SPICE.

[Tushar Patil (tpatil)]

Daniel : Just wanted to check if you are going to fix VNC access issue in spice?

[Daniel Berrange (berrange)]

My assumption is that whoever is writing the patches should cover both VNC and SPICE in their patch. It should not require them to have any particular knowledge of SPICE, since the code was designed to explicitly follow exactly the same design pattern as the existing OpenStack VNC code & has the same test coverage.

[Rohan (kanaderohan)]

Attached patch for including spice console fixes and tests + vnc console fixes and tests for grizzly.

Patches for stable/folsom, stable/essex remain the same (1058077_folsom_latest_29_jan.patch , 1058077_essex_latest_29_jan.patch)

[Rohan (kanaderohan)]

Nova-core, please review proposed patches.

[Rafi Khardalian (rkhardalian)]

There's a public review around this now, which showed up a few days ago:

https://review.openstack.org/22086

[Thierry Carrez (ttx)]

Independently (and publicly) reported as bug 1125378

[Tushar Patil (tpatil)]

Thierry: Just curious to know why this bug is marked as duplicate?