shared secret support in obfs2

I coded (well, it was already coded but I made it functional) support for shared secrets in my shared_secret obfsproxy branch.

Some things that I don't know if I like or not:

• I currently set the shared secret per-connection, instead of setting the shared secret per-protocol on startup.
• I created a protocol_params_t struct that contains protocol parameters (eg, if we are the initiator, shared secret, etc.). It gets passed to proto_new() when we are creating a protocol object for every connection. Maybe in the future we will need to put more stuff in there, maybe not.

added component::archived/obfsproxy owner::asn priority::medium resolution::implemented status::closed type::defect labels

Looks like that includes a couple of earlier commits that aren't about shared secrets at all?

Also... what's up with disabling all those unit tests?

Trac:
Status: new to needs_review

Replying to nickm:

Looks like that includes a couple of earlier commits that aren't about shared secrets at all?

Also... what's up with disabling all those unit tests?

Hi! This, indeed, includes the commits of the socks_send_fail_replies branch (#3214 (closed)). Fortunately, bug3214 got merged so the extra commits shouldn't disturb you that much.

I also had to disable the unit tests because I ended up changing proto_new() and all the obfs2 unit tests needed changes. I wanted to first see your comments on this branch, before fixing the unit tests.

I don't think I agree with having the shared secret be NUL-terminated in the protocol level. It's fine to only take C strings from the command line, but why limit the code by making it unable to handle something the protocol allows fine?

Also, it seems silly to just take the thing from the command line and shove it into the "secret" part of the protocol directly, truncating its later bytes. Instead we should hash it so that every byte of input counts, and the input doesn't need to be a given length. Maybe we should even do an iterated hash approach to slow down password-guessing attempts.

(Also, the time to change stuff is now: since our hash function is 32 bytes, let's make the shared secret get to be 32 bytes long.)

Also, why not copy the secret into the state? Keeping a pointer around to it and requiring that it never change or get freed seems like an iffy interface choice.

Also the only code I see here sets lsn->have_shared_secret = 1 unconditionally. That's not right, is it? I'm guessing that this is something else that you had for testing that you mean to turn off later?

Thanks for the comments! I improved the code in my shared_secret branch.

Replying to nickm:

I don't think I agree with having the shared secret be NUL-terminated in the protocol level. It's fine to only take C strings from the command line, but why limit the code by making it unable to handle something the protocol allows fine?

Also, it seems silly to just take the thing from the command line and shove it into the "secret" part of the protocol directly, truncating its later bytes. Instead we should hash it so that every byte of input counts, and the input doesn't need to be a given length. Maybe we should even do an iterated hash approach to slow down password-guessing attempts.

You are right. In my latest commits, I stopped treating the shared secret as a C-string through network.c and obfs2.c. The thing is that command line arguments are C-strings anyway. I assume that 'in the future' there will be another way to add shared secrets (config file?) for hardcore binary people. 'till then we are fine with a shared secret of arbitrary size, aren't we?

About iterated hashing, I see no reason to not do it.

This whole thing will also require spec overwrite. I'll prepare a patch for the iterated hashing and the spec soon.

(Also, the time to change stuff is now: since our hash function is 32 bytes, let's make the shared secret get to be 32 bytes long.)

True. Done!

Also, why not copy the secret into the state? Keeping a pointer around to it and requiring that it never change or get freed seems like an iffy interface choice.

True! I ended up copying the secret to the protocol_params_t struct, which I also documented and I think it fits the role quite nice. I also created a function that frees it when the listener dies.

Also the only code I see here sets lsn->have_shared_secret = 1 unconditionally. That's not right, is it? I'm guessing that this is something else that you had for testing that you mean to turn off later?

True!

Trac:
Status: needs_review to assigned

Looks better.

Yeah, my rationale for allowing arbitrary shared secrets is that we might want to have a C API in the future, and not just have this used through the command line.

Once the iterated hashing and spec changes in, let me know and I'll merge. I'll arbitrarily suggest something like 100,000 SHA256 iterations.

Replying to nickm:

Looks better.

Yeah, my rationale for allowing arbitrary shared secrets is that we might want to have a C API in the future, and not just have this used through the command line.

Once the iterated hashing and spec changes in, let me know and I'll merge. I'll arbitrarily suggest something like 100,000 SHA256 iterations.

Done. Check my branch.

I'll now do a break while bikeshedding pondering on the '100,000 SHA256 iterations':

PKCS!#5 (closed) loosely recommends more than 1000=10^3^ iterations. According to this, iOS3 was doing 210^3^ iterations, iOS4 is doing 10^4^ iterations and blackberry is nowadays doing 210^4^ of them.

A BFMI attacker in our case not only has to do the SHA256 iterations but afterwards also has to apply an AES-CTR-128. I think that your 10^5^ suggestion sounds like a safe choice with the current standards. Of course, it's 'easy' to make it customizable as well.

Trac:
Status: assigned to needs_review

Okay, I also revived the unit tests and I killed the annoying bug I was rambling about in IRC. Basically, in the

    if (state->pending_data_to_send) {
      crypt_and_transmit(state->send_crypto, state->pending_data_to_send, dest);
      evbuffer_free(state->pending_data_to_send);
      state->pending_data_to_send = NULL;
    }

blob in obfs2_recv(), the 'dest' argument of crypt_and_transmit() was pointing to the wrong evbuffer. So crypt_and_transmit() was spitting an encrypted version of my input back to me, which was in turn garbling my next input and in the end the data passed to the server had nothing to do with my actual input.

My current fix is not really nice, since it sends up the queued data only on the next proto_send(), but I didn't see a way of getting the correct evbuffer from inside obfs2_recv().

This was merged. The remnants of this can be found on #3302 (closed).

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

Trac:
Component: Pluggable transport to Obfsproxy

closed