Add max_depth option to unserialize()

[nikic]

Fixes bug #78549. It should also address https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=17584, https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=17589 and https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=17664.

This adds a new max_depth option to unserialize(), which limits the depth of nested data and prevents stack overflows during unserialization. The default value is 4096, which is not far from the stack overflow limit for debug builds. I think this depth limit is large enough that we can safely enable it by default.

[nikic]

@smalyshev Does this look reasonable?

[TysonAndre]

I had some questions about possible edge cases. (I work on the igbinary serializer, so I was interested in how those would be handled)

[smalyshev]

I like the idea of having it but I am concerned about the surprise effect of breaking somebody's data (and yes, 4096 is a lot, but I've seen lots of weird data, who knows...). How about doing it this way:

• having ini value - something like unserialize.max_depth - with the default of 4096, documented in php.ini and UPGRADING
• making this function default to the value of that ini value

This improves several things:

• Spotting new ini value is easier than spotting new optional value inside optional parameter and figuring out how to change it
• If it breaks anything for you, it's easy to get back to previous state without changing any code
• It's easier to make it much tighter if needed without changing every unserialize call

Generally we're pretty cautious with new ini values, but this looks like a kind of thing that would work well with one.

[nikic]

I've now added an unserialize.max_depth ini option to specify the default depth limit and also changed the behavior of max_depth wrt nested unserialize. If an inner unserialize sets max_depth, then we'll reset the depth to zero as well. When switching back to the outer unserialize the depth/max_depth is restored.

[nikic]

Any thoughts on the naming of the ini setting? I've called it unserialize.max_depth, but we have an existing unserialize_callback_func, so maybe it makes more sense to use unserialize_max_depth (without the dot)?

[KalleZ]

For consistency I prefer without the dot

[Girgias]

Yeah, without the dot for consistency would be my opinion too.

[nikic]

I've renamed the ini setting and merged this as 1806ce9 into 7.4.

[tored]

This change isn't documented in UPGRADING what I can tell.

[nikic]

@tored 1806ce9#diff-7748eb3bfdd3bf962553f6f9f2723c45

[tored]

My apologies, was looking in the master branch and not the php-7.4 branch.