1. This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn More.

Developers: serialization compatibility changes in 1.5.3

Discussion in 'XenForo Development Discussions' started by XenForo, Dec 1, 2015.

  1. XenForo

    XenForo Company Info Staff Member

    In an effort to provide an extra level of protection for a specific type of vulnerability, XenForo 1.5.3 makes some changes to the default handling of fields that are set as TYPE_SERIALIZED in data writers. While most developers should not need to make changes, there is a backwards compatibility break here in specific cases. This post will address several issues and changes that should be made.



    TYPE_SERIALIZED fields may not serialize objects by default

    By default, any TYPE_SERIALIZED field will error if the value to be serialized contains an object of any type. When setting the value, this will throw an error and prevent execution from continuing. (Note that there may be false positives in extremely rare cases, but this should effectively never happen.)

    If you are storing an object in a field of this type, you should add 'unsafe' => true as an option to the field definition. This will ensure compatibiilty with older versions and XenForo 1.5.3+. You must ensure that you never expose this field to an untrusted user in a way that they can directly set a value (particularly as a string) or there is risk of a security vulnerability.



    Array-type options: value and default values must be arrays

    While this was always the intention, this is now checked explicitly. You should double check that your add-on installs cleanly as if you made a mistake in the definition of an option, installation will be blocked. To fix this, you'll need to ensure that the default value is correctly defined. (This should not affect the vast majority of add-ons, but it's important to check.)



    Safe serialize and unserialize helpers

    You should never expose values that will be passed to unserialize() directly to untrusted users. Doing so risks a serious security vulnerability. If you are in doubt and you need to serialize something which may be passed through user input, you should use json_encode and json_decode.

    However, to provide an extra level of protection, XenForo 1.5.3 has added 2 new functions which you may wish to use:
    • XenForo_Helper_Php::safeUnserialize($serialized) - this function will prevent unserialization if an object is present (PHP 5) or unserialize with an object placeholder instead (PHP 7).
    • XenForo_Helper_Php::safeSerialize($toSerialize) - this function will check if safe unserialization will happen without issue and throw an exception if it won't run as expected. While serialization is safe, this can be used to prevent unsafe data from being saved.
    XenForo itself is now using these two methods in numerous places.
     
    PunKeel, Thomas.B, Xon and 13 others like this.
  2. Jaxel

    Jaxel Well-Known Member

    Does this change any of the procedures to serialized fields? For instance, does data get automatically serialized and unserialized when put and get from the datawriters?

    In the past, I just put the raw array into the datawriter, and it was automatically serialized.
     
  3. Chris D

    Chris D XenForo Developer Staff Member

    That behaviour remains the same. The only difference being the serialization by default will reject data which contains objects unless the field is explicitly opted out of this check by marking it as 'unsafe'.
     
  4. Jaxel

    Jaxel Well-Known Member

    So if you want to use arrays, you just need to add that mark and be done? Easy enough.

    Why would you use serialized fields, if you're not using arrays or objects?
     
  5. Chris D

    Chris D XenForo Developer Staff Member

    If you just want to store arrays as serialized data, you don't need to do anything.

    If you want to store objects, then you need to opt in to that by marking the field as 'unsafe'.

    It's essential to avoid serializing data originating from user input. Doing so makes your code to PHP object injection vulnerabilities:
    https://www.owasp.org/index.php/PHP_Object_Injection
     
  6. Mythotical

    Mythotical Well-Known Member

    As long as I'm already using json_encode and json_decode I'm safe then?
     
  7. Jake B.

    Jake B. Well-Known Member

    Yep, json won't turn anything into an object, thus no vb-like issue ;)
     
    Mythotical likes this.
  8. Mythotical

    Mythotical Well-Known Member

    Thanks
     
  9. TheLaw

    TheLaw Well-Known Member

    Is there a way to bypass this requirement during installation? Issue is as follows - after upgrading a site, other addons have been updated. Problem is that some require an update to an intermediate version and none of the intermediate versions are compliant with this requirement.
     
    Last edited: Dec 12, 2015
  10. Mike

    Mike XenForo Developer Staff Member

    No, it can't be bypassed; the issue needs to be fixed. Assuming you're getting an error when upgrading an add-on, you may be able to resolve it by tweaking the faulty value in the add-on's XML.
     
  11. HWS

    HWS Well-Known Member

    Which is not that easy, if you don't know which values are required. However we found out that setting the default value for an array type option to
    Code:
    a:1:{s:1:"a";s:1:"b";}
    lets you /install or upgrade every add-on with such a problem successfully. You just have to set the option immediately after the upgrade and click at "save" to override the faked defaults.
     
    TheLaw and Alfa1 like this.
  12. TheLaw

    TheLaw Well-Known Member

    Suggestion - these announcements are important to have and I'm glad that they are available. That said, it might help a great deal to have important notes or references highlighted before upgrading, such as items that are expected to break some compatibility. This could be placed either in the member section and/or during the upgrade process.

    As a result of what seemed like a minor upgrade from 1.5.2 to 1.5.3, I've got a site that will stay in limbo until we can figure out how to solve this issue. A couple of the critical addons require an upgrades to intermediate releases but those releases don't comply. Site owners may not realize these issues until they try to update an addon to find that too much time has passed on their site since the last backup. I do appreciate the tip @HWS left but it only seems to work for limited circumstances. No easy solution but an ounce of prevention is worth a pound of cure. Thanks for your consideration.
     
  13. Martok

    Martok Well-Known Member

    I thought it was pretty clear from both the announcement thread and this one (which it linked to) that there could be add-on issues. As for exactly which add-ons would break, there's no way for the developers to know.

    Indeed. That is why you should have a test site and always upgrade that first. It means that you can see if there are any issues before upgrading a live site. You should also always back up a live site prior to upgrading too just in case as should there be any issues still then you can easily revert the site. If you aren't doing these things, I'd highly recommend that you do in the future.
     
    resonansER likes this.
  14. TheLaw

    TheLaw Well-Known Member

    Of course developers wouldn't know in the past what happened now. My point was that site owners (not developers) may not fully appreciate the magnitude of changes made in a "maintenance release" for a minor point upgrade. There is also no notice for the typical site owner who logs into the customer area and realizes a minor point release is available and it seems innocuous. Hence I suggest going forward that if there is a good chance that such a release is likely to break addons, it might be a good idea to make this clear to site owners in a conspicuous fashion. No blame. Since it appears others have had some issues, I was hoping that there might be a way to minimize disruption, such as @HWS was kind enough to share. Hopefully we'll figure out something and will also benefit others should they realize the issue.

    Thanks - as I said, I have a backup but the issue wasn't discovered immediately. Just a few hours on some sites means significant data loss restoring from a backup so identifying potential issues in advance is critical. And when you've got 50+ addons to test, it's a massively time consuming experience and you don't always see what may break. I always perform much more rigorous testing on major point releases and hold off on all upgrades - even minor point releases - on my most important sites so that I can see the impact on the lesser sites, if any. That's the case here.
     
  15. Martok

    Martok Well-Known Member

    Did you upgrade on a test site first? I always do this for any update, whether it's a XenForo major or minor point release, or just an update to an add-on. Really worth doing as it catches things before touching a live site.
     
  16. TheLaw

    TheLaw Well-Known Member

    Irrelevant and not a teaching moment. Only when trying to upgrade the addon did we discover the problem because the addon required one additional upgrade to an intermediate release. And I doubt most site admins upgrade every addon concurrently with a software update, especially a minor point release.

    The site works. Fortunately the developer has been helping us out and we are good. Suggestion made. Thanks for listening.
     
    Last edited: Dec 13, 2015
  17. tyteen4a03

    tyteen4a03 Well-Known Member

    Security concerns override backwards compatibility.
     
  18. HWS

    HWS Well-Known Member

    Which is applicable for the serialization issue.

    But much more add-ons suffer from the minor change with the option default settings.
     

Share This Page