Wireshark · Ethereal-dev: Re: [Ethereal-dev] patch for packet-smb.c (Locking_AndX)

["Luis Claudio R. Goncalves" <lclaudio@xxxxxxxxxxxxxxxx>]

Hello!

Thanks for the nice explanation! :)

But... reading it over and over, I can see you are pointing to the patch I
wrote. Look at the following example in a x86 CPU:

If you have the value 0xAABBCCDDEEFF1122 for the offset or length value,
things would happen this way:

Value:   AA BB CC DD EE FF 11 22

Since the CPU is little endian, it would be stored in memory as:

Memory:  22 11 FF EE DD CC BB AA
         |           |
         |           +-- offset + 4    | from variables tvb and
         +-- offset                    | offset

Understanding how tvb_get_letohl() and pletohl() works, it is easier to see
that the actual code - in the URL you pointed below - does the following:

val = tvb_get_letohl(tvb, offset);

it means val = 0xEEFF1122

and then it stores in buf, positions 0 to 3, the values corresponding to le
order:

buf = 22 11 FF EE

right after that, val receives a new value:

val=tvb_get_letohl(tvb, offset+4);

which means val = 0xAABBCCDD

and stores this value in le order in buf, the same way it happened right
before, from positions 4 to 7. In the end of these operations, the value
of buf is:

buf = 22 11 FF EE DD CC BB AA

and it is exactly the same order it would be if stored as a 64bit little
endian integer.

As you stated before, the protoco says that the value should be sliced in
two 32bit little endian integers and the most significative of then should
be placed first (the big endian ordere for 32bit integers you've mentioned
before). If I got this all correctly, the current behavior is not compliant
with what you described before.

What my patch does is simply switch the 32bit integers used.

Sorry for being picky but please let me know if I got crazy :))

Thanks again for your time,
Luis


