Opened 4 years ago

Closed 4 years ago

#10893 closed defect (fixed)

ScrambleSuit spec improvements

Reported by: yawning Owned by: phw
Priority: Medium Milestone:
Component: Obfuscation/Pluggable transport Version:
Severity: Keywords: scramblesuit spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Things I've noticed when adding ScrambleSuit support to obfsclient:

  • The spec lies about the contents of MAC for the UniformDH handshake. Instead of "MAC(X | P_C | E)"/"MAC(X | P_S | E)" this should be "MAC(X | P_C | M_C | E)"/"MAC(Y | P_S | M_S | E)". The mark is part of the HMAC verifier, and for the server's MAC, the server's UniformDH key is used when computing the digest.
  • Should the server echo the epoch received from the client? The server should attempt to verify the client's identifier with E - 1 or E + 1 and E, and it implicitly knows the E value the client sent, so it should echo it. Or the client could also verify more than 1 MAC.
  • What happens when the random padding contains the mark? Should the client/server continue to scan for the MAC, or just fail the connection (The odds of this happening are *extremely unlikely* so failing it is probably fine).

Things that are totally missing:

  • The Protocol Polymorphism PRNG needs to be documented.

Some of the things I discussed with phw already. I still haven't tackled Ticket Handshake yet, so I may end up adding more stuff to this.

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by yawning

Component: ObfsproxyPluggable transport

comment:2 Changed 4 years ago by yawning

Another one.

  • k_t used for the Session Ticket handshake HMACs is the result of 2.3 Key Derivation, instead of being the raw k_t. The obfsproxy implementation is incorrect, but it's deployed so it wins, at least as far as what goes into obfsclient.
            log.debug("Redeeming stored session ticket.")
            (masterKey, rawTicket) = storedTicket
            self.deriveSecrets(masterKey)
            self.circuit.downstream.write(ticket.createTicketMessage(rawTicket,
                                                                self.sendHMAC))

comment:3 Changed 4 years ago by yawning

ScrambleSuit's maximum message length is 1448 bytes. Exluding the header, this results in
1437 bytes for the transported data.

Should be 1427 bytes, since the header is 21 bytes long.

comment:4 in reply to:  description Changed 4 years ago by phw

I created branch 10893 to work on the issues:
https://gitweb.torproject.org/user/phw/scramblesuit.git/shortlog/refs/heads/bug_10893.

Replying to yawning:

  • The spec lies about the contents of MAC for the UniformDH handshake. Instead of "MAC(X | P_C | E)"/"MAC(X | P_S | E)" this should be "MAC(X | P_C | M_C | E)"/"MAC(Y | P_S | M_S | E)". The mark is part of the HMAC verifier, and for the server's MAC, the server's UniformDH key is used when computing the digest.

Thanks, fixed.

  • Should the server echo the epoch received from the client? The server should attempt to verify the client's identifier with E - 1 or E + 1 and E, and it implicitly knows the E value the client sent, so it should echo it. Or the client could also verify more than 1 MAC.

The main purpose of E is to keep the server's replay cache small. Since the client does not maintain a replay cache, E could simply be echoed by the server. In fact, it might even be discarded altogether but that would break compatibility with old protocol versions.

And yes, the server should also check E-1 or E+1 to prevent the occasional authentication failure when an hour wraps.

  • What happens when the random padding contains the mark? Should the client/server continue to scan for the MAC, or just fail the connection (The odds of this happening are *extremely unlikely* so failing it is probably fine).

I agree that that failing is OK. Everything else would just add complexity.

Things that are totally missing:

  • The Protocol Polymorphism PRNG needs to be documented.

Yes. I'm somewhat limited by PyCrypto's CSPRNG API (or lack thereof) but I will come up with something. So far, I'm using a Mersenne Twister but a CSPRNG is certainly a better choice.

Last edited 4 years ago by phw (previous) (diff)

comment:5 Changed 4 years ago by phw

Quick update: Everything except protocol polymorphism should now be fixed in branch 10893.

comment:6 Changed 4 years ago by phw

So it looks like PyCrypto provides no straightforward way to seed a CSPRNG. I see the following options:

  1. Monkeypatch PyCrypto's internals which is ugly and error-prone.
  2. Use another Python crypto library which is also ugly and will bloat up the bundles.
  3. Use a PRNG which is not cryptographically secure. That's what I'm doing now because of the lack of better options. E.g., Python's random module uses a Mersenne Twister.
  4. Keep the spec vague and don't dictate how exactly the distributions for polymorphism should be generated or how samples should be drawn from them.

I think I prefer (but don't love) the fourth option. It would mean that the flow signature of two different implementations would probably differ but that doesn't have to be a bad thing. Opinions?

comment:7 Changed 4 years ago by phw

Status: newneeds_information

comment:8 Changed 4 years ago by yawning

I think 4 (keep it vague) with a provision stating that "implementations SHOULD use a CSPRNG" would be the best out of of the options that you presented. I agree that flow signatures being different between implementations is not necessarily a bad thing.

I could write a CTR_DRBG based on the AES code in obfsproxy if needed as well (I had to do that for obfsclient since there isn't a easy way to get separate CSPRNG instances out of OpenSSL).

comment:9 in reply to:  8 Changed 4 years ago by phw

Status: needs_informationneeds_review

Replying to yawning:

I think 4 (keep it vague) with a provision stating that "implementations SHOULD use a CSPRNG" would be the best out of of the options that you presented. I agree that flow signatures being different between implementations is not necessarily a bad thing.

Sounds good. I updated the spec which hopefully fixes this ticket.

I could write a CTR_DRBG based on the AES code in obfsproxy if needed as well (I had to do that for obfsclient since there isn't a easy way to get separate CSPRNG instances out of OpenSSL).

That sounds like a good medium-term thing to do. In the short term, we might be OK with the current implementation. I will open a dedicated ticket for CTR_DRBG.

comment:10 Changed 4 years ago by yawning

UniformDH handshake documentation:

HMAC-SHA256-128(k_B, X | P_C | E)

Should be updated to include M_C.

Session Ticket documentation:

MAC = HMAC-SHA256-128(k_sh, T | P | E)

| T | P | M | MAC(T | P | E) | T: session ticket

server then verifies the MAC which is built over T | P | E.

The HMAC generation includes the mark like in the UniformDH case and should be documented as such (See ticket.py:62).

Everything else looks good to me.

comment:11 Changed 4 years ago by phw

Resolution: fixed
Status: needs_reviewclosed

Should be fixed and merged to master.

Thanks a lot for all the work!

Note: See TracTickets for help on using tickets.