Skip to content

Commit 69cede4

Browse files
kalenikaliaksandrtrflynn89
authored andcommitted
AK+LibWeb: Make StringBase::bytes() lvalue-only
Disallow calling `StringBase::bytes()` on temporaries to avoid returning `ReadonlyBytes` that outlive the underlying string. With this change, we catch a real UAF: `load_result.data = maybe_response.release_value().bytes();` All other updated call sites were already safe, they just needed to use an intermediate named variable to satisfy the new lvalue-only requirement.
1 parent d1f34ef commit 69cede4

File tree

8 files changed

+31
-16
lines changed

8 files changed

+31
-16
lines changed

AK/StringBase.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,8 @@ class StringBase {
7171

7272
// Returns the underlying UTF-8 encoded bytes.
7373
// NOTE: There is no guarantee about null-termination.
74-
[[nodiscard]] ReadonlyBytes bytes() const LIFETIME_BOUND;
74+
[[nodiscard]] ReadonlyBytes bytes() const&& = delete;
75+
[[nodiscard]] ReadonlyBytes bytes() const& LIFETIME_BOUND;
7576
[[nodiscard]] u32 hash() const;
7677
[[nodiscard]] size_t byte_count() const;
7778
[[nodiscard]] ALWAYS_INLINE size_t length_in_code_units() const { return byte_count(); }
@@ -204,7 +205,7 @@ inline size_t ShortString::byte_count() const
204205
return byte_count_and_short_string_flag >> StringBase::SHORT_STRING_BYTE_COUNT_SHIFT_COUNT;
205206
}
206207

