| Message ID | 1548065736-21284-1-git-send-email-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] conntrack: fix ftp ipv4 address substitution | expand |
Thanks for the fix David I sent an alternative fix here: https://patchwork.ozlabs.org/patch/1028908/ Darrell On Mon, Jan 21, 2019 at 2:15 AM David Marchand <david.marchand@redhat.com> wrote: > replace_substring() wants the total size of the string in order to move > the end of the string after the part being replaced. > > When replacing the ipv4 address in repl_ftp_v4_addr(), the remain_size > variable must be updated after replace_substring() has been called, not > before. > Besides, the substraction is off by one, since we need to account for the > ',' that is skipped. > > This goes unnoticed most of the time, unless you choose carefully your > setup and ip addresses. > Example for ftp in passive mode with address 10.1.1.200 DNAT'd to > 10.1.100.1: > > Breakpoint 1, replace_substring (substr=0x1e68497 > "10,1,100,1,241,144).\r\n", substr_size=2 '\002', total_size=20 '\024', > rep_str=0x7fff92ee4e50 "10", rep_str_size=2 '\002') at > lib/conntrack.c:2864 > > Breakpoint 1, replace_substring (substr=0x1e6849a > "1,100,1,241,144).\r\n", substr_size=1 '\001', total_size=19 '\023', > rep_str=0x7fff92ee4e50 "1", rep_str_size=1 '\001') at > lib/conntrack.c:2864 > > Breakpoint 1, replace_substring (substr=0x1e6849c "100,1,241,144).\r\n", > substr_size=3 '\003', total_size=16 '\020', rep_str=0x7fff92ee4e50 "1", > rep_str_size=1 '\001') at lib/conntrack.c:2864 > > Breakpoint 1, replace_substring (substr=0x1e6849e "1,241,144).\r.\r\n", > substr_size=1 '\001', total_size=15 '\017', rep_str=0x7fff92ee4e50 > "200", rep_str_size=3 '\003') at lib/conntrack.c:2864 > > From the "unnated" side, the payload ends up being incorrectly terminated: > > 0x0040: b715 3232 3720 456e 7465 7269 6e67 2050 ..227.Entering.P > 0x0050: 6173 7369 7665 204d 6f64 6520 2831 302c assive.Mode.(10, > 0x0060: 312c 312c 3230 302c 3234 312c 3134 3429 1,1,200,241,144) > 0x0070: 2e0d 2e ... > > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > lib/conntrack.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index f732b9e..8eddc8e 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2817,7 +2817,6 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 > v4_addr_rep, > char *next_delim = memchr(byte_str, ',', 4); > ovs_assert(next_delim); > int substr_size = next_delim - byte_str; > - remain_size -= substr_size; > > /* Compose the new string for this octet, and replace it. */ > char rep_str[4]; > @@ -2825,6 +2824,7 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 > v4_addr_rep, > int replace_size = sprintf(rep_str, "%d", rep_byte); > replace_substring(byte_str, substr_size, remain_size, > rep_str, replace_size); > + remain_size -= substr_size + 1; > overall_delta += replace_size - substr_size; > > /* Advance past the octet and the following comma. */ > -- > 1.8.3.1 > >
diff --git a/lib/conntrack.c b/lib/conntrack.c index f732b9e..8eddc8e 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2817,7 +2817,6 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep, char *next_delim = memchr(byte_str, ',', 4); ovs_assert(next_delim); int substr_size = next_delim - byte_str; - remain_size -= substr_size; /* Compose the new string for this octet, and replace it. */ char rep_str[4]; @@ -2825,6 +2824,7 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep, int replace_size = sprintf(rep_str, "%d", rep_byte); replace_substring(byte_str, substr_size, remain_size, rep_str, replace_size); + remain_size -= substr_size + 1; overall_delta += replace_size - substr_size; /* Advance past the octet and the following comma. */
replace_substring() wants the total size of the string in order to move the end of the string after the part being replaced. When replacing the ipv4 address in repl_ftp_v4_addr(), the remain_size variable must be updated after replace_substring() has been called, not before. Besides, the substraction is off by one, since we need to account for the ',' that is skipped. This goes unnoticed most of the time, unless you choose carefully your setup and ip addresses. Example for ftp in passive mode with address 10.1.1.200 DNAT'd to 10.1.100.1: Breakpoint 1, replace_substring (substr=0x1e68497 "10,1,100,1,241,144).\r\n", substr_size=2 '\002', total_size=20 '\024', rep_str=0x7fff92ee4e50 "10", rep_str_size=2 '\002') at lib/conntrack.c:2864 Breakpoint 1, replace_substring (substr=0x1e6849a "1,100,1,241,144).\r\n", substr_size=1 '\001', total_size=19 '\023', rep_str=0x7fff92ee4e50 "1", rep_str_size=1 '\001') at lib/conntrack.c:2864 Breakpoint 1, replace_substring (substr=0x1e6849c "100,1,241,144).\r\n", substr_size=3 '\003', total_size=16 '\020', rep_str=0x7fff92ee4e50 "1", rep_str_size=1 '\001') at lib/conntrack.c:2864 Breakpoint 1, replace_substring (substr=0x1e6849e "1,241,144).\r.\r\n", substr_size=1 '\001', total_size=15 '\017', rep_str=0x7fff92ee4e50 "200", rep_str_size=3 '\003') at lib/conntrack.c:2864 From the "unnated" side, the payload ends up being incorrectly terminated: 0x0040: b715 3232 3720 456e 7465 7269 6e67 2050 ..227.Entering.P 0x0050: 6173 7369 7665 204d 6f64 6520 2831 302c assive.Mode.(10, 0x0060: 312c 312c 3230 302c 3234 312c 3134 3429 1,1,200,241,144) 0x0070: 2e0d 2e ... Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/conntrack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)