diff mbox series

[ovs-dev] ofp-actions: Fix use-after-free while decoding RAW_ENCAP.

Message ID 20210216222730.3550567-1-i.maximets@ovn.org
State Accepted
Headers show
Series [ovs-dev] ofp-actions: Fix use-after-free while decoding RAW_ENCAP. | expand

Commit Message

Ilya Maximets Feb. 16, 2021, 10:27 p.m. UTC 
While decoding RAW_ENCAP action, decode_ed_prop() might re-allocate
ofpbuf if there is no enough space left.  However, function
'decode_NXAST_RAW_ENCAP' continues to use old pointer to 'encap'
structure leading to write-after-free and incorrect decoding.

  ==3549105==ERROR: AddressSanitizer: heap-use-after-free on address
  0x60600000011a at pc 0x0000005f6cc6 bp 0x7ffc3a2d4410 sp 0x7ffc3a2d4408
  WRITE of size 2 at 0x60600000011a thread T0
    #0 0x5f6cc5 in decode_NXAST_RAW_ENCAP lib/ofp-actions.c:4461:20
    #1 0x5f0551 in ofpact_decode ./lib/ofp-actions.inc2:4777:16
    #2 0x5ed17c in ofpacts_decode lib/ofp-actions.c:7752:21
    #3 0x5eba9a in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7791:13
    #4 0x5eb9fc in ofpacts_pull_openflow_actions lib/ofp-actions.c:7835:12
    #5 0x64bb8b in ofputil_decode_packet_out lib/ofp-packet.c:1113:17
    #6 0x65b6f4 in ofp_print_packet_out lib/ofp-print.c:148:13
    #7 0x659e3f in ofp_to_string__ lib/ofp-print.c:1029:16
    #8 0x659b24 in ofp_to_string lib/ofp-print.c:1244:21
    #9 0x65a28c in ofp_print lib/ofp-print.c:1288:28
    #10 0x540d11 in ofctl_ofp_parse utilities/ovs-ofctl.c:2814:9
    #11 0x564228 in ovs_cmdl_run_command__ lib/command-line.c:247:17
    #12 0x56408a in ovs_cmdl_run_command lib/command-line.c:278:5
    #13 0x5391ae in main utilities/ovs-ofctl.c:179:9
    #14 0x7f6911ce9081 in __libc_start_main (/lib64/libc.so.6+0x27081)
    #15 0x461fed in _start (utilities/ovs-ofctl+0x461fed)

Fix that by getting a new pointer before using.

Credit to OSS-Fuzz.

Fuzzer regression test will fail only with AddressSanitizer enabled.

Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27851
Fixes: f839892a206a ("OF support and translation of generic encap and decap")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/ofp-actions.c                                  |   2 ++
 tests/automake.mk                                  |   3 ++-
 tests/fuzz-regression-list.at                      |   1 +
 .../ofp_print_fuzzer-6540965472632832              | Bin 0 -> 72 bytes
 4 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 tests/fuzz-regression/ofp_print_fuzzer-6540965472632832

