make signedness explicit in count checks

This commit is contained in:
Martin Michelsen
2022-10-09 12:50:34 -07:00
parent f088454c25
commit f18953c31e
2 changed files with 12 additions and 15 deletions
+6 -3
View File
@@ -865,7 +865,8 @@ static HandlerResult S_6x(shared_ptr<ServerState>,
const auto& cmd = check_size_t<G_AttackFinished_6x46>(data,
offsetof(G_AttackFinished_6x46, entries),
sizeof(G_AttackFinished_6x46));
if ((cmd.count > 11) || (cmd.count > cmd.header.size - 2)) {
size_t allowed_count = min<size_t>(cmd.header.size - 2, 11);
if (cmd.count > allowed_count) {
session.log.warning("Blocking subcommand 6x46 with invalid count");
return HandlerResult::Type::SUPPRESS;
}
@@ -873,7 +874,8 @@ static HandlerResult S_6x(shared_ptr<ServerState>,
const auto& cmd = check_size_t<G_CastTechnique_6x47>(data,
offsetof(G_CastTechnique_6x47, targets),
sizeof(G_CastTechnique_6x47));
if ((cmd.target_count > 10) || (cmd.target_count > cmd.header.size - 2)) {
size_t allowed_count = min<size_t>(cmd.header.size - 2, 10);
if (cmd.target_count > allowed_count) {
session.log.warning("Blocking subcommand 6x47 with invalid count");
return HandlerResult::Type::SUPPRESS;
}
@@ -881,7 +883,8 @@ static HandlerResult S_6x(shared_ptr<ServerState>,
const auto& cmd = check_size_t<G_SubtractPBEnergy_6x49>(data,
offsetof(G_SubtractPBEnergy_6x49, entries),
sizeof(G_SubtractPBEnergy_6x49));
if ((cmd.entry_count > 14) || (cmd.entry_count > cmd.header.size - 2)) {
size_t allowed_count = min<size_t>(cmd.header.size - 3, 14);
if (cmd.entry_count > allowed_count) {
session.log.warning("Blocking subcommand 6x49 with invalid count");
return HandlerResult::Type::SUPPRESS;
}
+6 -12
View File
@@ -356,10 +356,8 @@ static void on_subcommand_attack_finished(shared_ptr<ServerState> s,
const string& data) {
const auto& cmd = check_size_sc<G_AttackFinished_6x46>(data,
offsetof(G_AttackFinished_6x46, entries), sizeof(G_AttackFinished_6x46));
// The PSO GC client doesn't check bounds correctly when handling this
// command, which could cause it to byteswap parts of the stack, including the
// next frame pointer and return address. This of course leads to a crash.
if ((cmd.count > 11) || (cmd.count > cmd.header.size - 2)) {
size_t allowed_count = min<size_t>(cmd.header.size - 2, 11);
if (cmd.count > allowed_count) {
throw runtime_error("invalid attack finished command");
}
on_subcommand_forward_check_size_client(s, l, c, command, flag, data);
@@ -370,10 +368,8 @@ static void on_subcommand_cast_technique(shared_ptr<ServerState> s,
const string& data) {
const auto& cmd = check_size_sc<G_CastTechnique_6x47>(data,
offsetof(G_CastTechnique_6x47, targets), sizeof(G_CastTechnique_6x47));
// The PSO GC client doesn't check bounds correctly when handling this
// command, which could cause it to byteswap parts of the stack, including the
// next frame pointer and return address. This of course leads to a crash.
if ((cmd.target_count > 10) || (cmd.target_count > cmd.header.size - 2)) {
size_t allowed_count = min<size_t>(cmd.header.size - 2, 10);
if (cmd.target_count > allowed_count) {
throw runtime_error("invalid cast technique command");
}
on_subcommand_forward_check_size_client(s, l, c, command, flag, data);
@@ -384,10 +380,8 @@ static void on_subcommand_subtract_pb_energy(shared_ptr<ServerState> s,
const string& data) {
const auto& cmd = check_size_sc<G_SubtractPBEnergy_6x49>(data,
offsetof(G_SubtractPBEnergy_6x49, entries), sizeof(G_SubtractPBEnergy_6x49));
// The PSO GC client doesn't check bounds correctly when handling this
// command, which could cause it to byteswap parts of the stack, including the
// next frame pointer and return address. This of course leads to a crash.
if ((cmd.entry_count > 14) || (cmd.entry_count > cmd.header.size - 3)) {
size_t allowed_count = min<size_t>(cmd.header.size - 3, 14);
if (cmd.entry_count > allowed_count) {
throw runtime_error("invalid subtract PB energy command");
}
on_subcommand_forward_check_size_client(s, l, c, command, flag, data);