From c2eb8ba72e76b96f89da3d1f1d06d5b7ade586c7 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Wed, 23 Jul 2025 21:10:22 +0100 Subject: [PATCH] Fix: Disallow unsafe access to baseset version and parameters. Unsafe parameters are only allowed to be accessed for sprite resolving. --- src/newgrf.h | 6 +++++- src/newgrf/newgrf_act7_9.cpp | 19 ++++++++++++++++--- src/newgrf/newgrf_actd.cpp | 20 +++++++++++++++++--- src/newgrf/newgrf_internal.h | 2 +- src/newgrf_spritegroup.cpp | 15 +++++++++++++-- 5 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/newgrf.h b/src/newgrf.h index beb5a0ed4f..895e7568bc 100644 --- a/src/newgrf.h +++ b/src/newgrf.h @@ -130,6 +130,7 @@ struct GRFFile { std::vector> roadstops; std::vector param{}; + std::vector unsafe_param{}; std::vector labels{}; ///< List of labels @@ -164,10 +165,13 @@ struct GRFFile { ~GRFFile(); /** Get GRF Parameter with range checking */ - uint32_t GetParam(uint number) const + uint32_t GetParam(uint number, bool allow_unsafe = false) const { /* Note: We implicitly test for number < this->param.size() and return 0 for invalid parameters. * In fact this is the more important test, as param is zeroed anyway. */ + if (!allow_unsafe) { + if (number < std::size(this->unsafe_param) && this->unsafe_param[number]) return 0; + } return (number < std::size(this->param)) ? this->param[number] : 0; } }; diff --git a/src/newgrf/newgrf_act7_9.cpp b/src/newgrf/newgrf_act7_9.cpp index 4d08d2e67e..a00db5e548 100644 --- a/src/newgrf/newgrf_act7_9.cpp +++ b/src/newgrf/newgrf_act7_9.cpp @@ -113,7 +113,7 @@ void InitializePatchFlags() | (1U << 0x02); // extended string range } -uint32_t GetParamVal(uint8_t param, uint32_t *cond_val) +uint32_t GetParamVal(uint8_t param, uint32_t *cond_val, bool *unsafe) { /* First handle variable common with VarAction2 */ uint32_t value; @@ -149,7 +149,15 @@ uint32_t GetParamVal(uint8_t param, uint32_t *cond_val) default: /* GRF Parameter */ - if (param < 0x80) return _cur_gps.grffile->GetParam(param); + if (param < 0x80) { + if (unsafe != nullptr) { + if (param < _cur_gps.grffile->unsafe_param.size()) { + *unsafe |= _cur_gps.grffile->unsafe_param[param]; + return _cur_gps.grffile->GetParam(param, true); + } + } + return _cur_gps.grffile->GetParam(param); + } /* In-game variable. */ GrfMsg(1, "Unsupported in-game variable 0x{:02X}", param); @@ -234,14 +242,19 @@ static void SkipIf(ByteReader &buf) } } else if (param == 0x88) { /* GRF ID checks */ - GRFConfig *c = GetGRFConfig(cond_val, mask); + bool unsafe = GB(cond_val & mask, 0, 8) == (0xFF & mask) || (c != nullptr && c->flags.Test(GRFConfigFlag::System)); if (c != nullptr && c->flags.Test(GRFConfigFlag::Static) && !_cur_gps.grfconfig->flags.Test(GRFConfigFlag::Static) && _networking) { DisableStaticNewGRFInfluencingNonStaticNewGRFs(*c); c = nullptr; } + if (unsafe) { + GrfMsg(1, "SkipIf: GRFID 0x{:08X} is system GRF, skipping unsafe test", std::byteswap(cond_val)); + return; + } + if (condtype != 10 && c == nullptr) { GrfMsg(7, "SkipIf: GRFID 0x{:08X} unknown, skipping test", std::byteswap(cond_val)); return; diff --git a/src/newgrf/newgrf_actd.cpp b/src/newgrf/newgrf_actd.cpp index 8231c9a192..0a682be87d 100644 --- a/src/newgrf/newgrf_actd.cpp +++ b/src/newgrf/newgrf_actd.cpp @@ -226,6 +226,7 @@ static void ParamSet(ByteReader &buf) oper = GB(oper, 0, 7); } + bool unsafe = false; if (src2 == 0xFE) { if (GB(data, 0, 8) == 0xFF) { if (data == 0x0000FFFF) { @@ -312,6 +313,9 @@ static void ParamSet(ByteReader &buf) } } } else { + /* If the other GRF is a system GRF then this access is unsafe, even if the GRF is not loaded.*/ + unsafe |= (GB(data, 0, 8) == 0xFF); + if (unsafe) GrfMsg(1, "ParamSet: access to parameter {:02X} of GRFID 0x{:08X} marked unsafe", src1, std::byteswap(data)); /* Read another GRF File's parameter */ const GRFFile *file = GetFileByGRFID(data); GRFConfig *c = GetGRFConfig(data); @@ -324,7 +328,11 @@ static void ParamSet(ByteReader &buf) } else if (src1 == 0xFE) { src1 = c->version; } else { - src1 = file->GetParam(src1); + if (!unsafe) { + if (src1 < file->unsafe_param.size()) unsafe |= file->unsafe_param[src1]; + if (unsafe) GrfMsg(1, "ParamSet: access to parameter {:02X} of GRFID 0x{:08X} marked unsafe", src1, std::byteswap(data)); + } + src1 = file->GetParam(src1, true); } } } else { @@ -333,8 +341,8 @@ static void ParamSet(ByteReader &buf) * variables available in action 7, or they can be FF to use the value * of . If referring to parameters that are undefined, a value * of 0 is used instead. */ - src1 = (src1 == 0xFF) ? data : GetParamVal(src1, nullptr); - src2 = (src2 == 0xFF) ? data : GetParamVal(src2, nullptr); + src1 = (src1 == 0xFF) ? data : GetParamVal(src1, nullptr, &unsafe); + src2 = (src2 == 0xFF) ? data : GetParamVal(src2, nullptr, &unsafe); } uint32_t res; @@ -424,6 +432,7 @@ static void ParamSet(ByteReader &buf) break; case 0x8F: { // Rail track type cost factors + if (unsafe) break; extern RailTypeInfo _railtypes[RAILTYPE_END]; _railtypes[RAILTYPE_RAIL].cost_multiplier = GB(res, 0, 8); if (_settings_game.vehicle.disable_elrails) { @@ -476,6 +485,11 @@ static void ParamSet(ByteReader &buf) /* Resize (and fill with zeroes) if needed. */ if (target >= std::size(_cur_gps.grffile->param)) _cur_gps.grffile->param.resize(target + 1); _cur_gps.grffile->param[target] = res; + + if (unsafe) { + if (target >= std::size(_cur_gps.grffile->unsafe_param)) _cur_gps.grffile->unsafe_param.resize(target + 1); + _cur_gps.grffile->unsafe_param[target] = true; + } } else { GrfMsg(7, "ParamSet: Skipping unknown target 0x{:02X}", target); } diff --git a/src/newgrf/newgrf_internal.h b/src/newgrf/newgrf_internal.h index d4001219ec..0a2e65ab38 100644 --- a/src/newgrf/newgrf_internal.h +++ b/src/newgrf/newgrf_internal.h @@ -199,7 +199,7 @@ GRFFile *GetFileByGRFID(uint32_t grfid); GRFError *DisableGrf(StringID message = {}, GRFConfig *config = nullptr); void DisableStaticNewGRFInfluencingNonStaticNewGRFs(GRFConfig &c); bool HandleChangeInfoResult(std::string_view caller, ChangeInfoResult cir, GrfSpecFeature feature, uint8_t property); -uint32_t GetParamVal(uint8_t param, uint32_t *cond_val); +uint32_t GetParamVal(uint8_t param, uint32_t *cond_val, bool *unsafe = nullptr); void GRFUnsafe(ByteReader &); void InitializePatchFlags(); diff --git a/src/newgrf_spritegroup.cpp b/src/newgrf_spritegroup.cpp index 2a904b2916..2e1aef2f22 100644 --- a/src/newgrf_spritegroup.cpp +++ b/src/newgrf_spritegroup.cpp @@ -64,9 +64,20 @@ static inline uint32_t GetVariable(const ResolverObject &object, ScopeResolver * case 0x7D: return object.GetRegister(parameter); - case 0x7F: + case 0x7F: { if (object.grffile == nullptr) return 0; - return object.grffile->GetParam(parameter); + + /* Access to unsafe parameters is allowed when resolving for drawing graphics. */ + bool allow_unsafe = + object.callback == CBID_NO_CALLBACK || + object.callback == CBID_STATION_DRAW_TILE_LAYOUT || + object.callback == CBID_INDTILE_DRAW_FOUNDATIONS || + object.callback == CBID_CANALS_SPRITE_OFFSET || + object.callback == CBID_HOUSE_DRAW_FOUNDATIONS || + object.callback == CBID_AIRPTILE_DRAW_FOUNDATIONS; + + return object.grffile->GetParam(parameter, allow_unsafe); + } default: /* First handle variables common with Action7/9/D */