Opened 5 months ago

Closed 5 months ago

#26060 closed defect (fixed)

Invalid [Length] field when receiving RELAY cells via stem.client.Circuit

Reported by: plcp Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Normal Keywords: client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When receiving data via the stem.client.Circuit.send method, the current behavior is to unpack the RELAY cell before "repacking-it" in order to get a "raw bytes" representation of the ciphertext. The ciphertext is then decrypted and the cell repacked.

See https://gitweb.torproject.org/stem.git/tree/stem/client/__init__.py#n250

However, when a RELAY cell is unpacked, its [Length] field is used as-is: hence, a encrypted RELAY may see its content needlessly truncated. Note that here the original value of the [Length] field is not retained, thus we'll later be unable to retrieve it.

See https://gitweb.torproject.org/stem.git/tree/stem/client/cell.py#n347

Finally, when a RELAY cell is packed, the [Length] field is computed from the length of the data stored. As this field will later be decrypted, we have an invalid [Length] field for every RELAY cell we receive via the stem.client.Circuit send method.

See https://gitweb.torproject.org/stem.git/tree/stem/client/cell.py#n335

(note that was able to fix my local copy of stem by adding an extra keyword argument length=None to RelayCell's __init__ method that defaults to len(data) and by adding an extra keyword argument "encrypted" to RelayCell's _unpack class method that defaults to False in order to handle the discrepancy between the ciphertext/plaintext cases)

Child Tickets

Change History (6)

comment:1 Changed 5 months ago by atagar

Hi plcp, thanks for pointing this out! As you can probably tell the song and dance done by stem.client is a bit confusing. I stared at this for a while attempting to grok what you found but I probably haven't had enough caffeine.

Mind a test that demonstrates the bug?

comment:2 Changed 5 months ago by atagar

Keywords: client added

comment:3 Changed 5 months ago by plcp

Here is a little demo:

git clone "https://git.torproject.org/stem.git" stem-client
ln -s stem-client/stem
virtualenv venv
source venv/bin/activate
pip install -r ./stem-client/requirements.txt
tor PublishServerDescriptor 0 AssumeReachable 1 ExitRelay 0 ProtocolWarnings 1 SafeLogging 0 LogTimeGranularity 1 PidFile "$(mktemp)" SOCKSPort 0 ContactInfo none@example.com
 DataDirectory "$(mktemp -d)" ORPort 9050 DirPort 9051 Log "err stderr" &

wget https://raw.githubusercontent.com/plcp/tor-scripts/master/stem_26060_issue.py
python stem_26060_issue.py

You should obtain something like that:

Before repacking:
        Cell headers:
         - circ_id:     80000000
         - command:     03
        RELAY headers:
         - command:     6e
         - recognized:  eab1
         - stream_id:   c650
         - digest:      7e0f68b7
         - length:      68e4

After repacking:
        Cell headers:
         - circ_id:     80000000
         - command:     03
        RELAY headers:
         - command:     6e
         - recognized:  eab1
         - stream_id:   c650
         - digest:      7e0f68b7
         - length:      01f2             !! corrupted !!

After decryption:
        Cell headers:
         - circ_id:     80000000
         - command:     03
        RELAY headers:
         - command:     04               RELAY_CONNECTED
         - recognized:  0000
         - stream_id:   0001
         - digest:      a0b49e85
         - length:      6916             !! corrupted !!

Digest (from the RELAY cell):   a0b49e85
Digest (computed length):       db7932a8
Digest (expected length):       a0b49e85

I've managed to fix this issue as follows:
https://github.com/plcp/stem-client/commit/0e2ec2627df05ffcbd2d93be52b862e111bb400b

(note that I'm only using stem.client.cell and thus haven't modified stem.client.Circuit accordingly)

comment:4 Changed 5 months ago by atagar

Hi plcp, sorry about the delay and thanks for the catch! Stared at this a few hours and finally pushed a couple fixes. How does this look?

https://gitweb.torproject.org/stem.git/commit/?id=72ba79d
https://gitweb.torproject.org/stem.git/commit/?id=e3fab7b

comment:5 Changed 5 months ago by plcp

You documented the header_size too, nice!

At first glance, it seems good to me and that'll fix the issue – without modifying the stem.client.cell.RelayCell API.

comment:6 Changed 5 months ago by atagar

Resolution: fixed
Status: newclosed

Great! Thanks plcp. If you run into any further issues just let me know. And once again, thanks for the catch. :P

Note: See TracTickets for help on using tickets.