buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28606, PPP: Possible bug in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #28606: PPP: Possible bug in fsm.c:fsm_input()

Submitted by:  Iordan Neshev <iordan_neshev>
Submitted on:  Thu 14 Jan 2010 12:48:41 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Invalid
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission Jump to the original submission)

Thu 14 Jan 2010 05:32:20 PM UTC, comment #6:

Pls close it as invalid.

Iordan Neshev <iordan_neshev>
Thu 14 Jan 2010 04:45:04 PM UTC, comment #5:

Logical left to right evaluation as well as stopping evaluation as soon as the result is known is guaranteed by the standard.

It allows stuff like this to work:

if (ptr != NULL && ptr->member != something )

Bill Auerbach <billauerbach>
Thu 14 Jan 2010 04:01:26 PM UTC, comment #4:

You're right. fsm_input() is called only by ipcp and lcp protocols. They both define callbacks. I thought that every ppp protocol (f.ex upap) can call fsm_input(). If this was true, this would be a bug. I'm glad I was not right :)

Iordan Neshev <iordan_neshev>
Thu 14 Jan 2010 03:46:53 PM UTC, comment #3:

From reading the code, I think that 'callbacks' is always != NULL (initialized in ipcp_init() as well as in lcp_init()) and thus the check is not necessary. f->callbacks is also used in many other places without checking for NULL.

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 14 Jan 2010 02:20:47 PM UTC, comment #2:

>Since the code is evaluated from left to right (don't know if
>that's really part of the C standard, but show me one compiler
>that does it differently),


I don't know if this is part of the standard and I've never relied on the order to be from left to right. If you're right, the code you propose looks OK.

>Anyway, shouldn't we tell the pppd developer(s) about this bug >instead of discussing it here?


I sent him an email with link to this thread.

Btw I've never come up to this (default) case and can not confirm with test that this is a bug.

Iordan Neshev <iordan_neshev>
Thu 14 Jan 2010 02:02:36 PM UTC, comment #1:

Since the code is evaluated from left to right (don't know if that's really part of the C standard, but show me one compiler that does it differently), the following code will probably be enough:

if(!f->callbacks ||
!f->callbacks->extcode ||
!(*f->callbacks->extcode)(f, code, id, inp, len))

Anyway, shouldn't we tell the pppd developer(s) about this bug instead of discussing it here?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 14 Jan 2010 12:48:41 PM UTC, original submission:

This is a snippet of this function
void fsm_input(fsm f, u_char inpacket, int l)
{
...
switch (code) {// CONFREQ, CONFACK, CONFNAK ...
....

default:
FSMDEBUG((LOG_INFO, "fsm_input(%s): default: \n", PROTO_NAME(f)));

if( !f->callbacks->extcode ||
!(*f->callbacks->extcode)(f, code, id, inp, len) ) {
fsm_sdata(f, CODEREJ, ++f->id, inpacket, len + HEADERLEN);
}

The code in the default section looks suspicious to me.
If I understand it right ->extcode is pointer to a default callback function which should handle an incomming packet with code (described in fsm.h) which could not be recognized. If this default handler was also unable to handle the packet, then the packets is rejected by calling fsm_sdata(f, CODEREJ, .....);.

Every PPP protocol can have such callback, but currently
only ipcp and lcp define one - ipcp_callbacks and lcp_callbacks respectively.

Initialization:
f->callbacks = &ipcp_callbacks; for ipcp
f->callbacks = &lcp_callbacks; for lcp
f->callbacks = NULL for all other ppp protocols

ipcp_callbacks->extcode = NULL;
lcp_callbacks->extcode = lcp_extcode()

Since extcode for ipcp is NULL, all IPCP packets with unknown code (i.e. not handled by fsm_input) are immediately rejected

Unknown LCP packets are passed to lcp_extcode(). If it too fails to recognize the code, the packet is rejected as desribed.

Did I figured it out right?

If yes, then there is a possibility that
1. f->callbacks->extcode is evaluated when f->callbacks is NULL
2.(*f->callbacks->extcode)(f, code, id, inp, len)
could be called when f->callbacks->extcode is NULL.

Both would lead to reading from undefined memory location and/or execution of random code, which may be outside the memory space.

According to me the code should behave like that:

default:
{
u8_t send_rej = 0;
FSMDEBUG((LOG_INFO, "fsm_input(%s): default: \n", PROTO_NAME(f)));

if (!f->callbacks)
{
send_rej = 1;
}
else
{
if( !f->callbacks->extcode)
{
send_rej = 1;
}
else
{
if (!(*f->callbacks->extcode)(f, code, id, inp, len) )
{
send_rej = 1;
}
}
}

if (send_rej) {
fsm_sdata(f, CODEREJ, ++f->id, inpacket, len + HEADERLEN);
}
}

Iordan Neshev <iordan_neshev>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by billauerbach (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by iordan_neshev (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 14 Jan 2010 07:09:51 PM UTCgoldsimonStatusNone=>Invalid
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1