PHPCS using the Drupal coder standards, throws the following warning in "/src/Entity/Micon.php", line 192:
unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.
return unserialize(gzuncompress(stripslashes(base64_decode(strtr($data, '-_,', '+/=')))));
I don't have enough experience, but this could definitely be a potential security risk!
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2022-07-15 17_34_27-Window.png | 99.92 KB | grevil |
| #12 | micon_potential-security-risk-3271162-12.patch | 1.55 MB | jacobbell84 |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !12changes, plain diff MR !11changes, plain diff MR !8
Comments
Comment #2
jacobbell84 commentedHmm. I'm not sure why there is serialization there to begin with, it's dealing entirely with strings as far as I can tell. @JaceRider, do you recall if there was a reason for that? Thinking we just remove the unserialize/deserialize calls and write an update hook that re-saves existing archives without the serialization. I don't think all the string manipulation is needed either. Maybe it was when the module was first written, but I've saved base64 encoded strings in the configuration before and it automatically prefixes it with !!binary so that it will work.
Comment #3
jacerider commentedSorry I haven't been able to put really any time toward this project. Thanks @jacobbell84 for maintaining things.
The reason it is serialized is that it is an array of IcoMoon data. A better approach would have been JSON. The problem is that to change it now would break existing icon packages unless we wrote a migration/update that would convert the existing data to JSON.
Comment #4
anybodyPerhaps the risk could be mitigated by using allowed_classes parameter and ensuring this can never add any classes / executable code indeed?
Could someone post a short example of the non encoded contents? Is it an array or something?
Json and update hook would be the most safe way, in the end the risk is someone injecting executable / executed code.
Comment #5
jacobbell84 commentedNo worries, that's why having multiple maintainers is nice so we can cover each other as folks get busy :)
That helps though, let me do some testing and I'll see if I can set it up so that it doesn't use the serialize/unserialize methods. Thanks!
Comment #6
jacobbell84 commentedAnd also add an update hook so that we don't break everyone's installs :)
Comment #7
jacobbell84 commentedHi all, give this patch a try. In my testing, the encoding/gzcompression/serialization wasn't needed to store the zip. Just writing the zip into the config stored it properly with the binary prefix. Maybe it's a case where it was needed when the module was released, but not anymore. I did my testing on Drupal 9.3, so maybe someone that has Drupal 8 takes a pass at it? I also added an update hook that should convert any existing configs over to the new format.
Comment #9
jacobbell84 commentedAdding default install configuration to the patch
Comment #11
jacobbell84 commentedTrying to fix tests
Comment #12
jacobbell84 commentedFixing schema
Comment #13
jacobbell84 commentedComment #14
anybodyThank you very much, tje patch looks quite good to me, but perhaps @JaveRider could also have a look?
In this context it would perhaps make sense to opt into Drupal’s security advisory policy after fixing this issue?
Comment #15
jacobbell84 commentedHi @Anybody, that's the plan, I recently got my vetted security role for these projects :)
Comment #16
anybodyVery cool and congratulations, @jacobbell84! :) Looking forward to the next release.
Comment #17
grevil commentedDrupal did not automatically create the issue branch, only the issue fork for this issue, trying to manually create the issue branch, I accidentally based the branch on 8.x-1.x instead of 2.x...
Closing the original merge request and pushing the patch to the issue fork's 2.x branch instead.
Comment #20
grevil commentedComment #21
grevil commentedI am not able to import a icoMoon icon package.
The following error occurs:
Steps to reproduce
I also tried moving the zip file both into my private and public-files directory. Same error
Comment #22
grevil commentedThe icon package gets correctly unzipped and moved to "/web/sites/default/files/micon/test".
The problem is, that the "selection.json" is completely empty.
Comment #23
grevil commentedComment #24
grevil commentedComment #28
grevil commentedOK, the patch in #12 actually works great! Although, we need to rewrite the "Adding IcoMoon packages" Instructions on the main module page, since icoMoon moved their package builder to "https://icomoon.io/app" and the packages in "https://icomoon.io/#icons" can not be imported, since they have their selection.json in a different file folder then the self built packages from https://icomoon.io/app.
PS: Sorry for the merging mess again...
Comment #29
grevil commentedI will test the new update hook tomorrow, do a final code review, and then we can safely merge this!
Comment #30
grevil commentedAlright, the update hook also works like a charm! I added "'allowed_classes' => FALSE" to the update hook 'unserialize' function, since we only unserialize strings and never objects.
This way we prevent the security issue there as well, even if it is only used once per update.
I will update dev, create a new release and renew the module description on how to require the micon packages!
Comment #31
grevil commentedI will create a seperate release to 2.0.0-beta3, since I have not tested it on D8 (EOL). Just to be sure
Comment #33
grevil commentedThanks all! Changes are merged :)
Comment #34
anybodyThank you all for fixing this!! :)
You mean 2.1.0-beta0. Was just released.