torproject / tor Public
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
Ticket26288 041 02 #725
Ticket26288 041 02 #725
Conversation
Take apart the SENDME cell specific code and put it in sendme.{c|h}. This is
part of prop289 that implements authenticated SENDMEs.
Creating those new files allow for the already huge relay.c to not grow in LOC
and makes it easier to handle and test the SENDME cells in an isolated way.
This commit only moves code. No behavior change.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is a bit of a complicated commit. It moves code but also refactors part of it. No behavior change, the idea is to split things up so we can better handle and understand how SENDME cells are processed where ultimately it will be easier to handle authenticated SENDMEs (prop289) using the intermediate functions added in this commit. The entry point for the cell arriving at the edge (Client or Exit), is connection_edge_process_relay_cell() for which we look if it is a circuit or stream level SENDME. This commit refactors that part where two new functions are introduced to process each of the SENDME types. The sendme_process_circuit_level() has basically two code paths. If we are a Client (the circuit is origin) or we are an Exit. Depending on which, the package window is updated accordingly. Then finally, we resume the reading on every edge streams on the circuit. The sendme_process_stream_level() applies on the edge connection which will update the package window if needed and then will try to empty the inbuf if need be because we can now deliver more cells. Again, no behavior change but in order to split that code properly into their own functions and outside the relay.c file, code modification was needed. Part of #26840. Signed-off-by: David Goulet <dgoulet@torproject.org>
When we get a relay DATA cell delivered, we have to decrement the deliver window on both the circuit and stream level. This commit adds helper functions to handle the deliver window decrement. Part of #26840 Signed-off-by: David Goulet <dgoulet@torproject.org>
When we are about to send a DATA cell, we have to decrement the package window for both the circuit and stream level. This commit adds helper functions to handle the package window decrement. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, we would only close the stream when our deliver window was negative at the circuit-level but _not_ at the stream-level when receiving a DATA cell. This commit adds an helper function connection_edge_end_close() which sends an END and then mark the stream for close for a given reason. That function is now used both in case the deliver window goes below zero for both circuit and stream level. Part of #26840 Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to be able to deploy the authenticated SENDMEs, these two consensus parameters are needed to control the minimum version that we can emit and accept. See section 4 in prop289 for more details. Note that at this commit, the functions that return the values aren't used so compilation fails if warnings are set to errors. Closes #26842 Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
This code will obey the consensus parameter "sendme_emit_min_version" to know which SENDME version it should send. For now, the default is 0 and the parameter is not yet used in the consensus. This commit adds the support to send version 1 SENDMEs but aren't sent on the wire at this commit. Closes #26840 Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit makes tor able to parse and handle a SENDME version 1. It will look at the consensus parameter "sendme_accept_min_version" to know what is the minimum version it should look at. IMPORTANT: At this commit, the validation of the cell is not fully implemented. For this, we need #26839 to be completed that is to match the SENDME digest with the last cell digest. Closes #26841 Signed-off-by: David Goulet <dgoulet@torproject.org>
This makes tor remember the last seen digest of a cell if that cell is the last one before a SENDME on the Exit side. Closes #26839 Signed-off-by: David Goulet <dgoulet@torproject.org>
Now that we keep the last seen cell digests on the Exit side on the circuit object, use that to match the SENDME v1 transforming this whole process into a real authenticated SENDME mechanism. Part of #26841 Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to do so, depending on where the cell is going, we'll keep the last cell digest that is either received inbound or sent outbound. Then it can be used for validation. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Hi! This review covers the first commit, up to but not including "support SENDME v1 cell parsing"
src/trunnel/sendme.trunnel
Outdated
| /* SENDME version 1. Authenticated with digest. */ | ||
| struct sendme_data_v1 { | ||
| /* A 4 bytes digest. */ | ||
| u8 digest[4]; |
src/trunnel/sendme.trunnel
Outdated
|
|
||
| /* The data content depends on the version. */ | ||
| u16 data_len; | ||
| u8 data[data_len]; |
This could be a trunnel union, something like
{{{
union data[version] with length data_len {
0: ignore;
1: u8 digest[4];
}
}}}
src/core/or/sendme.c
Outdated
| conn->deliver_window += STREAMWINDOW_INCREMENT; | ||
| if (connection_edge_send_command(conn, RELAY_COMMAND_SENDME, | ||
| NULL, 0) < 0) { | ||
| log_warn(LD_APP,"connection_edge_send_command failed. Skipping."); | ||
| return; /* the circuit's closed, don't continue */ | ||
| log_warn(LD_APP, "connection_edge_send_command failed while sending " |
People aren't seeing these warnings in the wild. Should we call this LD_BUG?
src/core/or/relay.c
Outdated
| circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), | ||
| rh.length); | ||
| } | ||
| { |
Let's extract this into a separate function in some future commit.
src/core/or/relay.c
Outdated
| } | ||
| /* Resume reading on any streams now that we've processed a valid | ||
| * SENDME cell that updated our package window. */ | ||
| circuit_resume_edge_reading(circ, layer_hint); |
are you sure we're doing this in exactly the same cases where we were doing it before?
Yes. The processing of the circuit level cell above with sendme_process_circuit_level() returns an error if anything is wrong with the cell which closes the circuits. On success, we resume reading because our package window increased.
The ONLY difference is that we call circuit_read_valid_data() in the processing step if everything went well which is the same as we had before where on success we would resume reading and then increment the counters with the valid data function. Overall, it is the same behavior.
| @@ -82,3 +84,99 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| /* Process a circuit-level SENDME cell that we just received. The layer_hint, | |||
General question: how sure are you that there are no behavior changes in this commit?
I've spent quite a bit of time to go over each single line of this SENDME code in order to do exactly what we currently do with this refactoring commit.
My certainty is 99% I would say because of the amount of tests I did and time I spent in that code.
| return; | ||
| } | ||
|
|
||
| connection_edge_end(conn, reason); |
I could have sworn that we already had a function that did this, but it looks like we don't.
src/core/or/sendme.c
Outdated
| * layer_hint is NULL, this means we are the Exit end point else we are the | ||
| * Client. Update the package window and return its new value. */ | ||
| int | ||
| sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint) |
I'd suggest adding "note" between sendme and stream/circuit in these functions' names, to make their purpose more clear.
src/core/or/sendme.c
Outdated
| sendme_cell_set_version(cell, 0x01); | ||
|
|
||
| /* Copy the digest into the data payload. */ | ||
| crypto_digest_get_digest(cell_digest, |
Suggestion: instead of using a crypto_digest_t in all of these functions, it would be a good idea to use a relay_crypto_t, and put the function for generating the sendme tag into src/core/crypto/relay_crypto.c. That way, we separate the code that knows how the tag is constructed from the code that uses the tag. This might help us later on, when we want to change how circuit crypto works.
The use of crypto_digest_t has been removed with commit 3943d09. We do not compute the digest since we don't keep the digest object anymore.
src/core/or/sendme.c
Outdated
| /* Can we handle this version? */ | ||
| if (accept_version > SENDME_MAX_SUPPORTED_VERSION) { | ||
| log_warn(LD_PROTOCOL, "Unable to handle SENDME version %u " | ||
| "(from the consensus). We only support <= %d. " |
Why does this say "from the consensus"? it's coming from a cell, not the consensus. Maybe we shouldn't warn here, but send protocol_warn instead?
Ah!!! Gooooood catch, it is reverse. The second value is the one from the consensus. And yes protocol warn would is better!
Fixup commit: ddad00e
src/core/or/circuit_st.h
Outdated
| * sent by the client. It is done at the last cell before our package_window | ||
| * goes down to 0 which is when we expect a SENDME. The protocol doesn't | ||
| * allow more than 10 outstanding SENDMEs worth of data meaning this list | ||
| * should only contain at most 10 digests of 4 bytes each. */ |
We should document how these digests correspond to positions in the stream, exactly. Like, the most recent digest in the list corresponds to the state up until the cell we received at...
Also, "10" and "4" should really be constants here.
src/core/or/sendme.c
Outdated
| return; | ||
| } | ||
|
|
||
| digest = tor_malloc_zero(4); |
| /* We shouldn't have received this SENDME if we have no digests. Log at | ||
| * protocol warning because it can be tricked by sending many SENDMEs | ||
| * without prior data cell. */ | ||
| if (circ->sendme_last_digests == NULL || |
Maybe let's extract this digest-matching code into its own function?
src/core/or/sendme.c
Outdated
|
|
||
| /* Compare the digest with the one in the SENDME. This cell is invalid | ||
| * without a perfect match. */ | ||
| if (tor_memcmp(digest, sendme_data_v1_getconstarray_digest(data), |
If you're only checking for inequality, prefer tor_memneq() to tor_memcmp(). tor_memcmp() is more complicated because it has to determine ordering.
src/core/crypto/relay_crypto.c
Outdated
| crypto_digest_free(thishop->crypto.sendme_digest); | ||
| } | ||
| thishop->crypto.sendme_digest = | ||
| crypto_digest_dup(thishop->crypto.b_digest); |
This approach seems needlessly wasteful to me. The digest object is as large as the entire internal digest object's state, which is often much larger than the actual set of bytes you're transmitting. Why not just store the digest itself, rather than the crypto_digest_t?
src/core/crypto/relay_crypto.c
Outdated
| @@ -142,6 +142,13 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, | |||
| if (relay_digest_matches(thishop->crypto.b_digest, cell)) { | |||
| *recognized = 1; | |||
| *layer_hint = thishop; | |||
| /* Keep current digest of this cell for the possible SENDME. */ | |||
| if (thishop->crypto.sendme_digest) { | |||
| crypto_digest_free(thishop->crypto.sendme_digest); | |||
Should we log a BUG warning if we drop this value without having sent it in a SENDME cell?
Commit 3943d09 now makes it that we overwrite... At this point, we do not know if we are dropping it or not :S.
src/core/or/relay.c
Outdated
| * where the other side can predict all of the bytes in the payload and thus | ||
| * compute authenticated sendme cells without seeing the traffic. See | ||
| * proposal 289. */ | ||
| crypto_fast_rng_getbytes(get_thread_fast_rng(), |
I recall an old suggestion I made once: let's skip the first 4 bytes of the unused data. Having some bytes that we know will be set to 0 if they are unused has saved us a lot of times in the past.
Done in commit 0c23471
There is some more calculation to make sure we don't overflow here. Needs special attentions :).
To achieve such, this commit also changes the trunnel declaration to use a union instead of a seperate object for the v1 data. A constant is added for the digest length so we can use it within the SENDME code giving us a single reference. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
No behavior change. Only moving code and fixing part of it in order to use the parameters passed as pointers. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
The circuit and stream level functions that update the package window have been renamed to have a "_note_" in them to make their purpose more clear. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
No behavior change but code had to be refactored a bit. Also, the tor_memcmp() was changed to tor_memneq(). Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
The digest object is as large as the entire internal digest object's state, which is often much larger than the actual set of bytes you're transmitting. This commit makes it that we keep the digest itself which is 20 bytes. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
When adding random to a cell, skip the first 4 bytes and leave them zeroed. It has been very useful in the past for us to keep bytes like this. Some code trickery was added to make sure we have enough room for this 4 bytes offset when adding random. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Hi! We're getting closer and closer with this. I have some more notes on efficiency issues here, in particular. We need to minimize the number of digest calculations we do, since SHA1 is pretty expensive compared to the other stuff we do on each cell.
| union data[version] with length data_len { | ||
| 0x00: ignore; | ||
| 0x01: u8 v1_digest[TRUNNEL_SENDME_V1_DIGEST_LEN]; | ||
| }; |
Do we want parsing to fail if the version is 2 or more? If not we should add a default: case to the union.
I believe we do want to fail. A SENDME you can't parse (most likely unknown version) means you can't continue and thus the circuit is closed. This is the sendme cell validation code:
if (sendme_cell_parse(&cell, cell_payload, cell_payload_len) < 0) {
log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
"Unparseable SENDME cell received. Closing circuit.");
goto invalid;
}
src/core/crypto/relay_crypto.c
Outdated
| thishop->crypto.sendme_digest = | ||
| crypto_digest_dup(thishop->crypto.b_digest); | ||
|
|
||
| crypto_digest_get_digest(thishop->crypto.b_digest, |
Hm. Why are we doing this on every cell that we decrypt? It seems very wasteful; digests can be quite expensive.
src/core/crypto/relay_crypto.c
Outdated
| crypto_digest_free(or_circ->crypto.sendme_digest); | ||
| } | ||
| or_circ->crypto.sendme_digest = crypto_digest_dup(or_circ->crypto.b_digest); | ||
| crypto_digest_get_digest(or_circ->crypto.b_digest, |
As above, do we really need to do this on every cell we encrypt?
I'm hoping that we can only generate digests when we actually need them.
src/core/or/sendme.c
Outdated
| } | ||
| smartlist_add(circ->sendme_last_digests, digest); | ||
| /* Add the digest to the last seen list in the circuit. */ | ||
| digest = TO_OR_CIRCUIT(circ)->crypto.sendme_digest; |
Let's have a function here instead that calculates the sendme digest for us from the relay_crypto. That way:
- we can get the digest through some different algorithm later on when other kinds of crypto are in use
- we don't have to compute the digest with every cell
- we don't have to make sendme.c know about the internals of relay_crypto.
Not sure I understand. Probably here you mean a function in relay_crypto.c that returns the SENDME digest so we can use it to match the one in the circuit (circ->sendme_last_digests)?
right -- I mean that we shouldn't reach inside the crypto object here. Instead we should have an accessor function.
src/core/or/relay.c
Outdated
| * | ||
| * We'll skip the first 4 bytes of unused data because having some unused | ||
| * zero bytes has saved us a lot of times in the past. */ | ||
| random_bytes_len = RELAY_PAYLOAD_SIZE - |
I'm a little scared by signed/unsigned things when we're checking for negative numbers. Can we have a test for this? It might help to put it in a separate function. Or maybe it would be easier to wrap this part of the code in something like:
offset = RELAY_HEADER_SIZE + PAYLOAD_LEN + 4
if (offset < RELAY_PAYLOAD_SIZE) {
len = RELAY_PAYLOAD_SIZE - offset;
// ...
}
Sooooo... good thing you requested that because the code was actually not correct. The offset should have been tested against CELL_PAYLOAD_SIZE since we offset on cell.payload.
I've pushed a new commit that creates two helper function for the random padding and adds a unit test to test the offset value we use. I would recommend reviewing carefully here because it is very confusing to have payload_len not be the length of cell.payload but rather the data length.
See commit: 09f67b4
We'll use it this in order to know when to hash the cell for the SENDME instead of doing it at every cell. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
This way, we reduce the load by only hashing when we absolutely must. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
We add random padding to every cell if there is room. This commit not only fixes how we compute that random padding length/offset but also improves its safety with helper functions and a unit test. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Pull Request Test Coverage Report for Build 4881
|
src/core/or/sendme.c
Outdated
| @@ -286,10 +286,37 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, | |||
| return 0; | |||
| } | |||
|
|
|||
| /* Put the crypto.b_digest in the sendme_digest. */ | |||
| static void | |||
| note_cell_digest(const relay_crypto_t *crypto) | |||
(This should probably go into the relay_crypto module)
Moved it to relay_crypto.c and named relay_crypto_record_sendme_digest()
See new commit 4d6d633
src/core/or/sendme.c
Outdated
| /** Keep the current inbound cell digest for the next SENDME digest. This part | ||
| * is only done by the client as the circuit came back from the Exit. */ | ||
| void | ||
| sendme_circuit_note_outbound_cell(or_circuit_t *or_circ) |
This function's name makes it sound like it should get called for every cell. Maybe sendme_circuit_record_outbound_digest() ?
src/core/or/sendme.c
Outdated
| /** Keep the current inbound cell digest for the next SENDME digest. This part | ||
| * is only done by the client as the circuit came back from the Exit. */ | ||
| void | ||
| sendme_circuit_note_inbound_cell(crypt_path_t *cpath) |
src/core/or/sendme.c
Outdated
| * one cell (the possible SENDME cell) should be a multiple of the increment | ||
| * window value. */ | ||
| bool | ||
| sendme_circuit_is_next_cell(int window) |
I want to namespace it ... so it would end up a bit confusing like sendme_next_cell_is_sendme(). Also, the circuit in there I would like to keep because it cannot be used for a stream level SENDME check.
What about sendme_circuit_cell_is_next() ?
If you are ok with it, see fixup commit: 17636ed]
Because this function is poking within the relay_crypto_t object, move the function to the module so we can keep it opaque as much as possible. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
From nickm's review, improve the names of some functions. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
As per review from nickm, keep as much as we can the relay_crypto_t object opaque. Part of #26288 Signed-off-by: David Goulet <dgoulet@torproject.org>
No description provided.
The text was updated successfully, but these errors were encountered: