Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 4096] Wireshark's RADIUS retry detection incorrectly tags unrelated RADIUS pac

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4096





--- Comment #1 from Armen Vardanian <armenv@xxxxxxxxxxxxxxxxxxx>  2009-10-06 03:31:49 PDT ---
After some code review (it's my first time through the wireshark code...) I
found the code block that I think may be causing the incorrect duplicate
detection:
(taken from
http://anonsvn.wireshark.org/viewvc/trunk/epan/dissectors/packet-radius.c?view=markup)
function: dissect_radius()
-------------
/*
 * XXX - can we just use NO_ADDR_B?  Unfortunately,
 * you currently still have to pass a non-null
 * pointer for the second address argument even
 * if you do that.
 */
conversation = find_conversation(pinfo->fd->num, &pinfo->src, &null_address,
pinfo->ptype, pinfo->srcport,pinfo->destport, 0);
if (conversation == NULL)
{
        /* It's not part of any conversation - create a new one. */
        conversation = conversation_new(pinfo->fd->num, &pinfo->src,
&null_address, pinfo->ptype, pinfo->srcport,pinfo->destport, 0);
}
---------------------

for starters, I think the conversation matching is too broad by not providing a
destination IP address to match on. Although UDP makes no assumptions, RADIUS
does make an assumption as it relies upon a known and predictable IP Address
header for shared secret matching. If the UDP conversation has its IP header
modified, then you can argue that the RADIUS transaction _shouldn't_ match any
existing conversation (because something in the UDP chain is impersonating a
RADIUS server/client). Another case is when multiple proxies are contacted. If
you have 2 proxies with 256 transactions outstanding each, then I think you
would have 100% overlap of transactions with the implemented logic.

----------------------

/* Prepare the key data */
radius_call_key.code = rh.rh_code;
radius_call_key.ident = rh.rh_ident;
radius_call_key.conversation = conversation;
radius_call_key.req_time = pinfo->fd->abs_ts;
----------------------
I'm not entirely convinced that the radius_call_key data is enough to uniquely
identify a radius transaction for duplicate detection purposes. All we have is
the transaction type and the RADIUS transaction ID, which in high load is
likely to be overlapping with another transaction somewhere. given the
conversation identification issues described above, I think this has a knock-on
affect in falsely identifying radius transactions as duplicate.
----------------------

/* Look up the request */
radius_call = g_hash_table_lookup(radius_calls, &radius_call_key);
if (radius_call != NULL)
{
        /* We've seen a request with this ID, with the same
           destination, before - but was it *this* request? */
        if (pinfo->fd->num != radius_call->req_num)
        {
                /* No, so it's a duplicate request. Mark it as such. */
                rad_info->is_duplicate = TRUE;
                rad_info->req_num = radius_call->req_num;
                if (check_col(pinfo->cinfo, COL_INFO))
                {
                        col_append_fstr(pinfo->cinfo, COL_INFO, ", Duplicate
Request ID:%u",    rh.rh_ident);
                }
-----------------------
I don't see where the destination is being checked here in the code above.
g_hash_table_lookup never gets supplied with any information that would make it
aware of the IP destination (as we used &null_address for the original
conversation match). This then leads to a false flag as duplicate and the
erroneous report of the duplicate request, which is the symptom I'm seeing in
my trace processing.


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.