CVE-2018-25022: Restrict packet kinds that can be sent through onion path. by kurnevsky · Pull Request #872 · TokTok/c-toxcore

Conversation

This change is 

All committers have signed the CLA.

I think it’s a good idea to limit onion packets to only a restricted set.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

toxcore/onion.c, line 474 at r1 (raw file):

}

if (len <= SIZE\_IPPORT) {

Since there’s a check for the exact length above, I don’t think this is necessary.

toxcore/onion.c, line 478 at r1 (raw file):

}

if (plain\[SIZE\_IPPORT\] != NET\_PACKET\_ANNOUNCE\_REQUEST &&

In my opinion it would be better to write pack/unpack functions like ipport_pack/unpack instead of modifying the plain packet structure.

Comments from Reviewable

Copy link

kpp commented Apr 15, 2018

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

toxcore/onion.c, line 478 at r1 (raw file):

Previously, sudden6 wrote…

In my opinion it would be better to write pack/unpack functions like ipport_pack/unpack instead of modifying the plain packet structure.

Right. Let’s wait for @kurnevsky to help you with this code for a month while you will be stopping him from fixing 0-day vulnerability.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

toxcore/onion.c, line 478 at r1 (raw file):

Previously, kpp (Roman) wrote…

Right. Let’s wait for @kurnevsky to help you with this code for a month while you will be stopping him from fixing 0-day vulnerability.

? I think a proper fix is better than possibly introducing new problems with a bandaid fix.

Comments from Reviewable

@sudden6 merge this asap and release a new version. Then work on a clean fix. Dont let a flaw in the wild just because of review.

toxcore/onion.c, line 474 at r1 (raw file):

Previously, sudden6 wrote…

Since there’s a check for the exact length above, I don’t think this is necessary.

Above we check only that length of decrypted data is the same as length of encrypted minus mac bytes. There is check “length <= 1 + SEND_3” yet above, but it checks only that packet has enough length for onion return.

Comments from Reviewable

toxcore/onion.c, line 478 at r1 (raw file):

Previously, sudden6 wrote…

? I think a proper fix is better than possibly introducing new problems with a bandaid fix.

It’s the way the whole c-toxcore implemented - it doesn’t parse packets to structs, it just works with bytes. So I suggest not to refactor it here - refactoring will bring much bigger diff.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

toxcore/onion.c, line 478 at r1 (raw file):

Previously, kurnevsky (Evgeny Kurnevsky) wrote…

It’s the way the whole c-toxcore implemented - it doesn’t parse packets to structs, it just works with bytes. So I suggest not to refactor it here - refactoring will bring much bigger diff.

This code needs lots of refactoring later. I think we can defer it for now given the relative urgency.

Comments from Reviewable

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

toxcore/onion.c, line 474 at r1 (raw file):

Previously, kurnevsky (Evgeny Kurnevsky) wrote…

Above we check only that length of decrypted data is the same as length of encrypted minus mac bytes. There is check “length <= 1 + SEND_3” yet above, but it checks only that packet has enough length for onion return.

ok, didn’t see this, maybe that could be an issue in other functions too, but no need to fix that in this PR

toxcore/onion.c, line 478 at r1 (raw file):

Previously, iphydf wrote…

This code needs lots of refactoring later. I think we can defer it for now given the relative urgency.

ok for me then

Comments from Reviewable

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@iphydf Can this be merged asap plz? Then please release the 0.2.2 in emergency. Now that the flaw is disclosed, it needs to be shiped the fastest we can. Persistent groupchats can wait 0.2.3 so that this can be fixed. And clients can update their static builds.

does this mean all running nodes (including bootstrap nodes and echobot and groupbot) need to be updated aswell to fix this vulnerability?
or can old nodes be tolerated in the wild?

If I understood this correctly, old nodes have to be updated in order to be protected, but will still work.

@zoff99 old nodes have to be updated, but it will be enough to have only one updated node in onion path (3 random nodes) to be protected. Unfortunately it’s not enough to update only your node to be protected yourself.

if more serious vulnerabilities may be found in the future, we should think about a mechanism to shut out vulnerable nodes.

Sorry, accidently closed pr.
@zoff99 right, it’d be nice to have a way to get node version for this purpose.

Review is very important just as with any other code, so let’s not forget about that please. In the future we should think of a way for people to disclose vulnerabilities in a more secure way that doesn’t put users in danger and doesn’t cause everyone to panic and think of not reviewing the code. I’m not criticizing anyone here. I know that in this situation you are all doing the best you can :). It’s just something that we should try to do better in the future.

Copy link

kpp commented Apr 16, 2018

It is possible to send BootstrapInfo to all public nodes. There is a u32 as integer version of toxcore lib.

@iphydf how can we make sure a malicous node (custom source code) can not use this or any future vulnerability?

Disclaimer: I’m not very familiar with the onion module, so my review is pretty useless. I can’t confirm if there is a vulnerability, or if this patch is complete or if it’s correct to limit onion routing to only two of those packet kinds. What I can say, however, is that this patch looks correct as far as C code goes and it makes sense.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

Thank you for both reporting and fixing this! :)
I’m out of time tonight, but I’ll push a release with this and #867 (if done, otherwise I’ll cherry-pick the commit that fixes with autotools build from it) tomorrow evening.

Copy link

Member

nurupo commented Apr 18, 2018 • Loading

@kurnevsky Could you explain why is it not enough to update only your node to be protected yourself?

Alright, I figured it out.

To answer my own question, the issue is that the final destination of an onion reply has no idea if the packet it received comes from an onion or not.

From the Tox spec (linking to the md file because the website doesn’t render the diagram for some reason):

peer
  -> [onion1[onion2[onion3[data]]]] -> Node A
  -> [onion2[onion3[data]]][sendbackA] -> Node B
  -> [onion3[data]][sendbackB[sendbackA]] -> Node C
  -> [data][SendbackC[sendbackB[sendbackA]]]-> Node D (end)

Node D
  -> [SendbackC[sendbackB[sendbackA]]][response] -> Node C
  -> [sendbackB[sendbackA]][response] -> Node B
  -> [sendbackA][response] -> Node A
  -> [response] -> peer

You can see that when Node D sends a reply back to peer, peer has no idea whether the packet received from Node A was sent by Node A on her own behalf or on someone else’s behalf through onion.

@nurupo you are right, that’s why I suggested to restrict packet kinds. In the future we could verify onion paths (that it has at least one updated node), but it seems there is no easy way to get version of dht node. Anyway tox network should be updated (at least partly) to add such check.

Copy link

Member

nurupo commented Apr 18, 2018 • Loading

We could look into introducing a non-backwards compatible change into the wire protocol such that updated peers wouldn’t be able to communicate with older peers, like DHT version bump or something. That way, if you are updated, you are guaranteed to be secure. Also, this would make others users aware of the update going on.