Opened 10 years ago

Closed 8 years ago

#2791 enhancement closed duplicate (duplicate)

[Patch] Bad boundary check when parsing a Socks4a packet

Reported by: adam.lofts Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, Thijs Triemstra Branch:
Author:

Description

The current socks parsing code checks for one null after the header. However if the packet is a socks 4a packet the code assumes another null after the hostname. Thus the code breaks if the buffer does not yet contain a whole packet.

Attachments (2)

socks_bad_bounday_check.patch (3.0 KB) - added by adam.lofts 10 years ago.
Patch to check for two null bytes after the header
SOCKSv4a_support.patch (5.7 KB) - added by jesstess 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 10 years ago by adam.lofts

Patch to check for two null bytes after the header

comment:1 Changed 10 years ago by Glyph

Owner: changed from Glyph to adam.lofts

Nice to know somebody is using that code!

If you'd like to speed this along, please read http://twistedmatrix.com/trac/wiki/TwistedDevelopment. To start with, can you write some unit tests for the behavior you are changing so this bug doesn't resurface?

comment:2 Changed 8 years ago by jesstess

Owner: changed from adam.lofts to jesstess

comment:3 Changed 8 years ago by jesstess

Cc: jesstess added
Keywords: review added
Owner: jesstess deleted
Type: defectenhancement

This really amounts to a feature request to support SOCKSv4A, I think.

The new patch fixes up a small initialization problem in adam.lofts' code as well as some cosmetic issues and adds a unit test in twisted/test/test_socks.

comment:4 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to jesstess

twisted/test/test_socks.py needs a copyright header.

Changed 8 years ago by jesstess

Attachment: SOCKSv4a_support.patch added

comment:5 Changed 8 years ago by jesstess

Keywords: review added
Owner: jesstess deleted

So it does. Patch updated accordingly.

comment:6 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to jesstess

Humm we had some overlap here. #3886 and #3920 are pretty strongly related. At least one of those may be a duplicate of this. I reviewed this code anyway (I forgot about those tickets :), but it probably makes sense to take a look at the branch and patch associated with those tickets to make sure that continuing work here makes the most sense (all the redundant effort may have happened already, or there might be stuff worth taking from those tickets).

  1. On the new line 74 of twisted/protocols/socks.py, the index call can raise a ValueError. This case isn't covered by the unit tests, though; it should be.
  2. The variables idx1 and idx2 in SOCKSv4Incoming.dataReceived are not very descriptively named. :) These seem to be the ending positions of the user id and domain fields.
  3. Can you replace the string.split usage in this code with the split str method?
  4. In the unit tests, please use assertEquals rather than assertEqual (hm, is that in the coding standard yet? I don't see it.)

comment:7 Changed 8 years ago by jesstess

Resolution: duplicate
Status: newclosed

Fixed in #3886.

comment:8 Changed 7 years ago by <automation>

Owner: jesstess deleted
Note: See TracTickets for help on using tickets.