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

Compressible onions for rendez-vous routing #3557

Merged
merged 11 commits into from
Mar 11, 2020

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Feb 28, 2020

This is a draft of the compressible onion being discussed on the mailing list at the moment. It includes generation, compression, serialization and decompression of the compressible onions in the devtools/onion tool.

There are still a number of rough edges that I'll address before un-drafting this:

  • Move some more of the shared secret logic into sphinx.c. With this I also intend to introduce a new type compressed_onion that can be derived from a full onion, and that can be decompressed into a full onion. Currently this is handed around as a u8*.
  • There seems to be a stack-smashing bug somewhere, that I need to trace down.
  • Some of the commits need to be rephrased.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Agree that a struct compression_onion would wrap it nicely with a bow, but it's usable as-is for my tests!

common/sphinx.c Show resolved Hide resolved
common/sphinx.c Outdated Show resolved Hide resolved
devtools/onion.c Outdated Show resolved Hide resolved
common/sphinx.c Outdated Show resolved Hide resolved
@cdecker cdecker force-pushed the sphinx-prefill branch 3 times, most recently from a50543b to 3973557 Compare March 2, 2020 20:06
@cdecker
Copy link
Member Author

cdecker commented Mar 2, 2020

Reworked the API for handling compressed sphinx onions by adding the following functions:

  • serialize_compressed_onion
  • sphinx_decompress
  • sphinx_compress
  • sphinx_compressed_onion_deserialize
  • sphinx_compressed_onion_serialize

This should allow transforming the compressed onion from/to both full onions and serialized formats.

I also found the stack smashing issue, so this should be good to go :-) Keeping as a draft until the spec solidifies.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take "read_buffer" and throw it into the sun where future generations can't be exposed to it.

sphinx_compressed_onion_deserialize() MUST be safe against all inputs! If you used a pull_ function that would happen naturally, but also it shouldn't assert() on its inputs.

(The first thing I did is use this on an incoming TLV field, which would have been v. bad!)

common/sphinx.c Outdated Show resolved Hide resolved
devtools/onion.c Show resolved Hide resolved
common/sphinx.c Outdated Show resolved Hide resolved
common/sphinx.c Outdated Show resolved Hide resolved
@rustyrussell rustyrussell self-requested a review March 2, 2020 22:32
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make decompress function robust!

common/sphinx.c Outdated Show resolved Hide resolved
common/sphinx.c Outdated Show resolved Hide resolved
common/sphinx.c Outdated Show resolved Hide resolved
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still one deserialization bug, and gave you more work to do :)

common/sphinx.c Outdated
fromwire_u8_array(&cursor, &max, dst->routinginfo, routelen);
fromwire_u8_array(&cursor, &max, dst->mac, HMAC_SIZE);

assert(max == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert is true, but can we be more explicit?

I wonder if we'd be better with the hmac at the front for compressed onions? Makes it easier to pull apart if the variable record is at the end?

Either way, let's introduce a fromwire_tal_bytes() which does the tal_arr and fromwire_u8_array, IFF there is sufficient room in the msg (otherwise, don't even allocate). That prevents a whole class of errors, and means we can simply skip the routelen > ROUTING_INFO_SIZE check:

fromwire_pubkey(&cursor, &max, &dst->ephemeralkey);
dst->routinginfo = fromwire_talarr(&cursor, &max, max - HMAC_SIZE);
fromwire_u8_array(&cursor, &max, dst->mac, HMAC_SIZE);

if (!cursor)
    return tal_free(dst);

assert(max == 0);

Note that you still should check that cursor is not NULL: that pubkey could be malformed!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I like fromwire_tal_bytes, however it doesn't address the full issue, since that check isn't just there to avoid allocating a huge array, but also to enforce the logic that an onion that is larger than an uncompressed onion can't possibly be a compressed onion.

I added an initial check that the total compressed onion length is smaller than the total length of an uncompressed one, no longer computing routelen

@cdecker cdecker force-pushed the sphinx-prefill branch 6 times, most recently from 71b9da2 to 95ecdbb Compare March 6, 2020 12:51
@rustyrussell rustyrussell marked this pull request as ready for review March 10, 2020 23:27
These just run the test vectors and add a test for the devtools/onion tool so
we don't accidentally break them.
We will later use these to generate RV compressed onions and to opt into the
rendezvous style generation.
Adds the `--rendezvous-id` option allowing the caller to specify the node_id
of the rendez-vous node, and opting into the compressed onion generation.
This one generates a compressed onion, decompresses it, and then proceeds with
normal processing.
Also implements a way to decompress an onion using the devtools/onion tool

Changelog-Added: devtools: The `onion` tool can now generate, compress and decompress onions for rendez-vous routing
Expands the interface to play with onions a bit more. Potentially a bit
slower due to allocations, but that's a small price to pay. It also allows us
to avoid serializing a compressed onion to `u8*` if we process it right away.
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
It also removes the duplicate compression code and serialization code.
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
@rustyrussell
Copy link
Contributor

Ack: ebe9f9c

Rebase onto master, rename -devtool/onion option.

@rustyrussell
Copy link
Contributor

Removed final commit which broke tests; we can do a separate PR for that.

@rustyrussell rustyrussell merged commit 3710549 into ElementsProject:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants