Skip to content

Commit 1bc138b

Browse files
[native] Harden CoreCLR environment override parsing (#12731)
## Context The CoreCLR Debug environment override parser could parse uninitialized bytes or read past its allocation after short reads, undersized payload accounting, malformed headers, zero widths, or unterminated fixed-width strings. ## Changes - Continue reading through normal short reads and `EINTR`, then parse only the bytes actually read. - Bound and validate both fixed-width hexadecimal header fields. - Reject zero or overflowing widths and invalid record divisibility before arithmetic. - Validate name and value NUL terminators within each declared field before logging or applying them. - Preserve malloc/free ownership and the no-libc++ startup constraint. ## Validation - Isolated ASan/UBSan harness covering a valid 26-byte record, multiple records, repeated short reads, truncated EOF, missing terminators, zero width, and malformed headers. - Android arm64 Debug translation-unit compile with `-Wall -Wextra -Werror`. - Undefined libc++ symbol set matches the unmodified baseline object. - Full `dotnet build src/native/native-clr.csproj` was attempted but this unprepared worktree lacks `xa-prep-tasks.dll` and `Xamarin.Android.Tools.BootstrapTasks.dll`. - [x] Useful description of why the change is necessary. - [ ] Links to issues fixed (none) - [x] Unit tests / focused regression validation
1 parent 381b445 commit 1bc138b

1 file changed

Lines changed: 109 additions & 19 deletions

File tree

src/native/clr/runtime-base/android-system.cc

Lines changed: 109 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include <cstdint>
12
#include <limits>
23
#include <string_view>
34

@@ -115,6 +116,14 @@ AndroidSystem::setup_environment_from_override_file (const char *path) noexcept
115116
return;
116117
}
117118

119+
if (sbuf.st_size < 0 || static_cast<uintmax_t>(sbuf.st_size) > static_cast<uintmax_t>(std::numeric_limits<size_t>::max ())) {
120+
log_warnf (LOG_DEFAULT, "Failed to read the environment override file %s: invalid file size", path);
121+
if (close (fd) < 0) {
122+
log_warnf (LOG_DEFAULT, "Failed to close the environment override file %s: %s", path, strerror (errno));
123+
}
124+
return;
125+
}
126+
118127
auto file_size = static_cast<size_t>(sbuf.st_size);
119128
if (file_size == 0) {
120129
log_warnf (LOG_DEFAULT, "Failed to read the environment override file %s: file is empty", path);
@@ -125,31 +134,47 @@ AndroidSystem::setup_environment_from_override_file (const char *path) noexcept
125134
}
126135

127136
size_t nread = 0uz;
128-
ssize_t r;
137+
ssize_t r = 0;
138+
int read_errno = 0;
129139
char *buf = static_cast<char*> (std::malloc (file_size));
130140
if (buf == nullptr) [[unlikely]] {
131141
Helpers::abort_application (LOG_DEFAULT, "Unable to allocate memory for the environment override file");
132142
}
133143

134-
do {
144+
while (nread < file_size) {
135145
auto read_count = static_cast<read_count_type>(file_size - nread);
146+
if (read_count > static_cast<read_count_type>(std::numeric_limits<ssize_t>::max ())) {
147+
read_count = static_cast<read_count_type>(std::numeric_limits<ssize_t>::max ());
148+
}
149+
136150
r = read (fd, buf + nread, read_count);
137151
if (r > 0) {
138152
nread += static_cast<size_t>(r);
153+
continue;
154+
}
155+
156+
if (r < 0 && errno == EINTR) {
157+
continue;
139158
}
140-
} while (r < 0 && errno == EINTR);
141159

142-
int read_errno = errno;
160+
if (r < 0) {
161+
read_errno = errno;
162+
}
163+
break;
164+
}
165+
143166
if (close (fd) < 0) {
144167
log_warnf (LOG_DEFAULT, "Failed to close the environment override file %s: %s", path, strerror (errno));
145168
}
146169

170+
if (read_errno != 0) {
171+
log_warnf (LOG_DEFAULT, "Failed to read the environment override file %s: %s", path, strerror (read_errno));
172+
std::free (buf);
173+
return;
174+
}
175+
147176
if (nread == 0) {
148-
if (r < 0) {
149-
log_warnf (LOG_DEFAULT, "Failed to read the environment override file %s: %s", path, strerror (read_errno));
150-
} else {
151-
log_warnf (LOG_DEFAULT, "Failed to read the environment override file %s: unexpected end of file", path);
152-
}
177+
log_warnf (LOG_DEFAULT, "Failed to read the environment override file %s: unexpected end of file", path);
153178
std::free (buf);
154179
return;
155180
}
@@ -176,39 +201,104 @@ AndroidSystem::setup_environment_from_override_file (const char *path) noexcept
176201
return;
177202
}
178203