On Wed, Dec 03, 2003 at 09:43:31PM +1100, Ronnie Sahlberg wrote:
| I am sorry but I was not clear.
| 
| AFAICS your patch reverses the patch I added to packet-smb.c # 1.366
| and changes it back into treating this field as a 64 bit integer in
| LittleEndian
| format.
| 
| This is how ethereal used to work prior to the patch mentioned above, and
| incidentally how
| at least one other popular network monitoring software still works today.
| 
| The patch was added since captures were analyzed that showed that at least
| the MS implementation that generated those captures
| did not store this field as a 64 bit integer in LittleEndian format.
| In fact, that MS implementation stored it as TWO 32 bit integers each in
| LittleEndian format but these two 32 bit integers were
| stored in the reverse order.
| 
| This was prooved by looking at MS implementations and is why ethereal was
| patched. While it is not impossible that MS might reorder the individual
| fields
| in a PDU between different dialects, this has not really been observed
| before.
| 
| 
| "
| The length and offset for large file support for LockingAndX was broken.
| We treated this as just a normal 64bit integer in LittleEndian format.
| However, this is actually 2   32 bit integers, each in LittleEndian format
| but the two 32 bit fields are stored in BigEndian format relative to each
| other.
| Since we dont do 64 bit aritmetic I had to convert the field to FT_STRING as
| well
| so sorry, no creative len>xxx    filters anymore.   but at least we present
| the data in the correct way in the tree pane.
| We didnt see this one earlier since most locking_andx requests are probably
| for offset : 0   and length: -
| Funnily enough it seems that certain popular commercial products have the
| same bug as ethereal had up until 5 minutes ago.
| "
| 
| 
| What I mean is,   there were a specific reason the behaviour of ethereal
| changed in this patch :
| to be compatible with at least one known dialect of MS SMB.
| 
| 
| I would be interested in seeing capture files created between MS clients and
| MS servers before I think the patch should be applied.
| 
| 
| Can you generate a capture file between MS clients and MS servers that show
| this behaviour and that ethereal is wrong?
| You might need two MS clients and one MS server   and testing exclusive
| locking for lock ranges that would overlap
| if it is a 64 bit integer and would NOT overlap if it were not a 64 bit
| integer.
| (based on the assumption that two exclusive read/write locks that overlap
| are not permitted and can not exist at the same time)
| 
| 
| 
| 
| 
| ----- Original Message ----- 
| From: "Luis Claudio R. Goncalves"
| Sent: Saturday, November 29, 2003 2:44 AM
| Subject: Re: [Ethereal-dev] patch for packet-smb.c (Locking_AndX)
| 
| 
| > Hi!
| >
| > Sorry for taking so long to reply your message...
| >
| > I wrote a small windows program in C (compiled with bcc55) and captured
| > some test sessions with ethereal.
| >
| > The values for both offset and range are displayed correctly (as requested
| > by the windows client) in the samba and the winnt server but not in
| > ethereal. With that small patch I sent you before, values are shown
| > correctly. I'm not sure what kind of information or proof you want me to
| > send... anyways, the testing program is attached.
| >
| > Regards,
| > Luis
| >
| > On Sun, Nov 09, 2003 at 08:11:22AM +1100, Ronnie Sahlberg wrote:
| > | I.e. the code in ethereal wont be changed unless it can be shown that a
| real
| > | MS implementation does it differently from
| > | ethereal.
| > |
| > |
| > |
| > | ----- Original Message ----- 
| > | From: "Ronnie Sahlberg"
| > | Sent: Sunday, November 09, 2003 8:08 AM
| > | Subject: Re: [Ethereal-dev] patch for packet-smb.c (Locking_AndX)
| > |
| > |
| > | > I am not sure this fix is correct.
| > | >
| > | >
| > |
| http://www.ethereal.com/cgi-bin/viewcvs.cgi/ethereal/packet-smb.c.diff?r1=1.365&r2=1.366
| > | > The reason it was changed  FROM what you suggest TO what it is
| corrently
| > | > was based on what real Microsoft Clients/Servers report in the call.
| > | >
| > | >
| > | > ----- Original Message ----- 
| > | > From: "Luis Claudio R. Goncalves"
| > | > Sent: Saturday, November 08, 2003 7:02 AM
| > | > Subject: [Ethereal-dev] patch for packet-smb.c (Locking_AndX)
| > | >
| > | >
| > | > > Hi!
| > | > >
| > | > > I've been playing around with locks in samba and I've seen that the
| > | offset
| > | > > and length reported by ethereal for large file locks are not the
| same
| > | I've
| > | > > requested and thus not the same reported by samba logs.
| > | > >
| > | > > The locking offset and length are 64bit integers sent as OffsetHigh
| > | > (32bit)
| > | > > and OffsetLow (32bit). The glitch seems to be only the mix between
| high
| > | > and
| > | > > low 32bit words.
| > | > >
| > | > > The patch attached just does a little change in
| > | > > dissect_locking_andx_request(), file packet-smb.c
| > | > >
| > | > > I tried it with ethereal-0.9.10 and I've seen that this code still
| be
| > | the
| > | > > same in 0.9.16.
| > | > >
| > | > > Regards,
| > | > > Luis
| > | > >
| > | > > PS: I'm not subscribed to this mailing list... :)
| > | > > -- 
| > | >
| > | > _______________________________________________
| > | > Ethereal-dev mailing list
| > | > Ethereal-dev@xxxxxxxxxxxx
| > | > http://www.ethereal.com/mailman/listinfo/ethereal-dev
| > ---end quoted text---
| >
| > -- 
| > [ Luis Claudio R. Goncalves                  lclaudio@xxxxxxxxxxxxxxxx ]
| > [ Fingerprint:   4FDD B8C4 3C59 34BD 8BE9  2696 7203 D980 A448 C8F8    ]
| > [ Msc has come!!!! - Conectiva HA Team - Gospel User - Linuxer - !Java ]
| > [ Fault Tolerance - Real-Time - Distributed Systems - IECLB - IS 40:31 ]
| > [ LateNite Programmer        --  My Utmost for His Highest  --         ]
| >
| >
---end quoted text---

-- 
[ Luis Claudio R. Goncalves                  lclaudio@xxxxxxxxxxxxxxxx ]
[ Fingerprint:   4FDD B8C4 3C59 34BD 8BE9  2696 7203 D980 A448 C8F8    ]
[ Msc has come!!!! - Conectiva HA Team - Gospel User - Linuxer - !Java ]
[ Fault Tolerance - Real-Time - Distributed Systems - IECLB - IS 40:31 ]
[ LateNite Programmer        --  My Utmost for His Highest  --         ]