From 1f3bc5ea8074dc98edbee6eb90a308b283117954 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Thu, 5 Jan 2023 14:16:21 -0500 Subject: [PATCH] Illumos #15286: do_composition() needs sign awareness Authored by: Dan McDonald Reviewed by: Patrick Mooney Reviewed by: Richard Lowe Approved by: Joshua M. Clulow Ported-by: Richard Yao Illumos-issue: https://www.illumos.org/issues/15286 Illumos-commit: https://github.com/illumos/illumos-gate/commit/f137b22e734e85642da3e56e8b94da3f5f027c73 Porting Notes: The patch in illumos did not have much of a commit message, and did not provide attribution to the reporter, while original patch proposed to OpenZFS did, so I am listing the reporter (myself) and original patch author (also myself) below while including the original commit message with some minor corrections as part of the porting notes: In do_composition(), we have: size = u8_number_of_bytes[*p]; if (size <= 1 || (p + size) > oslast) break; There, we have type promotion from int8_t to size_t, which is unsigned. C will sign extend the value as part of the widening before treating the value as unsigned and the negative values we can counter are error values from U8_ILLEGAL_CHAR and U8_OUT_OF_RANGE_CHAR, which are -1 and -2 respectively. The unsigned versions of these under two's complement are SIZE_MAX and SIZE_MAX-1 respectively. The bounds check is written under the assumption that `size <= 1` does a signed comparison. This is followed by a pointer comparison to see if the string has the correct length, which is fine. A little further down we have: for (i = 0; i < size; i++) tc[i] = *p++; When an error condition is encountered, this will attempt to iterate at least SIZE_MAX-1 times, which will massively overflow the buffer, which is not fine. The kernel will kill the loop as soon as it hits the kernel stack guard on Linux systems built with CONFIG_VMAP_STACK=y, which should be just about all of them. That prevents arbitrary code execution and just about any other bad thing that a black hat attacker might attempt with knowledge of this buffer overflow. Other systems' kernels have mitigations for unbounded in-kernel buffer overflows that will catch this too. Also, the patch in illumos-gate made an effort to fix C style issues that had been fixed in the OpenZFS/ZFSOnLinux repository. Those issues had been mentioned in the email that I originally sent them about this issue. One of the fixes had not been already done, so it is included. Another to collect_a_seq()'s arguments was handled differently in OpenZFS. For the sake of avoiding unnecessary differences, it has been adopted. This has the interesting effect that if you correct the paths in the illumos-gate patch to match the current OpenZFS repository, you can reverse apply it cleanly. Original-patch-by: Richard Yao Reported-by: Richard Yao Co-authored-by: Dan McDonald Closes #14318 Closes #14342 --- module/unicode/u8_textprep.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/module/unicode/u8_textprep.c b/module/unicode/u8_textprep.c index 1940ee510..49e22c88c 100644 --- a/module/unicode/u8_textprep.c +++ b/module/unicode/u8_textprep.c @@ -23,6 +23,9 @@ * Use is subject to license terms. */ +/* + * Copyright 2022 MNX Cloud, Inc. + */ @@ -213,10 +216,10 @@ static const int8_t u8_number_of_bytes[0x100] = { /* 80 81 82 83 84 85 86 87 88 89 8A 8B 8C 8D 8E 8F */ I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, -/* 90 91 92 93 94 95 96 97 98 99 9A 9B 9C 9D 9E 9F */ +/* 90 91 92 93 94 95 96 97 98 99 9A 9B 9C 9D 9E 9F */ I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, -/* A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 AA AB AC AD AE AF */ +/* A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 AA AB AC AD AE AF */ I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, I_, /* B0 B1 B2 B3 B4 B5 B6 B7 B8 B9 BA BB BC BD BE BF */ @@ -1286,8 +1289,12 @@ TRY_THE_NEXT_MARK: saved_l = l - disp[last]; while (p < oslast) { - size = u8_number_of_bytes[*p]; - if (size <= 1 || (p + size) > oslast) + int8_t number_of_bytes = u8_number_of_bytes[*p]; + + if (number_of_bytes <= 1) + break; + size = number_of_bytes; + if ((p + size) > oslast) break; saved_p = p; @@ -1378,8 +1385,10 @@ SAFE_RETURN: */ static size_t collect_a_seq(size_t uv, uchar_t *u8s, uchar_t **source, uchar_t *slast, - boolean_t is_it_toupper, boolean_t is_it_tolower, - boolean_t canonical_decomposition, boolean_t compatibility_decomposition, + boolean_t is_it_toupper, + boolean_t is_it_tolower, + boolean_t canonical_decomposition, + boolean_t compatibility_decomposition, boolean_t canonical_composition, int *errnum, u8_normalization_states_t *state) {