All Projects FreeCAD  Addon Manager  Arch  Assembly  Draft  Expressions  FEM  File formats  GCS  OpenSCAD  Part  PartDesign  Path  Project Tools & Websites  Raytracing  Robot  Sketcher  Spreadsheet  TechDraw

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000762FreeCADBugpublic2012-06-26 09:202012-07-06 06:18
Reportershoogen Assigned Towmayer  
PriorityurgentSeveritymajorReproducibilityhave not tried
Status closedResolutionfixed 
Product Version0.12 
Fixed in Version0.13 
Summary0000762: cPickle allows to trigger arbitrary code execution from FCStd file
DescriptioncPickle is not meant to handle untrusted input and will undermine the current security model for python code. (which is that code should not be embedable in FCStd files).
Additional InformationThe Problems are described in http://nadiana.com/python-pickle-insecure
Additional malcious code could be hidden inside the zip structure and accessed using the python zipfile module.
Suggestions:
1. a safe serialisation method should be implemented.
TagsNo tags attached.
FreeCAD Information

Activities

2012-06-26 15:38

 

pickletest-win.FCStd (Attachment missing)

wmayer

2012-06-27 08:13

administrator   ~0002242

Unfortunately, json doesn't support custom python types and yaml is not part of the python libraries.

wmayer

2012-06-27 09:10

administrator   ~0002243

yaml works but again this has the method "load" which is also considered insecure. Its secure counterpart "safe_load" doesn't work either because it cannot restore custom python objects.

All what we actually wanted to do with pickling is to store the module name, the class name and create an instance when loading the project. So, I fear we have to do this on our own.

shoogen

2012-07-01 16:46

developer   ~0002254

2. There should be some backward combatibility.
2a. A whitelist could allow to recognize Draft Features.
2b. pickletools.dis() could allow an advanced user to decide wheter the input is sane.

wmayer

2012-07-02 10:07

administrator   ~0002256

> 2a. A whitelist could allow to recognize Draft Features.
A whitelist doesn't make sense because it stops all 3rd party module writers to reload their objects. And also you have to matintain this whitelist so you have to extend all the time.

> 2b. pickletools.dis() could allow an advanced user to decide wheter the input is sane.
The output of the pickletools is not very helful. And it shouldn't be up to the user to decide if he wants to run the code or not.

The only proper solution is to not run any code at all. Therefore we can write the module name and the class name directly to the XML and use json for serializing.
At project load we get the module and class name again from XML, load the module and create an instance and use json to deserialize it.

> 2. There should be some backward combatibility.
But this is not possible, then.

wmayer

2012-07-02 11:33

administrator   ~0002258

git show 7032f5f329f17aaf599cb0b8d5225c2a8bc80eec implements a way using json.

shoogen

2012-07-02 12:34

developer   ~0002259

Last edited: 2012-07-02 12:37

 Hi Werner,
at first I'd like to thank you for all time and effort you put to my request and answering my questions. I do this rarely on the tracker because i don't wan't to reopen tickes just by saying 'thank you'.

just skip the next part:
Maybe my last proposal was a bit unclear. I don't want to preeserve the functionality of the Pickel module. But I think that chaging the file format is a big deal.
Most people who created custom python feature will probably be able two work around this by themself. But I imagine, that there might be users, who rely on opening old Documents with Draft Features. I don't if there are any. And therefore it might be a waste of time to include a backward compatibility function in FreeCAD. On the other hand we shouldn't scare off users.
Maybe we can offer to convert files manualy or offer to create a conversion utility was soon as anyone requests it.
Assuming that all information (beside the Type Attribute) of Draft objects is stored in FreeCAD Properties, a regular expression to find out the Class name and 'Type' form the 'pickle' string can be quite easy.

I've thougth about moving the 0.12 (pickle) to 0.13 (json) converter to some python code that would be run (manualy) after the document is loaded. But the problem then is that the pickle strings (base64) are not available (without parsing the Document.xml again.

IMPORTANT PART:
As i understand the current implentation there is no clause in line 280 for the case that is either 'module' or 'class' attribute are not present. And therefore the would not be any error message for files created with earlier versions of FreeCAD???
Maybe the you can output the name of the Feature and the base64 encodes string in the error message. As this will help to judge how much effort needs to be takes to restore the Object.

shoogen

2012-07-03 07:05

developer   ~0002261

Last edited: 2012-07-03 07:12

 I haven't compiled it but it could look like this:
diff --git a/src/App/PropertyPythonObject.cpp b/src/App/PropertyPythonObject.cpp
index b461075..4a07c6d 100644
--- a/src/App/PropertyPythonObject.cpp
+++ b/src/App/PropertyPythonObject.cpp
@@ -278,6 +278,9 @@ void PropertyPythonObject::Restore(Base::XMLReader &reader)
                 Py::Module mod(PyImport_ImportModule(reader.getAttribute("module")),true);
                 this->object = PyInstance_NewRaw(mod.getAttr(reader.getAttribute("class")).ptr(), 0);
             }
+ else {
+ Base::Console().Warning("PropertyPythonObject::Restore: unsupported serialisation: %s\n", buffer);
+ }
         }
         catch (Py::Exception&) {
             Base::PyException e; // extract the Python error text

although the Feature name is not yet included in the message.
Maybe we should move this discussion to a seperate ticked or the forum as this i not a security issue)

wmayer

2012-07-03 07:28

administrator   ~0002263

> Assuming that all information (beside the Type Attribute) of Draft objects is stored in FreeCAD Properties, a regular expression to find out the Class name and 'Type' form the 'pickle' string can be quite easy.
Yes, this is probably the easist solution. It might be not 100% perfect but should at least create a valid proxy object.

> although the Feature name is not yet included in the message.
A proxy object is not only used in a DocumentObject but also in a ViewProvider or can be used in any other object derived from PropertyContainer.

wmayer

2012-07-03 10:34

administrator   ~0002266

git show f8c299c implements a lightweight method to restore the object and to load strings from serialized cPickle output

wmayer

2012-07-06 06:18

administrator   ~0002280

I think this can be considered as solved.

Issue History

Date Modified Username Field Change
2012-06-26 09:20 shoogen New Issue
2012-06-26 15:38 shoogen File Added: pickletest-win.FCStd
2012-06-27 08:13 wmayer Note Added: 0002242
2012-06-27 09:10 wmayer Note Added: 0002243
2012-06-28 11:46 wmayer Assigned To => wmayer
2012-06-28 11:46 wmayer Priority normal => urgent
2012-06-28 11:46 wmayer Status new => confirmed
2012-07-01 16:46 shoogen Note Added: 0002254
2012-07-02 10:07 wmayer Note Added: 0002256
2012-07-02 11:33 wmayer Note Added: 0002258
2012-07-02 11:34 wmayer Resolution open => fixed
2012-07-02 12:34 shoogen Note Added: 0002259
2012-07-02 12:37 shoogen Note Edited: 0002259
2012-07-03 07:05 shoogen Note Added: 0002261
2012-07-03 07:06 shoogen Note Edited: 0002261
2012-07-03 07:10 shoogen Note Edited: 0002261
2012-07-03 07:12 shoogen Note Edited: 0002261
2012-07-03 07:28 wmayer Note Added: 0002263
2012-07-03 10:34 wmayer Note Added: 0002266
2012-07-06 06:18 wmayer Note Added: 0002280
2012-07-06 06:18 wmayer Status confirmed => closed
2012-07-06 06:18 wmayer Fixed in Version => 0.13