Opened 6 years ago

Closed 6 years ago

#11092 closed defect (fixed)

scramblesuit should make sure that handshake padding is less than MAX_PADDING_LENGTH

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

Description

scramblesuit should check that incoming padding is less than MAX_PADDING_LENGTH

Child Tickets

Change History (8)

comment:1 Changed 6 years ago by phw

The obvious solution would be to simply close the TCP connection if authentication did not succeed in MAX_PADDING_LENGTH + something. However, adversaries could easily determine this limit by sending garbage data one byte at a time and check when the server closes the connection.

We already have the server's unique seed and it should probably be used to derive a server-specific limit which is then used to determine when an unauthenticated TCP connection should be closed.

comment:2 Changed 6 years ago by phw

Status: newneeds_review

comment:3 Changed 6 years ago by phw

Cc: yawning added

comment:4 Changed 6 years ago by yawning

In util.locateMark(mark) change "index = payload.find(mark)" to be bounded like :

    index = payload.find(mark, 0, const.MAX_PADDING_LENGTH +
                                  const.MARK_LENGTH)

Without the change, it is still possible to accept out-of-spec packets.

The current code no longer buffers data forever, but it still continues to process the handshake till the threshold is reached. In ScrambleSuitTransport.receivedDownstream consider doing something like:

    if self.weAreServer and (self.protoState == const.ST_AUTH_FAILED):
        self.rxHandshakeLength += len(data)
        data.drain(len(data))
        if self.rxHandshakeLength > self.srvState.closingThreshold:
            log.info("Terminating connection after having received %d"
                     " bytes because client could not "
                     "authenticate." % self.rxHandshakeLength)
            self.circuit.close()
            return
    else if self.weAreServer and (self.protoState == const.ST_WAIT_FOR_AUTH):

        # blah blah blah

        else:
            if len(data) > const.MAX_HANDSHAKE_LENGTH:
                self.rxHandshakeLength = len(data)
                data.drain(self.rxHandshakeLength)
                self.protoState = const.ST_AUTH_FAILED
                log.debug("Authentication failed after having received %d"
                          " bytes, will close after peer trips the threshold."
                          % self.rxHandshakeLength)
                return

Add const.MAX_HANDSHAKE_LENGTH, const.ST_WAIT_FOR_AUTH, self.rxHandshakeLength as appropriate (NB: Dry code. May have errors, but I hope the idea is clear.). This avoids doing any data processing after it has been clear that the handshake will never succeed and jettisons the invalid data immediately.

(At first I was like "well the changes that I'm about to suggest will be trivial I can just inline them in the bug", but maybe I would have been better off writing this as a patch. Sorry.)

Edit: Minor syntax issues

Last edited 6 years ago by yawning (previous) (diff)

comment:5 in reply to:  4 Changed 6 years ago by phw

Replying to yawning:

In util.locateMark(mark) change "index = payload.find(mark)" to be bounded like :

    index = payload.find(mark, 0, const.MAX_PADDING_LENGTH +
                                  const.MARK_LENGTH)

Done.

Without the change, it is still possible to accept out-of-spec packets.

The current code no longer buffers data forever, but it still continues to process the handshake till the threshold is reached. In ScrambleSuitTransport.receivedDownstream consider doing something like:

    ...

Add const.MAX_HANDSHAKE_LENGTH, const.ST_WAIT_FOR_AUTH, self.rxHandshakeLength as appropriate (NB: Dry code. May have errors, but I hope the idea is clear.). This avoids doing any data processing after it has been clear that the handshake will never succeed and jettisons the invalid data immediately.

Done.

I committed the changes on top of my bug_11092 branch. I also increased the closing threshold to the (somewhat arbitrary) value MAX_HANDSHAKE_LENGTH * 5. I guess that's not a problem since we no longer process data after authentication has failed.

comment:6 Changed 6 years ago by phw

For the record, Yawning: Feel free to teach obfsclient different behaviour. That's why I kept the spec vague. It's probably fine (and perhaps even desirable) that our implementations differ slightly. After all, it makes it harder to come up with one filter to rule them all.

comment:7 Changed 6 years ago by asn

Code looks good.
The new behavior of scramblesuit seems reasonable.

FWIW, I've only looked at the code and haven't tested it in obfsproxy yet.

comment:8 Changed 6 years ago by phw

Resolution: fixed
Status: needs_reviewclosed

Merged to master and pushed. Thanks.

Note: See TracTickets for help on using tickets.