scramblesuit should make sure that handshake padding is less than MAX_PADDING_LENGTH

added component::archived/obfsproxy owner::asn priority::medium pt-scramblesuit resolution::fixed status::closed type::defect labels

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.

I implemented a spec and code fix in branch bug_11092: https://gitweb.torproject.org/user/phw/scramblesuit.git/shortlog/refs/heads/bug_11092

Trac:
Status: new to needs_review

Trac:
Cc: N/A 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)

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

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.

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.

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.

Merged to master and pushed. Thanks.

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

closed

mentioned in issue #11100 (closed)

mentioned in issue #11271 (closed)

scramblesuit should make sure that handshake padding is less than MAX_PADDING_LENGTH

Child items ...

Activity