remove trusted=True from skops.io.load(s)
#422
Conversation
IMO this is a bit silly, with the same logic the whole pickle module would need to be removed. But I guess since there is a simple (albeit not really safer) solution, I'd be okay with the change. I'm wondering, however, if this should be deprecated?
We could add a check that
Nice! |
We used to have the hint that the API is really subject to change though. That's why I didn't deprecate. However, this has been used now for a while and pretty stable. So I removed the comment here and from now we can be even more conservative in changes. |
There was a problem hiding this comment.
The changes LGTM (though I still don't think they're really necessary).
I think mypy should pretty much make sure that the change was propagated completely through the code base. I still grepped through the code and found some instances that may need updates (see comments).
Apart from that, how about adding a paragraph to the docs that explicitly mentions that the trusted=True option was removed (and why using it was a bad idea anyway). Since this is a breaking change, making sure that users can quickly find a reference to this would be beneficial.
| @@ -14,6 +14,8 @@ v0.10 | |||
| - Removes Pythn 3.8 support and adds Python 3.12 Support :pr:`418` by :user:`Thomas Lazarus <lazarust>`. | |||
| - Removes a shortcut to add `sklearn-intelex` as a not dependency. | |||
| :pr:`420` by :user:`Thomas Lazarus < lazarust > `. | |||
| - ``trusted=True`` is now removed from ``skops.io.load`` and ``skops.io.loads``. | |||
| :pr:`422` by `Adrin Jalali`_. | |||
There was a problem hiding this comment.
Maybe add an explanation.
| @@ -14,6 +14,8 @@ v0.10 | |||
| - Removes Pythn 3.8 support and adds Python 3.12 Support :pr:`418` by :user:`Thomas Lazarus <lazarust>`. | |||
| - Removes a shortcut to add `sklearn-intelex` as a not dependency. | |||
| :pr:`420` by :user:`Thomas Lazarus < lazarust > `. | |||
| - ``trusted=True`` is now removed from ``skops.io.load`` and ``skops.io.loads``. | |||
| :pr:`422` by `Adrin Jalali`_. | |||
|
@BenjaminBossan you okay with this now? |
There was a problem hiding this comment.
Thanks for making the adjustments, this LGTM.
I have a nit about a type check, please take a look and decide if you want to fix it (you can merge no matter what).
While reviewing, I also stumbled upon the fact that from typing import Sequence is apparently deprecated:
https://docs.python.org/3/library/typing.html#typing.Sequence
Not sure if the tests should have caught that. Anyway, this can be addressed in a separate PR.
Cool, I'll open a separate PR for that. |
|
Could a release with this change be put out so that we can silence our CVE warning? 😅 |
|
@judahrand yes, working on a numpy2 compatibility PR and will release with that one. |
|
Release is out. |
We got a CVE report where an attacker can execute arbitrary code with
trusted=True. While one can argue this is a feature and not a bug, it does pose the issue that people might be overly optimistic about the security of the format.Therefore removing it might be a better idea. This doesn't change any functionality since users can still trust the file by blindly passing
trusted=get_untrusted_types(...)cc @BenjaminBossan
I also have to say, having typehints made the process a LOT easier here.
CVE:https://www.recordedfuture.com/vulnerability-database/CVE-2024-37065