Skip to content

Commit 123b668

Browse files
authored
Fix ASAN-detected plugin bugs across five components (#13013)
* Fix heap-buffer-overflow in stats_over_http -- use %.*s with string_view length instead of %s. * Fix delete type mismatch in ja4_fingerprint -- delete as JA4_data* and cast consistently in handle_read_request_hdr. * Fix use-after-free in txn_box Config destructor -- reorder _arena before _roots so arena outlives directive memory. * Fix memory leak in slice plugin -- free urlstr before early return on invalid content length. * Fix use-after-free in async_engine test plugin -- save writefd before OPENSSL_free releases backing memory.
1 parent 89fd6db commit 123b668

5 files changed

Lines changed: 16 additions & 11 deletions

File tree

plugins/experimental/ja4_fingerprint/plugin.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -358,9 +358,9 @@ handle_read_request_hdr(TSCont cont, TSEvent event, void *edata)
358358
return TS_SUCCESS;
359359
}
360360

361-
std::string *fingerprint{static_cast<std::string *>(TSUserArgGet(vconn, *get_user_arg_index()))};
362-
if (fingerprint) {
363-
append_JA4_headers(cont, txnp, fingerprint);
361+
JA4_data *data{static_cast<JA4_data *>(TSUserArgGet(vconn, *get_user_arg_index()))};
362+
if (data) {
363+
append_JA4_headers(cont, txnp, &data->fingerprint);
364364
} else {
365365
Dbg(dbg_ctl, "No JA4 fingerprint attached to vconn!");
366366
}
@@ -444,7 +444,7 @@ handle_vconn_close(TSCont /* cont ATS_UNUSED */, TSEvent event, void *edata)
444444
return TS_SUCCESS;
445445
}
446446
TSVConn const ssl_vc{static_cast<TSVConn>(edata)};
447-
delete static_cast<std::string *>(TSUserArgGet(ssl_vc, *get_user_arg_index()));
447+
delete static_cast<JA4_data *>(TSUserArgGet(ssl_vc, *get_user_arg_index()));
448448
TSUserArgSet(ssl_vc, *get_user_arg_index(), nullptr);
449449
TSVConnReenable(ssl_vc);
450450
return TS_SUCCESS;

plugins/experimental/txn_box/plugin/include/txn_box/Config.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -559,16 +559,18 @@ class Config
559559
/// Set of named configuration storage objects.
560560
std::unordered_map<swoc::TextView, swoc::MemSpan<void>, std::hash<std::string_view>> _named_objects;
561561

562+
/// For localizing data at a configuration level, primarily strings.
563+
/// Declared before _roots so it is destroyed after _roots, since directives
564+
/// in _roots reference memory allocated from this arena.
565+
swoc::MemArena _arena;
566+
562567
/// Top level directives for each hook. Always invoked.
563568
std::array<std::vector<Directive::Handle>, std::tuple_size<Hook>::value> _roots;
564569

565570
/// Largest number of directives across the hooks. These are updated during
566571
/// directive load, if needed. This includes the top level directives.
567572
std::array<size_t, std::tuple_size<Hook>::value> _directive_count{0};
568573

569-
/// For localizing data at a configuration level, primarily strings.
570-
swoc::MemArena _arena;
571-
572574
/// Additional clean up to perform when @a this is destroyed.
573575
swoc::IntrusiveDList<Finalizer::Linkage> _finalizers;
574576

plugins/slice/server.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ update_object_size(TSHttpTxn txnp, int64_t size, Config &config)
9797
if (urlstr != nullptr) {
9898
if (size <= 0) {
9999
DEBUG_LOG("Ignoring invalid content length for %.*s: %" PRId64, urllen, urlstr, size);
100+
TSfree(urlstr);
100101
return;
101102
}
102103

plugins/stats_over_http/stats_over_http.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -777,7 +777,8 @@ stats_origin(TSCont contp, TSEvent /* event ATS_UNUSED */, void *edata)
777777
icontp = TSContCreate(stats_dostuff, TSMutexCreate());
778778

779779
if (path_had_explicit_format) {
780-
Dbg(dbg_ctl, "Path had explicit format, ignoring any Accept header: %s", request_path_suffix.data());
780+
Dbg(dbg_ctl, "Path had explicit format, ignoring any Accept header: %.*s", static_cast<int>(request_path_suffix.size()),
781+
request_path_suffix.data());
781782
my_state->output_format = format_per_path;
782783
} else {
783784
// Check for an Accept header to determine response type.

tests/tools/plugins/async_engine.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,12 @@ async_destroy(ENGINE *e ATS_UNUSED)
173173
static void
174174
wait_cleanup(ASYNC_WAIT_CTX *ctx ATS_UNUSED, const void *key ATS_UNUSED, OSSL_ASYNC_FD readfd, void *pvwritefd)
175175
{
176-
OSSL_ASYNC_FD *pwritefd = (OSSL_ASYNC_FD *)pvwritefd;
176+
OSSL_ASYNC_FD *pwritefd = (OSSL_ASYNC_FD *)pvwritefd;
177+
OSSL_ASYNC_FD writefd_v = *pwritefd;
177178
close(readfd);
178-
close(*((OSSL_ASYNC_FD *)pwritefd));
179+
close(writefd_v);
179180
OPENSSL_free(pwritefd);
180-
fprintf(stderr, "Cleanup %d and %d\n", readfd, *pwritefd);
181+
fprintf(stderr, "Cleanup %d and %d\n", readfd, writefd_v);
181182
}
182183

183184
#define DUMMY_CHAR 'X'

0 commit comments

Comments
 (0)