GIT binary patch
literal 72
zcmX|$F$w@648$Vn=<ax+v-k}E|DoC{m~2U@Y{^_1NGx5<Xwb#*hWtE!pV6J1N}K~3
CK@3&^

literal 0
HcmV?d00001

Comments

William Tu Feb. 17, 2021, 1:47 a.m. UTC | #1 
On Tue, Feb 16, 2021 at 2:27 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> While decoding RAW_ENCAP action, decode_ed_prop() might re-allocate
> ofpbuf if there is no enough space left.  However, function
> 'decode_NXAST_RAW_ENCAP' continues to use old pointer to 'encap'
> structure leading to write-after-free and incorrect decoding.
>
>   ==3549105==ERROR: AddressSanitizer: heap-use-after-free on address
>   0x60600000011a at pc 0x0000005f6cc6 bp 0x7ffc3a2d4410 sp 0x7ffc3a2d4408
>   WRITE of size 2 at 0x60600000011a thread T0
>     #0 0x5f6cc5 in decode_NXAST_RAW_ENCAP lib/ofp-actions.c:4461:20
>     #1 0x5f0551 in ofpact_decode ./lib/ofp-actions.inc2:4777:16
>     #2 0x5ed17c in ofpacts_decode lib/ofp-actions.c:7752:21
>     #3 0x5eba9a in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7791:13
>     #4 0x5eb9fc in ofpacts_pull_openflow_actions lib/ofp-actions.c:7835:12
>     #5 0x64bb8b in ofputil_decode_packet_out lib/ofp-packet.c:1113:17
>     #6 0x65b6f4 in ofp_print_packet_out lib/ofp-print.c:148:13
>     #7 0x659e3f in ofp_to_string__ lib/ofp-print.c:1029:16
>     #8 0x659b24 in ofp_to_string lib/ofp-print.c:1244:21
>     #9 0x65a28c in ofp_print lib/ofp-print.c:1288:28
>     #10 0x540d11 in ofctl_ofp_parse utilities/ovs-ofctl.c:2814:9
>     #11 0x564228 in ovs_cmdl_run_command__ lib/command-line.c:247:17
>     #12 0x56408a in ovs_cmdl_run_command lib/command-line.c:278:5
>     #13 0x5391ae in main utilities/ovs-ofctl.c:179:9
>     #14 0x7f6911ce9081 in __libc_start_main (/lib64/libc.so.6+0x27081)
>     #15 0x461fed in _start (utilities/ovs-ofctl+0x461fed)
>
> Fix that by getting a new pointer before using.
>
> Credit to OSS-Fuzz.
>
> Fuzzer regression test will fail only with AddressSanitizer enabled.
>
> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27851
> Fixes: f839892a206a ("OF support and translation of generic encap and decap")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>


LGTM.
Acked-by: William Tu <u9012063@gmail.com>
Ilya Maximets Feb. 17, 2021, 4:09 p.m. UTC | #2 
On 2/17/21 2:47 AM, William Tu wrote:
> On Tue, Feb 16, 2021 at 2:27 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>>
>> While decoding RAW_ENCAP action, decode_ed_prop() might re-allocate
>> ofpbuf if there is no enough space left.  However, function
>> 'decode_NXAST_RAW_ENCAP' continues to use old pointer to 'encap'
>> structure leading to write-after-free and incorrect decoding.
>>
>>   ==3549105==ERROR: AddressSanitizer: heap-use-after-free on address
>>   0x60600000011a at pc 0x0000005f6cc6 bp 0x7ffc3a2d4410 sp 0x7ffc3a2d4408
>>   WRITE of size 2 at 0x60600000011a thread T0
>>     #0 0x5f6cc5 in decode_NXAST_RAW_ENCAP lib/ofp-actions.c:4461:20
>>     #1 0x5f0551 in ofpact_decode ./lib/ofp-actions.inc2:4777:16
>>     #2 0x5ed17c in ofpacts_decode lib/ofp-actions.c:7752:21
>>     #3 0x5eba9a in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7791:13
>>     #4 0x5eb9fc in ofpacts_pull_openflow_actions lib/ofp-actions.c:7835:12
>>     #5 0x64bb8b in ofputil_decode_packet_out lib/ofp-packet.c:1113:17
>>     #6 0x65b6f4 in ofp_print_packet_out lib/ofp-print.c:148:13
>>     #7 0x659e3f in ofp_to_string__ lib/ofp-print.c:1029:16
>>     #8 0x659b24 in ofp_to_string lib/ofp-print.c:1244:21
>>     #9 0x65a28c in ofp_print lib/ofp-print.c:1288:28
>>     #10 0x540d11 in ofctl_ofp_parse utilities/ovs-ofctl.c:2814:9
>>     #11 0x564228 in ovs_cmdl_run_command__ lib/command-line.c:247:17
>>     #12 0x56408a in ovs_cmdl_run_command lib/command-line.c:278:5
>>     #13 0x5391ae in main utilities/ovs-ofctl.c:179:9
>>     #14 0x7f6911ce9081 in __libc_start_main (/lib64/libc.so.6+0x27081)
>>     #15 0x461fed in _start (utilities/ovs-ofctl+0x461fed)
>>
>> Fix that by getting a new pointer before using.
>>
>> Credit to OSS-Fuzz.
>>
>> Fuzzer regression test will fail only with AddressSanitizer enabled.
>>
>> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27851
>> Fixes: f839892a206a ("OF support and translation of generic encap and decap")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> 
> LGTM.
> Acked-by: William Tu <u9012063@gmail.com>
> 

Thanks!
Applied to master and backported down to 2.11.

Patch doesn't apply cleanly to older branches, so I didn't backport it there.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e2e829772..0342a228b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4431,6 +4431,7 @@  decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae,
 {
     struct ofpact_encap *encap;
     const struct ofp_ed_prop_header *ofp_prop;
+    const size_t encap_ofs = out->size;
     size_t props_len;
     uint16_t n_props = 0;
     int err;
@@ -4458,6 +4459,7 @@  decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae,
         }
         n_props++;
     }
+    encap = ofpbuf_at_assert(out, encap_ofs, sizeof *encap);
     encap->n_props = n_props;
     out->header = &encap->ofpact;
     ofpact_finish_ENCAP(out, &encap);
diff --git a/tests/automake.mk b/tests/automake.mk
index dfec2ea10..44a65849c 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -134,7 +134,8 @@  FUZZ_REGRESSION_TESTS = \
 	tests/fuzz-regression/ofp_print_fuzzer-5722747668791296 \
 	tests/fuzz-regression/ofp_print_fuzzer-6285128790704128 \
 	tests/fuzz-regression/ofp_print_fuzzer-6470117922701312 \
-	tests/fuzz-regression/ofp_print_fuzzer-6502620041576448
+	tests/fuzz-regression/ofp_print_fuzzer-6502620041576448 \
+	tests/fuzz-regression/ofp_print_fuzzer-6540965472632832
 $(srcdir)/tests/fuzz-regression-list.at: tests/automake.mk
 	$(AM_V_GEN)for name in $(FUZZ_REGRESSION_TESTS); do \
             basename=`echo $$name | sed 's,^.*/,,'`; \
diff --git a/tests/fuzz-regression-list.at b/tests/fuzz-regression-list.at
index e3173fb88..2347c690e 100644
--- a/tests/fuzz-regression-list.at
+++ b/tests/fuzz-regression-list.at
@@ -21,3 +21,4 @@  TEST_FUZZ_REGRESSION([ofp_print_fuzzer-5722747668791296])
 TEST_FUZZ_REGRESSION([ofp_print_fuzzer-6285128790704128])
 TEST_FUZZ_REGRESSION([ofp_print_fuzzer-6470117922701312])
 TEST_FUZZ_REGRESSION([ofp_print_fuzzer-6502620041576448])
+TEST_FUZZ_REGRESSION([ofp_print_fuzzer-6540965472632832])
diff --git a/tests/fuzz-regression/ofp_print_fuzzer-6540965472632832 b/tests/fuzz-regression/ofp_print_fuzzer-6540965472632832
new file mode 100644
index 0000000000000000000000000000000000000000..2977ee312bd64025f458e0f4bd6bcf6ecdba9b29