Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict packet kinds that can be sent through onion path. #872

Merged
merged 1 commit into from Apr 16, 2018

Conversation

Projects
None yet
10 participants
@kurnevsky
Copy link

kurnevsky commented Apr 15, 2018

This change is Reviewable

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Apr 15, 2018

CLA assistant check
All committers have signed the CLA.

@kurnevsky kurnevsky referenced this pull request Apr 15, 2018

Closed

Onion vulnerability #873

@kurnevsky kurnevsky force-pushed the kurnevsky:restrict_onion_packets branch from 6a2ce23 to 6b97acb Apr 15, 2018

@sudden6

This comment has been minimized.

Copy link

sudden6 commented Apr 15, 2018

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

@kpp

This comment has been minimized.

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

@sudden6

This comment has been minimized.

Copy link

sudden6 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, 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

@SkyzohKey

This comment has been minimized.

Copy link

SkyzohKey commented Apr 15, 2018

@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.

@kurnevsky

This comment has been minimized.

Copy link
Author

kurnevsky commented Apr 15, 2018

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

@kurnevsky

This comment has been minimized.

Copy link
Author

kurnevsky commented Apr 15, 2018

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

@iphydf iphydf added this to the v0.2.2 milestone Apr 15, 2018

@iphydf

This comment has been minimized.

Copy link
Member

iphydf 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, 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

@sudden6

This comment has been minimized.

Copy link

sudden6 commented Apr 15, 2018

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

@sudden6 sudden6 referenced this pull request Apr 15, 2018

Merged

Release v0.2.2 #862

@sudden6

This comment has been minimized.

Copy link

sudden6 commented Apr 15, 2018

:lgtm_strong:


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


Comments from Reviewable

@SkyzohKey

This comment has been minimized.

Copy link

SkyzohKey commented Apr 15, 2018

@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.

@zoff99

This comment has been minimized.

Copy link

zoff99 commented Apr 15, 2018

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?

@sudden6

This comment has been minimized.

Copy link

sudden6 commented Apr 15, 2018

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

@kurnevsky

This comment has been minimized.

Copy link
Author

kurnevsky commented Apr 16, 2018

@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.

@zoff99

This comment has been minimized.

Copy link

zoff99 commented Apr 16, 2018

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

@kurnevsky kurnevsky closed this Apr 16, 2018

@kurnevsky kurnevsky reopened this Apr 16, 2018

@kurnevsky

This comment has been minimized.

Copy link
Author

kurnevsky commented Apr 16, 2018

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

@tox-user

This comment has been minimized.

Copy link
Member

tox-user commented Apr 16, 2018

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.

@kpp

This comment has been minimized.

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.

@zoff99

This comment has been minimized.

Copy link

zoff99 commented Apr 16, 2018

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

@nurupo

This comment has been minimized.

Copy link
Member

nurupo commented Apr 16, 2018

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.

:lgtm_strong:


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

@robinlinden

This comment has been minimized.

Copy link
Member

robinlinden commented Apr 16, 2018

:lgtm_strong:


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

@robinlinden robinlinden merged commit 6b97acb into TokTok:master Apr 16, 2018

7 of 8 checks passed

ci/circleci CircleCI is running your tests
Details
GPG All commits have a verified GPG signature
WIP ready for review
Details
code-review/reviewable 3/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.1%) to 76.557%
Details
license/cla Contributor License Agreement is signed.
Details
@robinlinden

This comment has been minimized.

Copy link
Member

robinlinden commented Apr 16, 2018

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.

@nurupo

This comment has been minimized.

Copy link
Member

nurupo commented Apr 18, 2018

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

@nurupo

This comment has been minimized.

Copy link
Member

nurupo commented Apr 18, 2018

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.

@kurnevsky

This comment has been minimized.

Copy link
Author

kurnevsky commented Apr 18, 2018

@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.

@nurupo

This comment has been minimized.

Copy link
Member

nurupo commented Apr 18, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.