204+
static constexpr size_t header_field_size = Constants::OVERRIDE_ENVIRONMENT_FILE_HEADER_SIZE / 2uz;
205+
static constexpr size_t header_value_size = header_field_size - 1uz;
206+
auto is_valid_width_field = [] (const char *field) noexcept -> bool {
207+
if (field [0] != '0' || (field [1] != 'x' && field [1] != 'X') || field [header_value_size] != '\0') {
208+
return false;
209+
}
210+
211+
for (size_t i = 2uz; i < header_value_size; ++i) {
212+
char c = field [i];
213+
if (!((c >= '0' && c <= '9') ||
214+
(c >= 'a' && c <= 'f') ||
215+
(c >= 'A' && c <= 'F'))) {
216+
return false;
217+
}
218+
}
219+
220+
return true;
221+
};
222+
223+
char name_width_field [header_field_size];
224+
memcpy (name_width_field, buf, header_field_size);
225+
if (!is_valid_width_field (name_width_field)) {
226+
log_warnf (LOG_DEFAULT, "Malformed header of the environment override file %s: name width has invalid format", path);
227+
std::free (buf);
228+
return;
229+
}
230+
231+
char value_width_field [header_field_size];
232+
memcpy (value_width_field, buf + header_field_size, header_field_size);
233+
if (!is_valid_width_field (value_width_field)) {
234+
log_warnf (LOG_DEFAULT, "Malformed header of the environment override file %s: value width has invalid format", path);
235+
std::free (buf);
236+
return;
237+
}
238+
179239
char *endptr;
180-
unsigned long name_width = strtoul (buf, &endptr, 16);
181-
if ((name_width == std::numeric_limits<unsigned long>::max () && errno == ERANGE) || (buf [0] != '\0' && *endptr != '\0')) {
240+
errno = 0;
241+
unsigned long name_width_raw = strtoul (name_width_field, &endptr, 16);
242+
if (errno == ERANGE || endptr != name_width_field + header_value_size) {
182243
log_warnf (LOG_DEFAULT, "Malformed header of the environment override file %s: name width has invalid format", path);
183244
std::free (buf);
184245
return;
185246
}
186247

187-
unsigned long value_width = strtoul (buf + 11, &endptr, 16);
188-
if ((value_width == std::numeric_limits<unsigned long>::max () && errno == ERANGE) || (buf [0] != '\0' && *endptr != '\0')) {
248+
errno = 0;
249+
unsigned long value_width_raw = strtoul (value_width_field, &endptr, 16);
250+
if (errno == ERANGE || endptr != value_width_field + header_value_size) {
189251
log_warnf (LOG_DEFAULT, "Malformed header of the environment override file %s: value width has invalid format", path);
190252
std::free (buf);
191253
return;
192254
}
193255

194-
uint64_t data_width = name_width + value_width;
195-
if (data_width > file_size - Constants::OVERRIDE_ENVIRONMENT_FILE_HEADER_SIZE || (file_size - Constants::OVERRIDE_ENVIRONMENT_FILE_HEADER_SIZE) % data_width != 0) {
256+
if (name_width_raw == 0 || value_width_raw == 0 ||
257+
static_cast<uintmax_t>(name_width_raw) > static_cast<uintmax_t>(std::numeric_limits<size_t>::max ()) ||
258+
static_cast<uintmax_t>(value_width_raw) > static_cast<uintmax_t>(std::numeric_limits<size_t>::max ())) {
259+
log_warnf (LOG_DEFAULT, "Malformed environment override file %s: invalid data size", path);
260+
std::free (buf);
261+
return;
262+
}
263+
264+
size_t name_width = static_cast<size_t>(name_width_raw);
265+
size_t value_width = static_cast<size_t>(value_width_raw);
266+
if (name_width > std::numeric_limits<size_t>::max () - value_width) {
267+
log_warnf (LOG_DEFAULT, "Malformed environment override file %s: invalid data size", path);
268+
std::free (buf);
269+
return;
270+
}
271+
272+
size_t data_width = name_width + value_width;
273+
size_t data_size = nread - Constants::OVERRIDE_ENVIRONMENT_FILE_HEADER_SIZE;
274+
if (data_width > data_size || data_size % data_width != 0) {
196275
log_warnf (LOG_DEFAULT, "Malformed environment override file %s: invalid data size", path);
197276
std::free (buf);
198277
return;
199278
}
200279

201-
uint64_t data_size = static_cast<uint64_t>(file_size);
202280
char *name = buf + Constants::OVERRIDE_ENVIRONMENT_FILE_HEADER_SIZE;
203-
while (data_size > 0 && data_size >= data_width) {
281+
while (data_size > 0) {
204282
if (*name == '\0') {
205283
log_warnf (LOG_DEFAULT, "Malformed environment override file %s: name at offset %td is empty", path, name - buf);
206284
std::free (buf);
207285
return;
208286
}
209287

210-
log_debugf (LOG_DEFAULT, "Setting environment variable from the override file %s: '%s' = '%s'", path, name, name + name_width);
211-
setup_environment (name, name + name_width);
288+
char *value = name + name_width;
289+
if (memchr (name, '\0', name_width) == nullptr) {
290+
log_warnf (LOG_DEFAULT, "Malformed environment override file %s: name at offset %td is not NUL-terminated", path, name - buf);
291+
std::free (buf);
292+
return;
293+
}
294+
if (memchr (value, '\0', value_width) == nullptr) {
295+
log_warnf (LOG_DEFAULT, "Malformed environment override file %s: value at offset %td is not NUL-terminated", path, value - buf);
296+
std::free (buf);
297+
return;
298+
}
299+
300+
log_debugf (LOG_DEFAULT, "Setting environment variable from the override file %s: '%s' = '%s'", path, name, value);
301+
setup_environment (name, value);
212302
name += data_width;
213303
data_size -= data_width;
214304
}

0 commit comments

Comments
 (0)