Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 8240] Dissector for the OpenVPN Protocol

[bugzilla-daemon@xxxxxxxxxxxxx]

Manuel Hofer changed bug 8240

What	Removed	Added
Attachment #9839 is obsolete		1
Attachment #9851 Flags		review_for_checkin?

Comment # 9 on bug 8240 from Manuel Hofer

Created attachment 9851 [details]
openvpn dissector patch

(In reply to comment #7)
> Ok. Looks like a solid base, altho some work needed....
> 
> Some comments (in no particular order);
> 
> 1. global protocol_tcp is referenced in dissect_openvpn() before being set in
>    dissect_openvpn_msg(0.
> 
>    It would seem to me that dissect_openvpn() needs to use the same test
>    as in dissect_openvpn_msg(). Also, if so, then protocol_tcp need not be
>    a global.
you are right, fixed

> 2. __attribute__ is for gcc. For portability (e.g., to compile on Windows)
>    use _U_ instead. _U_ is defined appropriately for different 
>    platforms.
> 
> 3. ssl_handle = find_dissector("ssl"); should be done only
>    once with ssl_handle being a static global.
> 
> 4. It would be appreciated if you would strip trailing whitespace.
> 
> 5. Please provide a reference to the protocol specification (in a comment
>    in the source).
> 
> 6. Copyright:
> 
>    - You should add a date (year).
>    - I'm guessing that the copyright is by the 5 individuals named and does
>      not include the University. Is this correct ?
>      If any case, the " *    Semester Project ... " text should probably be
> more
>      clearly separated from the (c) paragraph.
> 
> 7. issues detected by fix-encoding-args:
> 
>    proto_tree_add_item(data_tree, hf_openvpn_data, next_tvb, offset, -1, \
>         [[ENC_BIG_ENDIAN]-->[ENC_NA]]);
> 
>    proto_tree_add_item(openvpn_tree, hf_openvpn_hmac, tvb, offset, \
>         tls_auth_hmac_size, [[ENC_BIG_ENDIAN]-->[ENC_NA]]);
> 
>    tvb_get_bits32(tvb, offset*8+32, 32, [[FALSE]-->[ENC_BIG_ENDIAN]]);
> 
>    tvb_get_bits32(tvb, offset*8, 32, [[FALSE]-->[ENC_BIG_ENDIAN]]);
> 
> 8. @ ~ line 305 tvb_length_remaining() is called 3x.
>    It would be a bit nicer to call it once to call it just once).
2-8 fixed.

> 9. The following statement bothered me a bit:
> 
>     msg_lastframe = (tvb_length_remaining(tvb, offset) == 100 ? FALSE :
> TRUE);
> 
>     I believe tt works because the tvb (either UDP or TCP) will have
>     exactly one openvpn PDU; (UDP: by definition; TCP: because of the use 
>     of tcp_dissect_pdus()).
> 
>     I suggest adding a comment so that a casual reader of the code will
>     understand why the test works.
this is because openvpn itself only sends 100byte of ssl data at a time, and
the last fragment of such a transmission is always smaller than 100bytes. i
added the comment.

> 10. Only prefs vars should be under the /* Preferences */ comment.
>     The other stuff, ett vars, etc should be moved separately as appropiate.
> 
> 11. proto_tree_add_item(data_tree, hf_openvpn_data, next_tvb, offset, \
>          tvb_length_remaining(next_tvb, offset), ENC_BIG_ENDIAN);
> 
>     -1 can be used instead of tvb_length_remaining()
9-11 fixed.

> 12.  fragment_msgid use looks problematical:
>     - never initialized to zero (except after a PDU is re-assembled and
>       for the initial use of the dissector).
>       (This presumably might be handled by initializing the variable to 0 in
>       the init routine)
> . 
>     - *However*: storing fragment_msgid as a global would seem to me to
>                be a non-starter.
> 
>       Reason: I presume multiple UDP (?sessions) each with a different
>               msg_sessionid value can occur simultaneously.
> 
>               If so, you'll need to keep the fragment_msgid separately for
>               each sessionid (maybe per sessionid using a hash table or
>               maybe per UDP "conversation" if the UDP src/dest addr/port
>               quadruple will always be unique for a session).
>
i actually didnt think about that, thanks for pointing it out! i added a
conversation to store the message id, if udp is used. 


> 14. tvb_new_subset(tvb, offset, -1, -1)  --> tvb_new_subset_remaining(...)
> 
> 15. Relevant comments in Bug %8239 (wiki update, modelines, AUTHORS...) also
>    apply to this dissector.
> 
> 
> That's it for now. If you have any questions please let us now.

Wiki page will created asap, everything else should be fixed now :)

---

      You are receiving this mail because:

• You are watching all bug changes.