bug in calling tcp_output_fill_options() ?

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

bug in calling tcp_output_fill_options() ?

Hannes Gredler
hi,

I have run into a bug in tcp_output_fill_options() while trying to add TCP-AO (RFC5925) support.

--

it is my understanding that fourth argument in the callstack is the number of SACKs.

static void
tcp_output_fill_options(const struct tcp_pcb *pcb, struct pbuf *p, u8_t optflags, u8_t num_sacks)
                                                                                  ^^^^^^^^^^^^^^^

however there is still 4 calls which pass in the total option length which gets then
misinterpreted as number of SACKs. which in my case caused a buffer overflow in
tcp_output_fill_options()

Q: shouldn't all those arguments reset to zero as per below patch ?

thanks,

/hannes


diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
index 4e7cbb09..63c37448 100644
--- a/src/core/tcp_out.c
+++ b/src/core/tcp_out.c
@@ -2034,7 +2034,7 @@ tcp_rst(const struct tcp_pcb *pcb, u32_t seqno, u32_t ackno,
     LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
     return;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

 #ifdef LWIP_CUSTOM_HOOK_TCP_OUT_ADD
     LWIP_CUSTOM_HOOK_TCP_OUT_ADD(p, pcb);
@@ -2132,7 +2132,7 @@ tcp_keepalive(struct tcp_pcb *pcb)
                 ("tcp_keepalive: could not allocate memory for pbuf\n"));
     return ERR_MEM;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);
   err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);

   LWIP_DEBUGF(TCP_DEBUG, ("tcp_keepalive: seqno %"U32_F" ackno %"U32_F" err %d.\n",
@@ -2214,7 +2214,7 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
   if (TCP_SEQ_LT(pcb->snd_nxt, snd_nxt)) {
     pcb->snd_nxt = snd_nxt;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

   err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);

@@ -2259,7 +2259,7 @@ tcp_custom_rst(s8_t *instName, u32_t seqno, u32_t ackno,
     LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
     return;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

   p->instName = instName;

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: bug in calling tcp_output_fill_options() ?

goldsimon@gmx.de
Am 01.04.2020 um 11:05 schrieb Hannes Gredler:

> hi,
>
> I have run into a bug in tcp_output_fill_options() while trying to add TCP-AO (RFC5925) support.
>
> --
>
> it is my understanding that fourth argument in the callstack is the number of SACKs.
>
> static void
> tcp_output_fill_options(const struct tcp_pcb *pcb, struct pbuf *p, u8_t optflags, u8_t num_sacks)
>                                                                                   ^^^^^^^^^^^^^^^
>
> however there is still 4 calls which pass in the total option length which gets then
> misinterpreted as number of SACKs. which in my case caused a buffer overflow in
> tcp_output_fill_options()
>
> Q: shouldn't all those arguments reset to zero as per below patch ?

Seems like you're right, yes.

Regards,
Simon

>
> thanks,
>
> /hannes
>
>
> diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
> index 4e7cbb09..63c37448 100644
> --- a/src/core/tcp_out.c
> +++ b/src/core/tcp_out.c
> @@ -2034,7 +2034,7 @@ tcp_rst(const struct tcp_pcb *pcb, u32_t seqno, u32_t ackno,
>      LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
>      return;
>    }
> -  tcp_output_fill_options(pcb, p, 0, optlen);
> +  tcp_output_fill_options(pcb, p, 0, 0);
>
>  #ifdef LWIP_CUSTOM_HOOK_TCP_OUT_ADD
>      LWIP_CUSTOM_HOOK_TCP_OUT_ADD(p, pcb);
> @@ -2132,7 +2132,7 @@ tcp_keepalive(struct tcp_pcb *pcb)
>                  ("tcp_keepalive: could not allocate memory for pbuf\n"));
>      return ERR_MEM;
>    }
> -  tcp_output_fill_options(pcb, p, 0, optlen);
> +  tcp_output_fill_options(pcb, p, 0, 0);
>    err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);
>
>    LWIP_DEBUGF(TCP_DEBUG, ("tcp_keepalive: seqno %"U32_F" ackno %"U32_F" err %d.\n",
> @@ -2214,7 +2214,7 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
>    if (TCP_SEQ_LT(pcb->snd_nxt, snd_nxt)) {
>      pcb->snd_nxt = snd_nxt;
>    }
> -  tcp_output_fill_options(pcb, p, 0, optlen);
> +  tcp_output_fill_options(pcb, p, 0, 0);
>
>    err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);
>
> @@ -2259,7 +2259,7 @@ tcp_custom_rst(s8_t *instName, u32_t seqno, u32_t ackno,
>      LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
>      return;
>    }
> -  tcp_output_fill_options(pcb, p, 0, optlen);
> +  tcp_output_fill_options(pcb, p, 0, 0);
>
>    p->instName = instName;
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: bug in calling tcp_output_fill_options() ?

Hannes Gredler
hi simon,

thanks for the quick review - just figured that i was taking the patch from the wrong tree;
correct one is below and should apply cleanly;

/hannes

diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
index 7d7ea5ee..bfb033b1 100644
--- a/src/core/tcp_out.c
+++ b/src/core/tcp_out.c
@@ -2002,7 +2002,7 @@ tcp_rst(const struct tcp_pcb *pcb, u32_t seqno, u32_t ackno,
     LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
     return;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

   MIB2_STATS_INC(mib2.tcpoutrsts);

@@ -2096,7 +2096,7 @@ tcp_keepalive(struct tcp_pcb *pcb)
                 ("tcp_keepalive: could not allocate memory for pbuf\n"));
     return ERR_MEM;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);
   err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);

   LWIP_DEBUGF(TCP_DEBUG, ("tcp_keepalive: seqno %"U32_F" ackno %"U32_F" err %d.\n",
@@ -2178,7 +2178,7 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
   if (TCP_SEQ_LT(pcb->snd_nxt, snd_nxt)) {
     pcb->snd_nxt = snd_nxt;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

   err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);

On Wed, Apr 01, 2020 at 11:19:27AM +0200, [hidden email] wrote:
| Am 01.04.2020 um 11:05 schrieb Hannes Gredler:
| > hi,
| >
| > I have run into a bug in tcp_output_fill_options() while trying to add TCP-AO (RFC5925) support.
| >
| > --
| >
| > it is my understanding that fourth argument in the callstack is the number of SACKs.
| >
| > static void
| > tcp_output_fill_options(const struct tcp_pcb *pcb, struct pbuf *p, u8_t optflags, u8_t num_sacks)
| >                                                                                   ^^^^^^^^^^^^^^^
| >
| > however there is still 4 calls which pass in the total option length which gets then
| > misinterpreted as number of SACKs. which in my case caused a buffer overflow in
| > tcp_output_fill_options()
| >
| > Q: shouldn't all those arguments reset to zero as per below patch ?
|
| Seems like you're right, yes.
|
| Regards,
| Simon
|
| >
| > thanks,
| >
| > /hannes
| >
| >
| > diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
| > index 4e7cbb09..63c37448 100644
| > --- a/src/core/tcp_out.c
| > +++ b/src/core/tcp_out.c
| > @@ -2034,7 +2034,7 @@ tcp_rst(const struct tcp_pcb *pcb, u32_t seqno, u32_t ackno,
| >      LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
| >      return;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >
| >  #ifdef LWIP_CUSTOM_HOOK_TCP_OUT_ADD
| >      LWIP_CUSTOM_HOOK_TCP_OUT_ADD(p, pcb);
| > @@ -2132,7 +2132,7 @@ tcp_keepalive(struct tcp_pcb *pcb)
| >                  ("tcp_keepalive: could not allocate memory for pbuf\n"));
| >      return ERR_MEM;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >    err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);
| >
| >    LWIP_DEBUGF(TCP_DEBUG, ("tcp_keepalive: seqno %"U32_F" ackno %"U32_F" err %d.\n",
| > @@ -2214,7 +2214,7 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
| >    if (TCP_SEQ_LT(pcb->snd_nxt, snd_nxt)) {
| >      pcb->snd_nxt = snd_nxt;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >
| >    err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);
| >
| > @@ -2259,7 +2259,7 @@ tcp_custom_rst(s8_t *instName, u32_t seqno, u32_t ackno,
| >      LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
| >      return;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >
| >    p->instName = instName;
| >
| > _______________________________________________
| > lwip-devel mailing list
| > [hidden email]
| > https://lists.nongnu.org/mailman/listinfo/lwip-devel
| >
|
|
| _______________________________________________
| lwip-devel mailing list
| [hidden email]
| https://lists.nongnu.org/mailman/listinfo/lwip-devel

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel