fix(repair): restrict unserialize() in RemoveBrokenProperties#59979
Conversation
elicpeter
requested review from
Altahrim,
ArtificialOwl,
icewind1991 and
salmart-dev
and removed request for
a team
Altahrim
left a comment
Altahrim
left a comment
There was a problem hiding this comment.
Looks good, thank you!
There was a problem hiding this comment.
Pull request overview
Hardens the DAV properties repair step by preventing PHP object instantiation during unserialize() when scanning properties.propertyvalue for broken serialized payloads.
Changes:
- Updates
RemoveBrokenProperties::run()to callunserialize(..., ['allowed_classes' => false])when checking whetherpropertyvalueis broken.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
RemoveBrokenProperties::run() calls unserialize() on the property value column without restricting allowed_classes. The result is only compared against false to identify broken rows, so no class instantiation is needed. As written though, magic methods (__wakeup/__destruct) on any class referenced by the serialized payload still execute. The runtime decoder for the same column already restricts deserialization. See apps/dav/lib/DAV/CustomPropertiesBackend.php:675-678, which passes ['allowed_classes' => self::ALLOWED_SERIALIZED_CLASSES]. This change applies the same hardening to the repair step. It uses ['allowed_classes' => false] since the unserialized value is never used, only its truthiness is checked. No behavior change for valid or broken rows. Signed-off-by: Eli Peter <54954007+elicpeter@users.noreply.github.com>
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
|
/backport to stable34 |
Summary
RemoveBrokenProperties::run()callsunserialize()on thepropertyvaluecolumn without restrictingallowed_classes. The result is only compared againstfalseto identify broken rows, so no class instantiation is needed. As written though, magic methods (__wakeup/__destruct) on any class referenced by the serialized payload still execute.The runtime decoder for the same column already restricts deserialization. See
apps/dav/lib/DAV/CustomPropertiesBackend.php:675-678, which passes['allowed_classes' => self::ALLOWED_SERIALIZED_CLASSES]. This change applies the same hardening to the repair step. It uses['allowed_classes' => false]since the unserialized value is never used, only its truthiness is checked.No behavior change for valid or broken rows.
Found while testing an in-development static analysis tool I'm building against open-source PHP codebases.
TODO
Checklist
3. to review, feature component)stable32)AI (if applicable)