From 05b5a5b3b4b04823776feed6403b5a99b2e06d76 Mon Sep 17 00:00:00 2001 From: Klement Sekera Date: Mon, 28 Jun 2021 13:40:40 +0200 Subject: [PATCH] nat: harden ICMP handling Verify that headers are not truncated and that checksums are valid. Correct checksum computation in translation code. Type: fix Change-Id: I6acfcec4661411f83c86b15aafac90cd4538c0b5 Signed-off-by: Klement Sekera --- src/plugins/nat/nat44-ed/nat44_ed.c | 87 +++++++++++++++++++++--------- src/plugins/nat/nat44-ed/nat44_ed.h | 35 ++++++------ src/plugins/nat/nat44-ed/nat44_ed_in2out.c | 21 ++++++-- src/plugins/nat/nat44-ed/nat44_ed_out2in.c | 16 ++++-- test/test_nat44_ed.py | 4 ++ 5 files changed, 114 insertions(+), 49 deletions(-) diff --git a/src/plugins/nat/nat44-ed/nat44_ed.c b/src/plugins/nat/nat44-ed/nat44_ed.c index 41028d50a90..dcd7ae0a140 100644 --- a/src/plugins/nat/nat44-ed/nat44_ed.c +++ b/src/plugins/nat/nat44-ed/nat44_ed.c @@ -3401,10 +3401,9 @@ nat_6t_l3_l4_csum_calc (nat_6t_flow_t *f) } } -static_always_inline int nat_6t_flow_icmp_translate (snat_main_t *sm, - vlib_buffer_t *b, - ip4_header_t *ip, - nat_6t_flow_t *f); +static_always_inline int +nat_6t_flow_icmp_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, + ip4_header_t *ip, nat_6t_flow_t *f); static_always_inline void nat_6t_flow_ip4_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, @@ -3481,7 +3480,16 @@ nat_6t_flow_ip4_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, } static_always_inline int -nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, +it_fits (vlib_main_t *vm, vlib_buffer_t *b, void *object, size_t size) +{ + int result = ((u8 *) object + size <= + (u8 *) vlib_buffer_get_current (b) + b->current_length) && + vlib_object_within_buffer_data (vm, b, object, size); + return result; +} + +static_always_inline int +nat_6t_flow_icmp_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, nat_6t_flow_t *f) { if (IP_PROTOCOL_ICMP != ip->protocol) @@ -3492,8 +3500,19 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, if ((!vnet_buffer (b)->ip.reass.is_non_first_fragment)) { - if (icmp->checksum == 0) - icmp->checksum = 0xffff; + if (!it_fits (vm, b, icmp, sizeof (*icmp))) + { + return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED; + } + + ssize_t icmp_offset = (u8 *) icmp - (u8 *) vlib_buffer_get_current (b); + ip_csum_t sum = + ip_incremental_checksum (0, icmp, b->current_length - icmp_offset); + sum = (u16) ~ip_csum_fold (sum); + if (sum != 0) + { + return NAT_ED_TRNSL_ERR_INVALID_CSUM; + } if (!icmp_type_is_error_message (icmp->type)) { @@ -3515,7 +3534,7 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, if (!ip4_header_checksum_is_valid (inner_ip)) { - return NAT_ED_TRNSL_ERR_TRANSLATION_FAILED; + return NAT_ED_TRNSL_ERR_INNER_IP_CORRUPT; } nat_protocol_t inner_proto = @@ -3533,6 +3552,10 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, { case NAT_PROTOCOL_UDP: udp = (udp_header_t *) (inner_ip + 1); + if (!it_fits (vm, b, udp, sizeof (*udp))) + { + return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED; + } old_udp_sum = udp->checksum; nat_6t_flow_ip4_translate (sm, b, inner_ip, f, inner_proto, 1 /* is_icmp_inner_ip4 */, @@ -3546,12 +3569,14 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, ip_csum_update (new_icmp_sum, old_udp_sum, udp->checksum, udp_header_t, checksum); new_icmp_sum = ip_csum_fold (new_icmp_sum); - if (0xffff == new_icmp_sum) - new_icmp_sum = 0; icmp->checksum = new_icmp_sum; break; case NAT_PROTOCOL_TCP: tcp = (tcp_header_t *) (inner_ip + 1); + if (!it_fits (vm, b, tcp, sizeof (*tcp))) + { + return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED; + } old_tcp_sum = tcp->checksum; nat_6t_flow_ip4_translate (sm, b, inner_ip, f, inner_proto, 1 /* is_icmp_inner_ip4 */, @@ -3565,14 +3590,16 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, ip_csum_update (new_icmp_sum, old_tcp_sum, tcp->checksum, tcp_header_t, checksum); new_icmp_sum = ip_csum_fold (new_icmp_sum); - if (0xffff == new_icmp_sum) - new_icmp_sum = 0; icmp->checksum = new_icmp_sum; break; case NAT_PROTOCOL_ICMP: if (f->ops & NAT_FLOW_OP_ICMP_ID_REWRITE) { icmp46_header_t *inner_icmp = ip4_next_header (inner_ip); + if (!it_fits (vm, b, inner_icmp, sizeof (*inner_icmp))) + { + return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED; + } icmp_echo_header_t *inner_echo = (icmp_echo_header_t *) (inner_icmp + 1); if (f->rewrite.icmp_id != inner_echo->identifier) @@ -3602,9 +3629,10 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b, } static_always_inline nat_translation_error_e -nat_6t_flow_buf_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, - nat_6t_flow_t *f, nat_protocol_t proto, - int is_output_feature, int is_i2o) +nat_6t_flow_buf_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, + ip4_header_t *ip, nat_6t_flow_t *f, + nat_protocol_t proto, int is_output_feature, + int is_i2o) { if (!is_output_feature && f->ops & NAT_FLOW_OP_TXFIB_REWRITE) { @@ -3627,7 +3655,7 @@ nat_6t_flow_buf_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, 0 /* is_icmp_inner_ip4 */, 0 /* skip_saddr_rewrite */); } - return nat_6t_flow_icmp_translate (sm, b, ip, f); + return nat_6t_flow_icmp_translate (vm, sm, b, ip, f); } nat_6t_flow_ip4_translate (sm, b, ip, f, proto, 0 /* is_icmp_inner_ip4 */, @@ -3637,20 +3665,22 @@ nat_6t_flow_buf_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, } nat_translation_error_e -nat_6t_flow_buf_translate_i2o (snat_main_t *sm, vlib_buffer_t *b, - ip4_header_t *ip, nat_6t_flow_t *f, - nat_protocol_t proto, int is_output_feature) +nat_6t_flow_buf_translate_i2o (vlib_main_t *vm, snat_main_t *sm, + vlib_buffer_t *b, ip4_header_t *ip, + nat_6t_flow_t *f, nat_protocol_t proto, + int is_output_feature) { - return nat_6t_flow_buf_translate (sm, b, ip, f, proto, is_output_feature, + return nat_6t_flow_buf_translate (vm, sm, b, ip, f, proto, is_output_feature, 1 /* is_i2o */); } nat_translation_error_e -nat_6t_flow_buf_translate_o2i (snat_main_t *sm, vlib_buffer_t *b, - ip4_header_t *ip, nat_6t_flow_t *f, - nat_protocol_t proto, int is_output_feature) +nat_6t_flow_buf_translate_o2i (vlib_main_t *vm, snat_main_t *sm, + vlib_buffer_t *b, ip4_header_t *ip, + nat_6t_flow_t *f, nat_protocol_t proto, + int is_output_feature) { - return nat_6t_flow_buf_translate (sm, b, ip, f, proto, is_output_feature, + return nat_6t_flow_buf_translate (vm, sm, b, ip, f, proto, is_output_feature, 0 /* is_i2o */); } @@ -3683,6 +3713,15 @@ format_nat_ed_translation_error (u8 *s, va_list *args) case NAT_ED_TRNSL_ERR_FLOW_MISMATCH: s = format (s, "flow-mismatch"); break; + case NAT_ED_TRNSL_ERR_PACKET_TRUNCATED: + s = format (s, "packet-truncated"); + break; + case NAT_ED_TRNSL_ERR_INNER_IP_CORRUPT: + s = format (s, "inner-ip-corrupted"); + break; + case NAT_ED_TRNSL_ERR_INVALID_CSUM: + s = format (s, "invalid-checksum"); + break; } return s; } diff --git a/src/plugins/nat/nat44-ed/nat44_ed.h b/src/plugins/nat/nat44-ed/nat44_ed.h index 15e8e480b4e..6abdbadae43 100644 --- a/src/plugins/nat/nat44-ed/nat44_ed.h +++ b/src/plugins/nat/nat44-ed/nat44_ed.h @@ -125,13 +125,14 @@ typedef enum #undef _ } snat_session_state_t; -#define foreach_nat_in2out_ed_error \ -_(UNSUPPORTED_PROTOCOL, "unsupported protocol") \ -_(OUT_OF_PORTS, "out of ports") \ -_(BAD_ICMP_TYPE, "unsupported ICMP type") \ -_(MAX_SESSIONS_EXCEEDED, "maximum sessions exceeded") \ -_(NON_SYN, "non-SYN packet try to create session") \ -_(TCP_CLOSED, "drops due to TCP in transitory timeout") +#define foreach_nat_in2out_ed_error \ + _ (UNSUPPORTED_PROTOCOL, "unsupported protocol") \ + _ (OUT_OF_PORTS, "out of ports") \ + _ (BAD_ICMP_TYPE, "unsupported ICMP type") \ + _ (MAX_SESSIONS_EXCEEDED, "maximum sessions exceeded") \ + _ (NON_SYN, "non-SYN packet try to create session") \ + _ (TCP_CLOSED, "drops due to TCP in transitory timeout") \ + _ (TRNSL_FAILED, "couldn't translate packet") typedef enum { @@ -149,7 +150,8 @@ typedef enum _ (MAX_SESSIONS_EXCEEDED, "maximum sessions exceeded") \ _ (NON_SYN, "non-SYN packet try to create session") \ _ (TCP_CLOSED, "drops due to TCP in transitory timeout") \ - _ (HASH_ADD_FAILED, "hash table add failed") + _ (HASH_ADD_FAILED, "hash table add failed") \ + _ (TRNSL_FAILED, "couldn't translate packet") typedef enum { @@ -1132,17 +1134,18 @@ typedef enum NAT_ED_TRNSL_ERR_SUCCESS = 0, NAT_ED_TRNSL_ERR_TRANSLATION_FAILED = 1, NAT_ED_TRNSL_ERR_FLOW_MISMATCH = 2, + NAT_ED_TRNSL_ERR_PACKET_TRUNCATED = 3, + NAT_ED_TRNSL_ERR_INNER_IP_CORRUPT = 4, + NAT_ED_TRNSL_ERR_INVALID_CSUM = 5, } nat_translation_error_e; -nat_translation_error_e -nat_6t_flow_buf_translate_i2o (snat_main_t *sm, vlib_buffer_t *b, - ip4_header_t *ip, nat_6t_flow_t *f, - nat_protocol_t proto, int is_output_feature); +nat_translation_error_e nat_6t_flow_buf_translate_i2o ( + vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, + nat_6t_flow_t *f, nat_protocol_t proto, int is_output_feature); -nat_translation_error_e -nat_6t_flow_buf_translate_o2i (snat_main_t *sm, vlib_buffer_t *b, - ip4_header_t *ip, nat_6t_flow_t *f, - nat_protocol_t proto, int is_output_feature); +nat_translation_error_e nat_6t_flow_buf_translate_o2i ( + vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip, + nat_6t_flow_t *f, nat_protocol_t proto, int is_output_feature); void nat_6t_l3_l4_csum_calc (nat_6t_flow_t *f); diff --git a/src/plugins/nat/nat44-ed/nat44_ed_in2out.c b/src/plugins/nat/nat44-ed/nat44_ed_in2out.c index bfabdbd71d0..ead5685c6f7 100644 --- a/src/plugins/nat/nat44-ed/nat44_ed_in2out.c +++ b/src/plugins/nat/nat44-ed/nat44_ed_in2out.c @@ -1177,16 +1177,18 @@ nat44_ed_in2out_fast_path_node_fn_inline (vlib_main_t *vm, nat_free_session_data (sm, s0, thread_index, 0); nat_ed_session_delete (sm, s0, thread_index, 1); next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED]; goto trace0; } if (NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_i2o ( - sm, b0, ip0, f, proto0, is_output_feature))) + vm, sm, b0, ip0, f, proto0, is_output_feature))) { nat_free_session_data (sm, s0, thread_index, 0); nat_ed_session_delete (sm, s0, thread_index, 1); next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED]; goto trace0; } @@ -1330,8 +1332,12 @@ nat44_ed_in2out_slow_path_node_fn_inline (vlib_main_t *vm, if (NAT_NEXT_DROP != next[0] && s0 && NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_i2o ( - sm, b0, ip0, &s0->i2o, proto0, is_output_feature))) + vm, sm, b0, ip0, &s0->i2o, proto0, is_output_feature))) { + nat_free_session_data (sm, s0, thread_index, 0); + nat_ed_session_delete (sm, s0, thread_index, 1); + next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED]; goto trace0; } @@ -1348,8 +1354,12 @@ nat44_ed_in2out_slow_path_node_fn_inline (vlib_main_t *vm, if (NAT_NEXT_DROP != next[0] && s0 && NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_i2o ( - sm, b0, ip0, &s0->i2o, proto0, is_output_feature))) + vm, sm, b0, ip0, &s0->i2o, proto0, is_output_feature))) { + nat_free_session_data (sm, s0, thread_index, 0); + nat_ed_session_delete (sm, s0, thread_index, 1); + next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED]; goto trace0; } @@ -1425,11 +1435,12 @@ nat44_ed_in2out_slow_path_node_fn_inline (vlib_main_t *vm, if (NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_i2o ( - sm, b0, ip0, &s0->i2o, proto0, is_output_feature))) + vm, sm, b0, ip0, &s0->i2o, proto0, is_output_feature))) { nat_free_session_data (sm, s0, thread_index, 0); nat_ed_session_delete (sm, s0, thread_index, 1); - s0 = NULL; + next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED]; goto trace0; } diff --git a/src/plugins/nat/nat44-ed/nat44_ed_out2in.c b/src/plugins/nat/nat44-ed/nat44_ed_out2in.c index eaf89937e54..124b64e29f3 100644 --- a/src/plugins/nat/nat44-ed/nat44_ed_out2in.c +++ b/src/plugins/nat/nat44-ed/nat44_ed_out2in.c @@ -1020,6 +1020,7 @@ nat44_ed_out2in_fast_path_node_fn_inline (vlib_main_t * vm, nat_free_session_data (sm, s0, thread_index, 0); nat_ed_session_delete (sm, s0, thread_index, 1); next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED]; goto trace0; } } @@ -1027,9 +1028,10 @@ nat44_ed_out2in_fast_path_node_fn_inline (vlib_main_t * vm, if (NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_o2i ( - sm, b0, ip0, f, proto0, 0 /* is_output_feature */))) + vm, sm, b0, ip0, f, proto0, 0 /* is_output_feature */))) { next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED]; goto trace0; } @@ -1183,8 +1185,11 @@ nat44_ed_out2in_slow_path_node_fn_inline (vlib_main_t * vm, if (NAT_NEXT_DROP != next[0] && s0 && NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_o2i ( - sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */))) + vm, sm, b0, ip0, &s0->o2i, proto0, + 0 /* is_output_feature */))) { + next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED]; goto trace0; } @@ -1202,8 +1207,11 @@ nat44_ed_out2in_slow_path_node_fn_inline (vlib_main_t * vm, if (NAT_NEXT_DROP != next[0] && s0 && NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_o2i ( - sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */))) + vm, sm, b0, ip0, &s0->o2i, proto0, + 0 /* is_output_feature */))) { + next[0] = NAT_NEXT_DROP; + b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED]; goto trace0; } @@ -1312,7 +1320,7 @@ nat44_ed_out2in_slow_path_node_fn_inline (vlib_main_t * vm, if (NAT_ED_TRNSL_ERR_SUCCESS != (translation_error = nat_6t_flow_buf_translate_o2i ( - sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */))) + vm, sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */))) { next[0] = NAT_NEXT_DROP; goto trace0; diff --git a/test/test_nat44_ed.py b/test/test_nat44_ed.py index b8774a20d3d..ee76ce845c6 100644 --- a/test/test_nat44_ed.py +++ b/test/test_nat44_ed.py @@ -1215,6 +1215,7 @@ class TestNAT44ED(NAT44EDTestCase): self.pg_start() capture = self.pg1.get_capture(len(pkts)) self.verify_capture_out(capture, ignore_port=True) + self.logger.debug(self.vapi.cli("show trace")) # out2in pkts = self.create_stream_out(self.pg1) @@ -1223,6 +1224,7 @@ class TestNAT44ED(NAT44EDTestCase): self.pg_start() capture = self.pg0.get_capture(len(pkts)) self.verify_capture_in(capture, self.pg0) + self.logger.debug(self.vapi.cli("show trace")) # in2out pkts = self.create_stream_in(self.pg0, self.pg1, ttl=2) @@ -1231,6 +1233,7 @@ class TestNAT44ED(NAT44EDTestCase): self.pg_start() capture = self.pg1.get_capture(len(pkts)) self.verify_capture_out(capture, ignore_port=True) + self.logger.debug(self.vapi.cli("show trace")) # out2in pkts = self.create_stream_out(self.pg1, ttl=2) @@ -1239,6 +1242,7 @@ class TestNAT44ED(NAT44EDTestCase): self.pg_start() capture = self.pg0.get_capture(len(pkts)) self.verify_capture_in(capture, self.pg0) + self.logger.debug(self.vapi.cli("show trace")) # in2out pkts = self.create_stream_in(self.pg0, self.pg1, ttl=1) -- 2.16.6