207-
inline ReadonlyBytes StringBase::bytes() const
208+
inline ReadonlyBytes StringBase::bytes() const&
208209
{
209210
if (is_short_string())
210211
return m_impl.short_string.bytes();

Libraries/LibWeb/Fetch/BodyInit.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,8 @@ WebIDL::ExceptionOr<Infrastructure::BodyWithType> extract_body(JS::Realm& realm,
103103
source = serialized_form_data.serialized_data;
104104
// FIXME: Set length to unclear, see html/6424 for improving this.
105105
// Set type to `multipart/form-data; boundary=`, followed by the multipart/form-data boundary string generated by the multipart/form-data encoding algorithm.
106-
type = MUST(ByteBuffer::copy(MUST(String::formatted("multipart/form-data; boundary={}", serialized_form_data.boundary)).bytes()));
106+
auto type_string = MUST(String::formatted("multipart/form-data; boundary={}", serialized_form_data.boundary));
107+
type = MUST(ByteBuffer::copy(type_string.bytes()));
107108
return {};
108109
},
109110
[&](GC::Root<DOMURL::URLSearchParams> const& url_search_params) -> WebIDL::ExceptionOr<void> {

Libraries/LibWeb/Fetch/Fetching/Fetching.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,8 +1795,10 @@ GC::Ref<PendingResponse> http_network_or_cache_fetch(JS::Realm& realm, Infrastru
17951795

17961796
// 8. If contentLength is non-null, then set contentLengthHeaderValue to contentLength, serialized and
17971797
// isomorphic encoded.
1798-
if (content_length.has_value())
1799-
content_length_header_value = MUST(ByteBuffer::copy(String::number(*content_length).bytes()));
1798+
if (content_length.has_value()) {
1799+
auto content_length_string = String::number(*content_length);
1800+
content_length_header_value = MUST(ByteBuffer::copy(content_length_string.bytes()));
1801+
}
18001802

18011803
// 9. If contentLengthHeaderValue is non-null, then append (`Content-Length`, contentLengthHeaderValue) to
18021804
// httpRequest’s header list.

Libraries/LibWeb/Fetch/Infrastructure/HTTP/Requests.cpp

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,8 @@ String Request::serialize_origin() const
215215
ByteBuffer Request::byte_serialize_origin() const
216216
{
217217
// Byte-serializing a request origin, given a request request, is to return the result of serializing a request origin with request, isomorphic encoded.
218-
return MUST(ByteBuffer::copy(serialize_origin().bytes()));
218+
auto serialized_origin = serialize_origin();
219+
return MUST(ByteBuffer::copy(serialized_origin.bytes()));
219220
}
220221

221222
// https://fetch.spec.whatwg.org/#concept-request-clone
@@ -285,14 +286,17 @@ void Request::add_range_header(u64 first, Optional<u64> const& last)
285286
auto range_value = MUST(ByteBuffer::copy("bytes"sv.bytes()));
286287

287288
// 3. Serialize and isomorphic encode first, and append the result to rangeValue.
288-
range_value.append(String::number(first).bytes());
289+
auto serialized_first = String::number(first);
290+
range_value.append(serialized_first.bytes());
289291

290292
// 4. Append 0x2D (-) to rangeValue.
291293
range_value.append('-');
292294

293295
// 5. If last is given, then serialize and isomorphic encode it, and append the result to rangeValue.
294-
if (last.has_value())
295-
range_value.append(String::number(*last).bytes());
296+
if (last.has_value()) {
297+
auto serialized_last = String::number(*last);
298+
range_value.append(serialized_last.bytes());
299+
}
296300

297301
// 6. Append (`Range`, rangeValue) to request’s header list.
298302
auto header = Header {

Libraries/LibWeb/HTML/HTMLFormElement.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,8 @@ ErrorOr<void> HTMLFormElement::submit_as_entity_body(URL::URL parsed_action, Vec
861861
auto pairs = TRY(convert_to_list_of_name_value_pairs(entry_list));
862862

863863
// 2. Let body be the result of running the application/x-www-form-urlencoded serializer with pairs and encoding.
864-
body = TRY(ByteBuffer::copy(url_encode(pairs, encoding).bytes()));
864+
auto query = url_encode(pairs, encoding);
865+
body = TRY(ByteBuffer::copy(query.bytes()));
865866

866867
// 3. Set body to the result of encoding body.
867868
// NOTE: `encoding` refers to `UTF-8 encode`, which body already is encoded as because it uses AK::String.
@@ -888,7 +889,8 @@ ErrorOr<void> HTMLFormElement::submit_as_entity_body(URL::URL parsed_action, Vec
888889
auto pairs = TRY(convert_to_list_of_name_value_pairs(entry_list));
889890

890891
// 2. Let body be the result of running the text/plain encoding algorithm with pairs.
891-
body = TRY(ByteBuffer::copy(TRY(plain_text_encode(pairs)).bytes()));
892+
auto serialized_body = TRY(plain_text_encode(pairs));
893+
body = TRY(ByteBuffer::copy(serialized_body.bytes()));
892894

893895
// FIXME: 3. Set body to the result of encoding body using encoding.
894896

Libraries/LibWeb/Loader/ResourceLoader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ void ResourceLoader::handle_resource_load_request(LoadRequest const& request, Re
326326
}
327327

328328
FileLoadResult load_result;
329-
load_result.data = maybe_response.release_value().bytes();
329+
load_result.data = maybe_response.value().bytes();
330330
load_result.response_headers.set("Content-Type"sv, "text/html"sv);
331331
on_resource(load_result);
332332
return;

Libraries/LibWeb/ReferrerPolicy/AbstractOperations.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,11 @@ Optional<URL::URL> determine_requests_referrer(Fetch::Infrastructure::Request co
108108

109109
// 6. If the result of serializing referrerURL is a string whose length is greater than 4096, set referrerURL to
110110
// referrerOrigin.
111-
if (referrer_url.has_value() && referrer_url.value().serialize().bytes().size() > 4096)
112-
referrer_url = referrer_origin;
111+
if (referrer_url.has_value()) {
112+
auto serialized_referrer_url = referrer_url.value().serialize();
113+
if (serialized_referrer_url.bytes().size() > 4096)
114+
referrer_url = referrer_origin;
115+
}
113116

114117
// 7. The user agent MAY alter referrerURL or referrerOrigin at this point to enforce arbitrary policy
115118
// considerations in the interests of minimizing data leakage. For example, the user agent could strip the URL

Libraries/LibWeb/XHR/XMLHttpRequest.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,8 @@ WebIDL::ExceptionOr<void> XMLHttpRequest::send(Optional<DocumentOrXMLHttpRequest
573573
// 2. If body is a Document, then set this’s request body to body, serialized, converted, and UTF-8 encoded.
574574
if (body->has<GC::Root<DOM::Document>>()) {
575575
auto string_serialized_document = TRY(body->get<GC::Root<DOM::Document>>().cell()->serialize_fragment(HTML::RequireWellFormed::No));
576-
m_request_body = Fetch::Infrastructure::byte_sequence_as_body(realm, string_serialized_document.to_utf8().bytes());
576+
auto string_serialized_document_utf8 = string_serialized_document.to_utf8();
577+
m_request_body = Fetch::Infrastructure::byte_sequence_as_body(realm, string_serialized_document_utf8.bytes());
577578
}
578579
// 3. Otherwise:
579580
else {
@@ -1014,7 +1015,8 @@ String XMLHttpRequest::get_all_response_headers() const
10141015
output.append(0x3A); // ':'
10151016
output.append(0x20); // ' '
10161017
// FIXME: The spec does not mention isomorphic decode. Spec bug?
1017-
output.append(Infra::isomorphic_decode(header.value).bytes());
1018+
auto decoder_header = Infra::isomorphic_decode(header.value);
1019+
output.append(decoder_header.bytes());
10181020
output.append(0x0D); // '\r'
10191021
output.append(0x0A); // '\n'
10201022
}

0 commit comments

Comments
 (0)