Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#3291 closed defect (not a bug)

obfs2 should wait before closing connection on wrong magic/plength

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

Description

...
If the MAGIC_VALUE does not match, or the PADLEN value is greater
than MAX_PADDING, the party receiving it should wait for a random
amount of time then close the connection.
...

says the spec.

...
if (magic != OBFUSCATE_MAGIC_VALUE)
     return -1;
...

says the source.

The spec also doesn't specify the time interval that we should be randomly waiting.

Child Tickets

Change History (11)

comment:1 Changed 10 years ago by asn

I coded something in my bug3291 branch and I also did a patchy update of the spec.
timespec seems to be a POSIX thing which kills every chance of this being multiplatform and I can't do anything more before getting a Windows development machina.

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

Replying to asn:

I coded something in my bug3291 branch and I also did a patchy update of the spec.
timespec seems to be a POSIX thing which kills every chance of this being multiplatform and I can't do anything more before getting a Windows development machina.

On fresh morning's thoughts, my lame nanosleep() solution won't work, since it borks other connections while it's sleeping.

comment:3 Changed 10 years ago by rransom

Did you try the evtimer_ functions?

comment:4 Changed 10 years ago by asn

Status: newneeds_review

Okay, I have a hopefully better implementation on my bu3291 branch now.
I used it's bug3302 as it's parent branch even though it's not yet merged since it uses the new proto_recv() return code enum thing.
I forcefully fast forwarded it in the old branch; I hope everything went fine.

comment:5 Changed 10 years ago by nickm

As noted, RECV_OBFS2_PENDING should probably be called RECV_SEND_PENDING or something. It's something that other protocols can return too, right?

C tip: the cast here is redundant:

+  if (evbuffer_add_buffer(dest,source)<0)
+    return (enum recv_ret) RECV_BAD;
+  else
+    return (enum recv_ret) RECV_GOOD;

RECV_BAD and RECV_GOOD are already of the type you need, and adding extra casts makes it look like you're doing something dangerous when really it's all good.

The comment on obfs2_recv is now wrong.

The RECV_BAD_WAIT patch makes no sense to me. What is it for? Why can't we just close the connection?

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

Replying to nickm:

The RECV_BAD_WAIT patch makes no sense to me. What is it for? Why can't we just close the connection?

<asn> The RECV_BAD_WAIT patch is to implement the spec (check top of #3291).
<asn> Now, when I first saw that part of the spec I thought "Oh okay, this is obviously to avoid active-timing attacks where someone sends a random packet to the obfsproxy and sees
      if it kills the connection immediately, then that someone knows that something is fishy (since it accepted other packets but not his).".
<asn> But now that you questioned that in the ticket, I'm not sure what it is for.
<asn> Because it obviously doesn't counter an "attack" like the above.
<asn> But why is it written in the spec, then?

comment:7 Changed 10 years ago by nickm

Oops; when I saw the patch, I was reading it as part of 3302, where it didn't make sense. Here it makes sense. Will review again.

comment:8 Changed 9 years ago by nickm

Hm. The event_set interface is deprecated. Why not use event_new() ?

Also, what frees the event we calloc there? It looks like it leaks.

Also, let's not abbreviate "libevent" as "libev" -- there's another project out there called "libev".

Looks like set_rand_timeout() has an undocumented maximum value.

In crypto_rand_int ... never copy code without crediting the source.

comment:9 Changed 9 years ago by nickm

I think for now we're thinking of deferring this one, yeah?

comment:10 Changed 9 years ago by asn

Resolution: not a bug
Status: needs_reviewclosed

This is probably not gonna happen. The spec. was updated in f439d980caef07a834771fde90a726dcf156e31d.

comment:11 Changed 9 years ago by arma

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