Fix: Disallow unsafe access to baseset version and parameters.

Unsafe parameters are only allowed to be accessed for sprite resolving.

src/newgrf.h

@@ -130,6 +130,7 @@ struct GRFFile {
 	std::vector<std::unique_ptr<struct RoadStopSpec>> roadstops;
 
 	std::vector<uint32_t> param{};
+	std::vector<bool> unsafe_param{};
 
 	std::vector<GRFLabel> 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;
 	}
 };

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;

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 <data>.  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);
 			}

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();

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 */