Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3202 closed defect (implemented)

shared secret support in obfs2

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

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?

comment:2 in reply to:  1 Changed 8 years ago by asn

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). 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.

comment:3 Changed 8 years ago by 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.

(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?

comment:4 in reply to:  3 Changed 8 years ago by asn

Status: needs_reviewassigned

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!

comment:5 Changed 8 years ago by 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.

comment:6 in reply to:  5 Changed 8 years ago by asn

Status: assignedneeds_review

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 loosely recommends more than 1000=103 iterations.
According to this, iOS3 was doing 2*103 iterations, iOS4 is doing 104 iterations and blackberry is nowadays doing 2*104 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 105 suggestion sounds like a safe choice with the current standards. Of course, it's 'easy' to make it customizable as well.

comment:7 Changed 8 years ago by asn

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().

comment:8 Changed 8 years ago by asn

Resolution: implemented
Status: needs_reviewclosed

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

comment:9 Changed 7 years ago by arma

Component: Pluggable transportObfsproxy
Note: See TracTickets for help on using tickets.