Unexpected resolution of `${}` in metadata

[TurkeyMcMac]

Minetest version

Minetest 5.6.0-dev-677ac4f78

OS / Hardware

Operating system: Ubuntu 22.04 LTS CPU: Intel® Core™ i5-4200U CPU @ 1.60GHz × 4

Summary

This may not be a bug, but it is confusing and undocumented. When getting data from metadata, if the value found is in the format ${K}, then the value returned is the value of key K. There is a small recursion limit. This behavior could be a problem if you want to store arbitrary strings in metadata and one of the strings happens to be in the ${} format; you will get unexpected results.

Here are two ways this behavior could be removed:

1. Stop Metadata::getString and Metadata::getStringToRef from calling Metadata::resolveString.
2. Set the default recursion parameter of getString and getStringToRef to 2, so that the recursive resolution is only done when encountering the ${} syntax in formspecs.

If this feature is removed, compatibility would be broken, in however minor a way.

Steps to reproduce

Run this code:

local s = core.get_mod_storage()                                                
s:set_string("a", "1111")                                                       
s:set_string("b", "${a}")                                                       
assert(s:get_int("b") == 1111)

[appgurueu]

This is horrible. Setting strings should not have any interpolation side effects. This "feature" shouldn't be documented, it should be ripped out as soon as possible.

[appgurueu]

Perhaps worth noting that this seems to be used in legacy code:

	else if(id == NODEMETA_SIGN) // SignNodeMetadata
	{
		meta->setString("text", deSerializeString16(is));
		//meta->setString("infotext","\"${text}\"");
		meta->setString("infotext",
				std::string("\"") + meta->getString("text") + "\"");
		meta->setString("formspec","field[text;;${text}]");
		return false;
	}

in content_nodemeta.cpp:

$ grep -r '${' --include "*.cpp"
content_nodemeta.cpp:		//meta->setString("infotext","\"${text}\"");
content_nodemeta.cpp:		meta->setString("formspec","field[text;;${text}]");
metadata.cpp:	if (recursion <= 1 && str.substr(0, 2) == "${" && str[str.length() - 1] == '}') {

[rollerozxa]

Oh god burn it with fire, I was looking over some of this compability code a while back and was wondering why all this code only applicable for world formats <23 (pre-0.4.0) should still be left in. Hopefully it has gone enough time that worlds have been converted to something slightly newer and the compat could be dropped (this would be compat for things that are over a decade now at this point).

[sfan5]

Perhaps worth noting that this seems to be used in legacy code:

No it is not, this issue about ${ affecting direct reads from metadata. The formspec interpolation feature may share the same code but this issue isn't about it.

[...], an attacker can inject an arbitrary string into a meta field, triggering a different serialization

No they can't. You even quoted the code that resolves the references, a quick look should tell why this doesn't work.

[appgurueu]

Perhaps worth noting that this seems to be used in legacy code:

No it is not, this issue about ${ affecting direct reads from metadata. The formspec interpolation feature may share the same code but this issue isn't about it.

[...], an attacker can inject an arbitrary string into a meta field, triggering a different serialization

No they can't. You even quoted the code that resolves the references, a quick look should tell why this doesn't work.

Yeah, I just realized and thus deleted my comment.

[erlehmann]

@rubenwardy could we get a quick cdb search results page for the string ${ in mod lua code to figure out if anyone relies on it?

[rubenwardy]

CDB zipgrep doesn't work with special characters like that, it returns 0 results even when escaping with \. This is one of the reasons why it's an internal only tool

[TurkeyMcMac]

By the way, I described a workaround here: https://github.com/minetest/minetest/issues/12167#issuecomment-1190415597

[TurkeyMcMac]

I suppose that a C++ method get_raw_string could be added which would be the same as get_string except it would pass UINT_MAX as the recursion parameter to getString. But this couldn't be polyfilled with Lua.