This repository has been archived by the owner on Mar 4, 2021. It is now read-only.
HTTP Host and TLS SNI fragmentation/stuttering/fuzzing test #1522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bassosimone
reviewed
Dec 20, 2017
| auto aread = std::make_shared<std::string>(); | ||
| auto awrite = std::make_shared<std::string>(); | ||
|
|
||
| SSL_CTX* ctx = SSL_CTX_new(SSLv23_method()); //XXX don't ignore errors |
There was a problem hiding this comment.
SSLv23_method() cannot fail, IIRC. It should return a pointer to a statically allocated struct.
bassosimone
reviewed
Dec 20, 2017
| return *aread; //XXX probably raise exception | ||
| } | ||
|
|
||
| if (connect(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) < 0) { |
There was a problem hiding this comment.
If think you can ask OpenSSL to connect on your behalf
bassosimone
reviewed
Dec 20, 2017
|
|
||
| void dpi_fragment(Settings options, Callback<SharedPtr<report::Entry>> callback, | ||
| SharedPtr<Reactor> reactor, SharedPtr<Logger> logger) { | ||
| reactor->call_in_thread(logger, [=]() { |
There was a problem hiding this comment.
I suggest to do [callback = std::move(callback)]. This is C++14 and should guarantee that the thread uniquely owns the callback. I remember not doing that was leading to weird behavior sometimes.
bassosimone
reviewed
Dec 20, 2017
| logger->info("unfragmented https response length: %d", unfragmented_https.length()); | ||
| logger->info("fragmented http response length: %d", fragmented_http.length()); | ||
| logger->info("unfragmented http response length: %d", unfragmented_http.length()); | ||
| callback(entry); |
There was a problem hiding this comment.
I suggest to do
reactor->call_soon([entry = std::move(entry), callback = std::move(callback)]() {
callback(entry);
});so that the callback is called from the I/O thread loop, which is what usually happens.
bassosimone
reviewed
Dec 20, 2017
| int main(std::list<Callback<BaseTest &>> &initializers, int argc, char **argv) { | ||
| mk::nettests::DpiFragmentTest test; | ||
| int ch; | ||
| while ((ch = getopt(argc, argv, "B:f:")) != -1) { |
There was a problem hiding this comment.
You should clean the options string here
joelanders
force-pushed
the
sni_stutter
branch
from
d80070b to
e15375d
Compare
joelanders
force-pushed
the
sni_stutter
branch
from
e15375d to
d74f415
Compare
|
joelanders
changed the title
|
Closing as stated in ooni/spec#103 (comment). |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to spec PR
So instead of trying to hook things at the libevent level, which even Simone says is Hard To Do (:)), I'm doing a good ol' blocking
socket(); connect(); select(); read(); write();loop in my own thread. I'm telling OpenSSL to talk to some buffers instead of the socket directly, and I'm taking care of read/writing them to the socket (with thesleep()trick around the plaintext hostname).