-
Notifications
You must be signed in to change notification settings - Fork 278
Fixed arbitrary code execution exploit by preventing deserialzation of objects by default #1142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed arbitrary code execution exploit by preventing deserialzation of objects by default #1142
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a security-focused enhancement to object serialization in the engine. It adds boolean control parameters ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
a610c2c to
fd52a25
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/variant/variant_parser.cpp (1)
1200-1338: TypedDictionary[...]parsing lacks security guards present inArray[...]parsing (object-deserialization bypass)In the
"Array"branch (lines ~1115-1125), when the element type isResource/SubResource/ExtResource, there is an explicit check:if (!p_allow_objects) { r_err_str = R"(Object decoding is prevented because "allow_objects" is false)"; return ERR_UNAUTHORIZED; }followed by
err = parse_value(token, resource, p_stream, line, r_err_str, p_res_parser, p_allow_objects);In contrast, the
"Dictionary"branch (lines 1230–1235 and 1275–1280) has no such guard. It directly calls:
err = parse_value(token, resource, p_stream, line, r_err_str, p_res_parser);(withoutp_allow_objectsargument)- Later:
err = _parse_dictionary(values, p_stream, line, r_err_str, p_res_parser);(withoutp_allow_objectsargument)Both
parse_valueand_parse_dictionaryhavebool p_allow_objects = falseas default in the header, so these call sites silently use the default instead of propagating the caller's setting. This creates an asymmetry:Array[Resource]rejects Resource type annotations whenp_allow_objects = false, butDictionary[Resource, ...]allows them, bypassing the intended security restriction.To close this gap and maintain consistency with the
Arraypath:
Add a guard before attempting to parse Resource/SubResource/ExtResource in the Dictionary branch (similar to Array):
if (!p_allow_objects) { r_err_str = R"(Object decoding is prevented because "allow_objects" is false)"; return ERR_UNAUTHORIZED; }Explicitly forward
p_allow_objectson allparse_valueand_parse_dictionarycalls within the Dictionary branch:err = parse_value(token, resource, p_stream, line, r_err_str, p_res_parser, p_allow_objects); ... err = _parse_dictionary(values, p_stream, line, r_err_str, p_res_parser, p_allow_objects);
🧹 Nitpick comments (8)
editor/import/dynamic_font_import_settings.cpp (1)
854-862: Opting this .import load into object deserializationThe extra
truecorrectly adapts to the newConfigFile::load(path, allow_objects)signature and preserves existing behavior for font.importfiles, which are editor‑owned and local. Consider a brief comment here (or near the API declaration) noting that this path intentionally allows object values, to avoid future confusion about the security implications of the flag.editor/import/resource_importer_texture.cpp (1)
100-156: Round‑tripping texture.importconfigs with object supportPassing
trueto bothcf->load(src_path, true);andcf->save(src_path, true);keeps texture.importfiles compatible if they contain object‑typed values, which is important for existing projects. Since these files are generated and owned by the editor, this explicit opt‑in is reasonable; you may optionally add a short comment to make the security intent of the flag clear at one of these central call sites.doc/classes/FileAccess.xml (1)
307-315: Docs correctly describe object (de)serialization flags; minor wording nitThe updated descriptions for
get_var(allow_objects)andstore_var(value, full_objects)align with the new safe‑by‑default behavior and clearly reference the underlyingbytes_to_var[_with_objects]/var_to_bytes[_with_objects]helpers and RCE risk. Only minor nit: in the last sentence, “an usage flag” should be “a usage flag”.Also applies to: 560-568
core/variant/variant_parser.cpp (1)
2034-2374: Object serialization gating viap_full_objectsis solid; consider aligning script-typed dictionaries with arrays
VariantWriter::writenow:
- Adds
bool p_full_objects, OR’d with theapplication/run/disable_modified_security_assistanceproject setting.- Emits
"null"plus a warning instead of serializingVariant::OBJECTwhen!p_full_objects, preventing arbitrary object graphs from being written inadvertently.- Properly threads
p_full_objectsinto recursivewrite(...)calls for objects, dictionaries, arrays, and their contents.- Grows
write_to_stringto pass the new flag.In the typed container branches:
- For typed arrays, when
array.get_typed_script()is valid, you now gate emitting aResource(...)reference for that script onp_full_objects, falling back to class name + warning when full object encoding is disabled.- For typed dictionaries, the analogous key/value
*_scriptbranches still always try to encode the script as aResource(...)reference regardless ofp_full_objects.To keep the “no objects when
p_full_objects == false” contract easy to reason about, it would be safer and more consistent to apply the same gating pattern to the dictionary key/value script branches as you did for arrays (i.e., only attempt to serialize scripts viap_encode_res_func/encode_resource_reference()whenp_full_objectsis true, and otherwise fall back tokey_class_name/value_class_namewith a warning).Also applies to: 2386-2405, 2421-2434, 2613-2617
editor/run/editor_run.cpp (1)
160-163: Passing the stop shortcut viaVariantWriter::write_to_stringlooks correctThis cleanly serializes the
editor/stop_running_projectshortcut (withp_full_objects = true) into__GODOT_EDITOR_STOP_SHORTCUT__for all spawned instances. Given this only touches local editor state, ignoring theErrorreturn fromwrite_to_stringis acceptable, though you may optionally want to log if it ever fails.tests/core/io/test_config_file.h (1)
119-138: Excellent security-focused test case.The test properly validates the core security enhancement:
- Default behavior (line 128) correctly rejects object deserialization with
ERR_UNAUTHORIZED- Explicit opt-in (line 134) allows object parsing when
p_allow_objects=true- Appropriate error suppression on line 127-129 for expected rejection
- Defensive check on line 132 confirms the key is not present after rejection
Optional: Consider additional test scenarios
For more comprehensive coverage, you might consider testing:
- Multiple objects in a single config file
- Mixed content (objects and non-objects in same config)
- Verifying the parsed object is actually usable (e.g., checking properties beyond just type)
However, these are nice-to-have enhancements rather than requirements.
modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs (1)
606-610: String–Variant helpers correctly separate safe vs. object-capable (de)serializationThe changes to
StrToVar/VarToStrto passgodot_bool.False, plus the newStrToVarWithObjects/VarToStrWithObjectsthat passgodot_bool.True, cleanly harden the default behavior while still exposing an explicit opt‑in for object (de)serialization. Interop usage and disposal patterns (using godotStr,using ret,CreateTakingOwnershipOfDisposableValue) remain correct, and the XML docs clearly document the security implications and the intended pairings. This is a solid, non-breaking API shape for the new security model (existing code that relied on object round‑trip will need to switch to the*WithObjectsvariants consciously).Also applies to: 620-625, 627-641, 671-674, 688-693, 695-707
editor/file_system/editor_file_system.cpp (1)
618-619: Object deserialization kept only for editor‑owned resource metadata; verify_get_import_dest_pathsdefaultAll these
VariantParser::parse_tag*call sites are parsing.import/.md5/ resource text from the project, so explicitly opting intop_allow_objects=true(or doing so via ConfigFile elsewhere) is consistent with the security goal of disabling objects only for generic data parsing.One outlier is
_get_import_dest_paths(), which still callsparse_tag_assign_eof(&stream, ..., nullptr, true)without the finalp_allow_objectsargument and will therefore use the new default (likelyfalse). If an importer ever writes object‑typed data into.import(e.g. viametadata), this helper could now fail to parse where it previously succeeded, subtly impacting reimport MD5 tracking.Consider making
_get_import_dest_paths()explicit as well if you want to preserve old semantics for all.importconsumers.Also applies to: 703-705, 930-931, 956-956, 1085-1085, 1142-1142, 1171-1171, 1197-1198, 1240-1241, 1282-1282, 1320-1320, 1364-1364
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
core/config/project_settings.cppcore/io/config_file.compat.inccore/io/config_file.cppcore/io/config_file.hcore/io/resource_importer.cppcore/io/resource_loader.cppcore/variant/variant.cppcore/variant/variant_parser.cppcore/variant/variant_parser.hcore/variant/variant_utility.cppcore/variant/variant_utility.hdoc/classes/@GlobalScope.xmldoc/classes/ConfigFile.xmldoc/classes/FileAccess.xmldoc/classes/ProjectSettings.xmleditor/debugger/editor_file_server.cppeditor/docks/filesystem_dock.cppeditor/docks/import_dock.cppeditor/export/editor_export_platform.cppeditor/file_system/editor_file_system.cppeditor/import/3d/scene_import_settings.cppeditor/import/audio_stream_import_settings.cppeditor/import/dynamic_font_import_settings.cppeditor/import/resource_importer_texture.cppeditor/project_manager/project_dialog.cppeditor/project_manager/project_list.cppeditor/run/editor_run.cppeditor/scene/3d/gpu_particles_collision_sdf_editor_plugin.cppmain/main.cppmisc/extension_api_validation/4.3-stable.expectedmodules/gdscript/tests/scripts/runtime/features/metatypes.gdmodules/gltf/gltf_document.cppmodules/mono/glue/GodotSharp/GodotSharp/Core/GD.csmodules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.csmodules/mono/glue/runtime_interop.cppscene/resources/resource_format_text.cpptests/core/io/test_config_file.htests/core/variant/test_variant.h
🧰 Additional context used
🧬 Code graph analysis (16)
core/config/project_settings.cpp (1)
core/variant/variant_parser.cpp (4)
parse_tag_assign_eof(1908-1978)parse_tag_assign_eof(1908-1908)write_to_string(2613-2617)write_to_string(2613-2613)
modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs (2)
modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs (22)
godotsharp_str_to_var(600-600)godot_bool(42-42)godot_bool(50-51)godot_bool(68-68)godot_bool(100-100)godot_bool(159-160)godot_bool(242-242)godot_bool(317-317)godot_bool(411-411)godot_bool(419-419)godot_bool(444-446)godot_bool(465-466)godot_bool(473-473)godot_bool(475-476)godot_bool(489-489)godot_bool(491-491)godot_bool(551-551)godot_variant(162-163)godot_variant(183-184)godot_variant(338-338)godotsharp_var_to_str(605-605)godot_string(248-248)modules/mono/glue/runtime_interop.cpp (4)
godotsharp_str_to_var(1502-1517)godotsharp_str_to_var(1502-1502)godotsharp_var_to_str(1496-1500)godotsharp_var_to_str(1496-1496)
tests/core/variant/test_variant.h (1)
core/variant/variant_parser.h (1)
VariantParser(39-167)
core/io/resource_loader.cpp (1)
core/variant/variant_parser.cpp (2)
parse_tag_assign_eof(1908-1978)parse_tag_assign_eof(1908-1908)
core/io/resource_importer.cpp (1)
core/variant/variant_parser.cpp (2)
parse_tag_assign_eof(1908-1978)parse_tag_assign_eof(1908-1908)
editor/run/editor_run.cpp (1)
core/variant/variant_parser.cpp (2)
write_to_string(2613-2617)write_to_string(2613-2613)
core/variant/variant_utility.cpp (2)
core/variant/variant_parser.cpp (4)
write_to_string(2613-2617)write_to_string(2613-2613)parse(1980-1992)parse(1980-1980)core/io/config_file.cpp (2)
parse(270-274)parse(270-270)
core/variant/variant_parser.cpp (2)
core/io/config_file.cpp (2)
parse(270-274)parse(270-270)core/variant/variant.cpp (8)
p_variant(866-868)p_variant(866-866)p_variant(870-879)p_variant(870-870)p_variant(881-889)p_variant(881-881)p_variant(2655-2796)p_variant(2655-2655)
core/io/config_file.cpp (1)
core/variant/variant_parser.cpp (4)
write_to_string(2613-2617)write_to_string(2613-2613)parse(1980-1992)parse(1980-1980)
scene/resources/resource_format_text.cpp (1)
core/variant/variant_parser.cpp (6)
parse_tag_assign_eof(1908-1978)parse_tag_assign_eof(1908-1908)parse_tag(1892-1906)parse_tag(1892-1892)write_to_string(2613-2617)write_to_string(2613-2613)
core/variant/variant.cpp (1)
core/variant/variant_parser.cpp (2)
write_to_string(2613-2617)write_to_string(2613-2613)
tests/core/io/test_config_file.h (1)
core/io/config_file.h (1)
ConfigFile(40-97)
main/main.cpp (1)
core/variant/variant_parser.cpp (2)
parse_tag_assign_eof(1908-1978)parse_tag_assign_eof(1908-1908)
modules/mono/glue/runtime_interop.cpp (2)
core/variant/variant_parser.cpp (4)
write_to_string(2613-2617)write_to_string(2613-2613)parse(1980-1992)parse(1980-1980)core/io/config_file.cpp (2)
parse(270-274)parse(270-270)
core/io/config_file.h (1)
core/io/config_file.cpp (26)
_internal_load(261-268)_internal_load(261-261)_internal_save(193-213)_internal_save(193-193)_parse(276-308)_parse(276-276)_bind_methods(314-342)_bind_methods(314-314)save(147-156)save(147-147)load(215-224)load(215-215)parse(270-274)parse(270-270)encode_to_text(125-145)encode_to_text(125-125)clear(310-312)clear(310-310)load_encrypted(226-241)load_encrypted(226-226)load_encrypted_pass(243-259)load_encrypted_pass(243-243)save_encrypted(158-173)save_encrypted(158-158)save_encrypted_pass(175-191)save_encrypted_pass(175-175)
core/variant/variant_parser.h (1)
core/variant/variant_parser.cpp (20)
_parse_dictionary(1701-1765)_parse_dictionary(1701-1701)_parse_array(1661-1699)_parse_array(1661-1661)_parse_tag(1767-1890)_parse_tag(1767-1767)parse_tag(1892-1906)parse_tag(1892-1892)parse_tag_assign_eof(1908-1978)parse_tag_assign_eof(1908-1908)parse_value(679-1659)parse_value(679-679)get_token(164-522)get_token(164-164)parse(1980-1992)parse(1980-1980)write(2034-2605)write(2034-2034)write_to_string(2613-2617)write_to_string(2613-2613)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🍏 iOS / Template (target=template_release)
🔇 Additional comments (29)
editor/import/audio_stream_import_settings.cpp (1)
436-452: Consistent use of allow_objects=true for audio.importfilesUsing
config_file->load(p_path + ".import", true);is consistent with the new API and keeps existing audio import metadata readable, including any object-typed values. Given these.importfiles are editor‑generated project assets, this explicit opt‑in to object deserialization looks appropriate.core/config/project_settings.cpp (2)
884-914: Project settings keep full object support on load/save
VariantParser::parse_tag_assign_eof(..., true, true);andVariantWriter::write_to_string(value, vstr, nullptr, nullptr, true, true);explicitly allow object‑typed variants inproject.godot(and the binary format) and serialize them accordingly. That preserves existing behavior for project settings while the new security model tightens defaults elsewhere.Given project files are local and under the developer’s control, this seems like the right trade‑off; just ensure no untrusted remote data is ever funneled through these project‑settings loaders.
Also applies to: 1084-1094
1561-1576: Security toggle setting is registered with a safe default
GLOBAL_DEF_RST("application/run/disable_modified_security_assistance", false);correctly adds the new project setting with a default that keeps the strengthened security behavior enabled unless the user explicitly opts out. Using theRSTvariant is consistent with other core runtime settings that may need reset semantics.Looks good; just verify that the docs and any UI around this setting clearly communicate that setting it to
truere‑enables unsafe object deserialization semantics.core/variant/variant.cpp (1)
3491-3495: Preserving full object representation inget_construct_string()Adapting
get_construct_string()toVariantWriter::write_to_string(*this, vars, nullptr, nullptr, true, true);keeps its behavior compatible and allows full object serialization in the construct string, which is what callers of this debugging/introspection helper typically expect. This looks correct; just avoid using these construct strings as a parsing surface for untrusted input now that safer, object‑disabled paths exist elsewhere.core/variant/variant_utility.cpp (1)
1056-1095: *New _with_objects helpers and allow_objects wiring look consistent
var_to_str/str_to_varnow usefull_objects = false/allow_objects = falsewhile the new*_with_objectsvariants opt intotrue, and all four functions are properly registered as utility functions. Error handling instr_to_var*still results in a defaultVarianton parse failure, preserving caller expectations.Also applies to: 1811-1816
editor/project_manager/project_list.cpp (1)
587-599: Allowing objects when loading localproject.godotis appropriateSwitching to
cf->load(conf, true);keeps project metadata loading behavior unchanged for on‑disk projects, while the new defaultallow_objects = falsecan still be used in genuinely untrusted contexts (e.g., networked config). This is a reasonable compatibility choice for the project manager.editor/debugger/editor_file_server.cpp (1)
60-66: UsingConfigFile::load(..., true)for local.importmetadata is fineThe
.importfiles read here are local editor artifacts, so allowing objects for this load maintains compatibility when parsing remap info without opening a new attack surface. The rest of the change (added arg only) is non‑disruptive.modules/gdscript/tests/scripts/runtime/features/metatypes.gd (1)
13-15: Test updated to use object-enabled stringification correctlySwitching to
var_to_str_with_objects(value)incheck_gdscript_native_class()matches the new API split while preserving the expected output formatting for the class name, so the metatypes test continues to cover the object‑serialization path explicitly.modules/gltf/gltf_document.cpp (1)
4058-4076: Explicitallow_objects = trueon.importConfigFile load – confirm this is desired under new security model
config->load(file_path + ".import", true);now unconditionally enables object deserialization when reading the.importconfig forgenerator_parameters. Given the PR’s goal of blocking object deserialization by default, please double‑check that this site is intended to bypass the new default and that.importcontents are always trusted in your threat model. If the intent is to keep this path governed by the new project setting, consider threading the setting’s value in here instead of hard‑codingtrue.tests/core/variant/test_variant.h (1)
1914-1955: Excellent test coverage for object serialization security feature.This test case thoroughly validates the new object handling behavior:
- Verifies that default serialization writes "null" instead of object data (line 1925)
- Confirms full object serialization when
p_full_objects=true(lines 1928-1930)- Validates that parsing fails with ERR_UNAUTHORIZED when
p_allow_objects=false(lines 1938-1942)- Ensures successful round-trip when both flags are enabled (lines 1948-1954)
The test properly exercises both the security-by-default behavior and the opt-in mechanism for object handling.
core/variant/variant_utility.h (1)
149-151: Good API design for opt-in object serialization.The new
var_to_str_with_objectsandstr_to_var_with_objectsfunctions provide an explicit, discoverable way to enable object serialization when needed. This follows the principle of secure-by-default while maintaining flexibility.The naming convention clearly indicates these functions handle objects differently from the standard
var_to_str/str_to_var, making the security implications transparent to API users.editor/export/editor_export_platform.cpp (1)
1338-1351: Please confirm opting back into object (de)serialization for.importfiles
config->load(path + ".import", true)and the correspondingconfig->encode_to_text(true)calls explicitly re-enable object handling for.importfiles, whereas the new defaults are meant to keep object encoding/decoding off for security.If
.importcontents are expected to remain primitive-only (strings, numbers, arrays, dictionaries), it would be safer to leave these calls at the default (no objects) to fully benefit from the RCE hardening. If there are known use cases that legitimately require objects inside.import, can you document that assumption and confirm that this surface is acceptable in the threat model?Also applies to: 1436-1445, 1503-1513
main/main.cpp (1)
3005-3012: Allowing objects when parsing early editor settings looks appropriatePassing
truefor the newp_allow_objectsparameter inVariantParser::parse_tag_assign_eof(&stream, ..., &rp_new, true, true)when reading the editor settings file makes sense: this is user-local configuration, not remotely supplied data, so opting into full object deserialization here doesn’t undermine the new default hardening for untrusted inputs.editor/docks/import_dock.cpp (1)
104-104: Opting into object-capable.importConfigFile load/save looks consistentAll updated
ConfigFile::load/savecalls for*.importnow explicitly passtruefor the new boolean parameter. This keeps import configuration behavior unchanged under the stricter defaults while scoping object-capable parsing to editor-owned.importfiles only. The error handling and overall control flow are preserved, so this looks correct and aligned with the intended security change.Also applies to: 238-238, 422-422, 557-557, 638-638, 685-685
editor/import/3d/scene_import_settings.cpp (1)
793-794: Import settings.importload correctly opts into object-aware parsingSwitching
config->load(p_path + ".import")toconfig->load(p_path + ".import", true)is consistent with other import-related call sites and ensures this code path continues to handle any object-valued params under the new safer defaults. Error handling and the subsequent logic remain unchanged.editor/docks/filesystem_dock.cpp (1)
1249-1250: ConfigFile.importloads updated correctly for new APIBoth
_select_fileand_update_import_docknow callconfig->load(fpath + ".import", true), matching the newConfigFile::loadsignature and keeping existing.importsemantics under the hardened defaults. Since these are editor-only reads of on-disk import metadata and the surrounding logic is unchanged, the change is safe and consistent.Also applies to: 3919-3920
core/io/resource_importer.cpp (1)
78-79: Allowing objects for.importparsing and ConfigFile I/O here is appropriateThese call sites are only touching engine‑generated
.importconfigs, so explicitly passingp_simple_tag=true, p_allow_objects=trueandload/save(..., true)is the right way to preserve existing metadata/params behavior while keeping the new secure default elsewhere.Also applies to: 352-353, 595-601
editor/file_system/editor_file_system.cpp (1)
2590-2591: ConfigFile and VariantWriter calls correctly opt into full object support for.importUsing
config->load(..., true)/cf->load(..., true)andVariantWriter::write_to_string(..., true, true)when reading/writing.importfiles ensures complex Variant params and metadata (including potential Resource/Object values) continue to round‑trip even though the underlying parser/writer is now strict by default. This keeps importer configuration fully compatible while leaving other text‑based APIs in the safer mode.Also applies to: 2717-2717, 2807-2808, 2965-2965, 2990-2990
modules/mono/glue/runtime_interop.cpp (1)
1502-1517:godotsharp_str_to_varcorrectly threadsp_allow_objectsthrough VariantParserForwarding
p_allow_objectsintoVariantParser::parseand returning an error string Variant on failure preserves previous behavior while integrating the new allow‑objects guard. This matches the security intent and should be safe for existing Mono callers.scene/resources/resource_format_text.cpp (2)
275-276: Explicitly allowing objects in text resource/scene parsing is consistent with the new security modelAll these
VariantParser::parse_tag*call sites operate solely on.tscn/.tresresource text (and related dependency scans). Opting intop_allow_objects=truehere preserves prior behavior for project assets while leaving generic parsing APIs in their safer default “no objects” mode.Given these files are already trusted input in Godot/Redot’s threat model, this looks correct and keeps the RCE fix focused on JSON/config/multiplayer data, not engine resource formats.
Also applies to: 359-359, 382-382, 477-478, 597-598, 727-727, 931-931, 956-956, 1085-1085, 1142-1142, 1171-1171, 1197-1198, 1240-1241, 1282-1282, 1320-1320, 1364-1364
1951-1953: Usingp_full_objects=truewhen writing.tres/.tscnpreserves full Variant expressivenessThese
VariantWriter::write_to_string(..., _write_resources, this, use_compat, true)calls control how resource properties, node instances/placeholders, node properties, and connection binds are serialized in text resource/scene files.Setting
p_full_objects=truehere is necessary to keep existing.tres/.tscncontent (including embedded resources and other object‑valued fields) serializable despite the stricter global defaults, and is scoped to trusted on‑disk assets.Also applies to: 2015-2016, 2022-2023, 2030-2033, 2065-2066
modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs (1)
600-601: Interop signatures align with new object-handling flagsAdding
p_allow_objects/p_full_objectstogodotsharp_str_to_varandgodotsharp_var_to_strcleanly mirrors the native side changes and preserves method ordering forUnmanagedCallbacks. As long as the glue inruntime_interop.cppand the managedGD.*wrappers were regenerated/updated to match these new signatures, this looks correct.Also applies to: 605-605
doc/classes/@GlobalScope.xml (1)
1434-1457: GlobalScope serialization docs clearly separate safe vs. object-enabled APIsThe updated
str_to_var/var_to_strdescriptions and the new*_with_objectsmethods accurately document the new behavior (no objects by default, explicit opt‑in with strong RCE warnings). This is consistent with the engine-side flags and should help users choose the safer API by default.Also applies to: 1547-1549, 1569-1575
core/io/config_file.h (1)
45-51: ConfigFile API cleanly introduces allow/full object flags and compat hooksThe header changes consistently thread
p_allow_objects/p_full_objectsthrough_internal_load,_internal_save,_parse, and all public load/save/parse/encode/encrypted methods, with safe defaults (false). The deprecated*_bind_compat_redot764declarations plus_bind_compatibility_methods()give a clear separation between legacy bindings and the new, more explicit API. This matches the cpp and docs behavior.Also applies to: 55-69, 84-89, 92-96
core/variant/variant_parser.h (1)
44-45: Parser/writer flags support object gating and fix uninitialized bufferZero-initializing
readahead_bufferremoves the uninitialized array warning and avoids any undefined reads before the first fill. Addingp_allow_objects(defaulting tofalse) through the private and public parsing APIs, andp_full_objectsto the writer APIs, provides a consistent mechanism to disable object decoding/encoding by default while still allowing explicit opt‑in where needed. The signatures are self-consistent and align with the rest of the PR.Also applies to: 147-158, 165-167
doc/classes/ConfigFile.xml (1)
101-108: ConfigFile documentation correctly reflects allow/full object flags and risksThe added
allow_objects/full_objectsparameters and the updated descriptions accurately describe the new behavior and its security implications, and tie it back to the sharedstr_to_var/var_to_strmechanisms. The warnings about using these options only for trusted data are appropriate and aligned with the engine changes.Also applies to: 167-173, 179-183, 189-193, 195-205, 207-215, 221-225, 231-235
core/io/config_file.compat.inc (1)
33-81: Compat wrappers route legacy APIs through new safe implementationsThe redot764 compatibility methods delegate all legacy
save/load/parse/encode_to_text(and encrypted variants) calls to the new signatures withallow_objects/full_objectshard-coded tofalse, so old entry points remain available but now benefit from the safer defaults. The_bind_compatibility_methods()registration usingbind_compatibility_methodis consistent with the rest of the engine’s compat machinery.core/io/config_file.cpp (2)
34-35: ConfigFile save/encode paths correctly honor full_objects flagIncluding
config_file.compat.incand extendingencode_to_text,save, and_internal_saveto acceptp_full_objectsand pass it intoVariantWriter::write_to_stringwires the new “full object” behavior through all save/encode code paths, including encrypted saves. Defaults remainfalse, so object encoding is opt‑in. The changes are internally consistent and align with the updated docs.Also applies to: 125-156, 158-213
215-275: ConfigFile load/parse now thread allow_objects down to the parserThe new
p_allow_objectsparameter onload,load_encrypted,load_encrypted_pass,parse,_internal_load, and_parse, and its propagation intoVariantParser::parse_tag_assign_eof, ensure that object deserialization is disabled by default and only enabled when explicitly requested. Reinitializingassignandvalueinside the loop in_parsealso makes the parsing logic more robust against stale data. The bindings expose the flag with sane defaults across all load/parse/encrypted methods.Also applies to: 261-307, 327-339
fd52a25 to
41e0413
Compare
Adapted from commit dalexeev/godot@377ff79 Add "disable_modified_security_assistance" project setting Warns about application performing unsafe behavior by default when enabled Rename compat functions to redot764 Fix uninitialized array warning Co-authored-by: Spartan322 <Megacake1234@gmail.com>
41e0413 to
1727370
Compare
Adapted from commit dalexeev/godot@377ff79
Fixes #760
This fixes an arbitrary code execution exploit in Godot's serialization system that would allow a malicious actor to inject code into an object, that would get executed during the deserialization process.
This could be injected into things like JSON objects and data commonly exchanged in multiplayer environments allowing an attacker to execute their own scripts and code on remote machines such as other players or the server.
Logic was added to block this behavior by default, and an error is thrown instead. A setting was also added to re-enable the old behavior if a developer really needs this functionality.
Add "disable_modified_security_assistance" project setting
Warns about application performing unsafe behavior by default when enabled
Rename compat functions to redot764
Fix uninitialized array warning
Summary by CodeRabbit
Release Notes
New Features
var_to_str_with_objects()andstr_to_var_with_objects()utility functions for variant serialization and deserialization with object support.application/run/disable_modified_security_assistancefor security configuration.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.