Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

DoS vector in protocol message stream #912

Closed
rade opened this issue Jun 13, 2015 · 11 comments · Fixed by #1098
Closed

DoS vector in protocol message stream #912

rade opened this issue Jun 13, 2015 · 11 comments · Fixed by #1098
Assignees
Labels
Milestone

Comments

@rade
Copy link
Member

rade commented Jun 13, 2015

I haven't tested this, but I reckon there is DoS vector in the protocol message stream that attackers can exploit without breaking our crypto.

The weave TCP protocol is a gob stream of messages: a map[string][string] for the first message, the handshake, followed by []byte messages. The latter are all encrypted when crypto is enabled. Note that it's the individual messages that are encrypted, not the stream. And the first message - the handshake - is always unencrypted.

It is therefore possible for an attacker to exploit weaknesses in gob. The following spring to mind:

  1. triggering an OOM crash by sending a very large gob message
  2. triggering an OOM crash by sending a sequence of octets that triggers a large memory allocation in gob
  3. triggering a loop in the gob decoder

I am quite sure 1 is possible. There is a 1GB limit in place but a) that's still quite large, and b) doesn't apply everywhere.

2 is different from 1 because it does not require network capacity and is therefore impossible to guard against with firewalls that enforce capacity limits. And I think it is possible, based on my reading of the gob decoder, i.e. there are places where a capacity is read and then an allocation of that capacity is performed.

I don't think 3 is possible, based on the description of gob. But I could well be wrong.

@inercia
Copy link
Contributor

inercia commented Jun 15, 2015

When you say "very large gob message", does the receiver accept messages with any length? I guess there should be a reasonable upper limit...

@rade
Copy link
Member Author

rade commented Jun 15, 2015

see my analysis above re length. The only limit I can find is 1GB, and it does not apply everywhere.

@inercia
Copy link
Contributor

inercia commented Jun 15, 2015

You analysis talks about the gob limits, but what I mean is whether we check the message length before passing it to the gob decoder... I think it does not make sense to try to decode any message (specially if we want protocol upgradeability)

@rade
Copy link
Member Author

rade commented Jun 15, 2015

There is no such thing as "messages" before the gob decoder. The gob layer is what gives us our message framing.

@rade rade modified the milestones: n/a, 1.0 Jun 15, 2015
@inercia
Copy link
Contributor

inercia commented Jun 15, 2015

Oh, I see, so we are in the hands of gob...

@squaremo
Copy link
Contributor

squaremo commented Jul 7, 2015

Oh, I see, so we are in the hands of gob...

Nice :)

It strikes me that either

  1. The maximum buffer size in gob should be adjustable (drastically, downwards); or,
  2. gob is a poor choice for the purpose to which weave puts it.

@paulbellamy paulbellamy self-assigned this Jul 7, 2015
@paulbellamy
Copy link
Contributor

Wire-encodings of messages is limited to 1GB, but by using specific structs you can create a gob stream where:

Encoded:     15 MB
Allocated: 1931 MB

Not yet sure if it is exploitable in weave.

@rade
Copy link
Member Author

rade commented Jul 8, 2015

Is that using straight gob or did it involve injecting byte sequences that "look like gob"? It's the latter that I suspect might permit allocating arbitrary large amounts of memory from little input. Not sure though; very much depends on the structure and implementation of the gob codec.

@paulbellamy
Copy link
Contributor

regular gob. Encoding a large recursive tree struct of strings (because zero-value strings are omitted from transmission).

@paulbellamy
Copy link
Contributor

slices have their size checked before decoding, but structs don't.

@rade rade added this to the current milestone Jul 8, 2015
@paulbellamy
Copy link
Contributor

Related: golang/go#10490 and golang/go#10273

dpw added a commit to dpw/weave that referenced this issue Jul 8, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid weaveworks#912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
dpw added a commit to dpw/weave that referenced this issue Jul 8, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid weaveworks#912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
dpw added a commit to dpw/weave that referenced this issue Jul 9, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid weaveworks#912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
dpw added a commit to dpw/weave that referenced this issue Jul 9, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid weaveworks#912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
dpw added a commit to dpw/weave that referenced this issue Jul 9, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid weaveworks#912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
dpw added a commit to dpw/weave that referenced this issue Jul 9, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid weaveworks#912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
dpw added a commit that referenced this issue Jul 13, 2015
To allow users to forgo compatibility with Weave Net 1.0 in order to
avoid #912, by setting --protocol-min-version=2.
Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
rade added a commit that referenced this issue Jul 14, 2015
New connection protocol version with encrypted features map

Closes #1029. Closes #912.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants