BUG: Make allow_pickle=False the default for loading

[ivanov]

a partial mitigation of #12759.

see also https://nvd.nist.gov/vuln/detail/CVE-2019-6446

I'm not sure if this commit should start with 'API:' - and it should
certainly be documented, but I'm not sure if this will go into 1.16 or
1.17.

[ivanov]

The vulnerability only applies to loading, but I thought it'd be awkward if the default parameters did not round-trip save-load with default parameters.

[stefanv]

This looks like the correct default value for that parameter. @seberg suggested we let the user know how to fix the situation, should their file contain a pickle.

[seberg]

@ivanov thanks, indeed, unless someone seriously argues this is critical, could you go with allow_pickle=None which then gives a FutureWarning instead? IIRC, FutureWarnings are always visible, so it will at least educate over the dangers.

With such a warning I think we should probably backport it as well, since I assume downstream packages will at worst log warnings in their test suits, it should be OK. (I am not sure about the typical setup, numpy would ignore such warnings in tests for release versions)

We could probably even aim for changing it by 1.18, with the option of delaying if any downstream packages get hit by it. The current error message is very correct, but maybe could be clarified a bit more for very unexperienced users.

[charris]

This should get a mention in the 1.17.0-notes as changed behavior.

[eric-wieser]

+1 on seberg's suggestion. This doesn't strike me as a real enough problem to break everyone saving object arrays without a deprecation period

[oleksandr-pavlyk]

@seberg @eric-wieser Just to clarify, do you mean adding allow_pickle=None as default for numpy.load, numpy.save, numy.savez and numpy.NpzFile?

[oleksandr-pavlyk]

By the way, changing default of allow_pickle of _savez renders it impossible to save object arrays with numpy.savez and numpy.savez_compressed. Those functions simply do not provide for the way to control values of allow_pickle and pickle_kwargs of the underlying _savez.

[ivanov]

@oleksandr-pavlyk is right , I added the passing of that arg to the savez functions, but neglected to push that out to this PR. Will integrate allow_pickle=None as a transition path with a warning next

[oleksandr-pavlyk]

@ivanov I also needed to change test_io.py as follows to allow tests to pass:

diff --git a/numpy/lib/tests/test_io.py b/numpy/lib/tests/test_io.py
index 7ef25538b..5c9ece928 100644
--- a/numpy/lib/tests/test_io.py
+++ b/numpy/lib/tests/test_io.py
@@ -179,7 +179,14 @@ class RoundtripTest(object):

 class TestSaveLoad(RoundtripTest):
     def roundtrip(self, *args, **kwargs):
-        RoundtripTest.roundtrip(self, np.save, *args, **kwargs)
+        load_kwds = kwargs.get('load_kwds', {})
+        load_kwds['allow_pickle'] = load_kwds.get('allow_pickle', True)
+        save_kwds = kwargs.get('save_kwds', {})
+        save_kwds['allow_pickle'] = save_kwds.get('allow_pickle', True)
+        kwargs_copy = kwargs
+        kwargs_copy['load_kwds'] = load_kwds
+        kwargs_copy['save_kwds'] = save_kwds
+        RoundtripTest.roundtrip(self, np.save, *args, **kwargs_copy)
         assert_equal(self.arr[0], self.arr_reloaded)
         assert_equal(self.arr[0].dtype, self.arr_reloaded.dtype)
         assert_equal(self.arr[0].flags.fnc, self.arr_reloaded.flags.fnc)
@@ -187,7 +194,11 @@ class TestSaveLoad(RoundtripTest):

 class TestSavezLoad(RoundtripTest):
     def roundtrip(self, *args, **kwargs):
-        RoundtripTest.roundtrip(self, np.savez, *args, **kwargs)
+        load_kwds = kwargs.get('load_kwds', {})
+        load_kwds['allow_pickle'] = load_kwds.get('allow_pickle', True)
+        kwargs_copy = kwargs
+        kwargs_copy['load_kwds'] = load_kwds
+        RoundtripTest.roundtrip(self, np.savez, *args, **kwargs_copy)
         try:
             for n, arr in enumerate(self.arr):
                 reloaded = self.arr_reloaded['arr_%d' % n]

The reason TestSavezLoad are only setting load_kwds, but not save_kwds was because I did not go as far locally, and did not modify the defaults to _savez function in npyio.py.

[ivanov]

I've been hoping to figure this out on my own, but now I've spent so much time trying to do that that it makes sense to just ask instead of wading through the git log to infer the process.

Should I make a doc/release/1.16.2-notes.rst to document this change, or does this go into doc/changelog/1.17.0-changelog.rst and then that will get included in a backport?

Here's what the FutureWarning looks like at the moment:

$ python -c 'import numpy as np; np.savez("hi.npz", [{"yo": "sup"}])'

/home/pi/code/numpy/numpy/lib/npyio.py:746: FutureWarning: Object arrays will not be saved by default in the future because allow_pickle will default to False. You should add allow_pickle=True explicitly to eliminate this warning.

[seberg]

@ivanov just add the 1.17.0 release notes for now, I think. I believe we tended to be on the side of: there is no reason to disallow saving, I very much agree with Erics argument that it is hell if a long running script dies at the end because of such a silly thing. I would be OK with a general warning, but I am not sure it is actually useful.

[oleksandr-pavlyk]

@ivanov This code does not compile with Python 2.7 as def _savez_dispatcher(file, *args, allow_pickle=None, **kwds): is invalid syntax, so porting this change back to 1.16.2 would require changes.

@seberg The scipy 1.2.1 contains a test, scipy.sparse.tests.test_matrix_io::test_malicious_load, that expects save to not execute __reduce__.

[seberg]

@oleksandr-pavlyk thanks for the pointer, but it seems to me that it does not matter here? Since that already sers allow_pickle=False explicitly on new enough numpy versions, so it already is clean with respect to any change we are doing. Also good point about the backporting!

@ivanov I think the messages could probably be simplified a bit (but have to think about it myself first). We will definitely need tests for the deprecation. If in doubt you can put them into the test_deprecations.py file but together with the normal tests is just as well.

[oleksandr-pavlyk]

@seberg Yes, indeed. I misread the test. I assumed it was expecting savez to raise the ValueError.

[seberg]

Can you remove the Deprecation for saving, although we can discuss it before if you like. We can also discuss if we should give a UserWarning on writing a pickle.
I still think we can probably make the text a bit more to the point (imagine someone coding for 2 weeks being frustrated their data does not load).

Oleksandrs comment on Backporting/Py 2.7 compatibility is very important of course (since I guess we want to do that).

[stefanv]

What is NumPy's policy on pushing to other developers' branches? In scikit-image, we often fix nitpicks on behalf of other authors, to save frustration.

[seberg]

@stefanv, I think we (at least me) just do not have much experience with it, or a policy in numpy. Probably something we should pick up, since I guess it is much less frustrating and safe an iteration or two. I guess conflicts are rare enough. You would only use it for nits really or also for very clear fixups?

[stefanv]

In scikit-image, we use it for small fixes, but also for larger fixes if the developer is less experienced. More experienced developers tend not to appreciate it if you modify their PRs too heavily :)

[charris]

@stefanv We allow editing online to fix nits, it is encouraged to get otherwise good PRs merged. Although sometimes having the user jump through the hoops is educational :) Are you suggesting pushing directly to the user's branch?

[stefanv]

On Tue, 12 Feb 2019 11:08:58 -0800, Charles Harris wrote: @stefanv We allow editing online to fix nits, it is encouraged to get otherwise good PRs merged. Although sometimes having the user jump through the hoops is educational :) Are you suggesting pushing directly to the user's branch?

Yes, I despise the online editor: it often causes difficulty for us. The same effect can be achieved by cloning the contributor's branch, making changes, and pushing them back up—all using the editing environment you are used to.

[charris]

"API" looks like a good marker, I'll add it.

[rgommers]

Should I make a doc/release/1.16.2-notes.rst to document this change, or does this go into doc/changelog/1.17.0-changelog.rst and then that will get included in a backport?

It doesn't look to me like an API change like this should be backported to 1.16.2. A bigger warning for this was already added to the docs, so for the rest the regular policy should apply: warning in 1.17.0, change of behavior in 1.19.0

[charris]

How about just disable the default load with this PR, which handles the CVE and backports easily, and make the warning/deprecation another PR.

[shoyer]

How about just disable the default load with this PR, which handles the CVE and backports easily, and make the warning/deprecation another PR.

Sounds good to me!

[stefanv]

+1

[mattip]

split into two PRs. This one is now only about loading, and changes the default for allow_pickle to False

[charris]

I think there is consensus around this now, but will wait. I don't think we need to test the default, anyone want to make an argument for that?

[rgommers]

I don't think we need to test the default,

agreed

[eric-wieser]

Looks good to me once the ..versionchanged is added that I mention above

[stefanv]

👏

[rgommers]

woohoo! looking forward to 1.16.3:)