How much is set in stone?

[Paul Rubin]

Erno Kuusela <erno-news at erno.iki.fi> writes:
> | In fact it does the opposite--both the documentation and the pickle
> | implementation (look at the "security" check for pickled strings)
> | appear written with the idea that unickling is intended to be safe for
> | untrusted strings.
> 
> i can't see that idea in the documentation even if i try.

The pickle docs mention the non-pickle-ability of code objects as a
security advantage of pickle over marshal.  Clearly the doc writer
wasn't aware that unpickling is insecure for other reasons.  Also, the
docs for marshal recommend using pickle instead of marshal for RPC.
Any pickle-based RPC server exposed to the internet (see for example
pyro.sourceforge.net) would of course have a security hole.

> i don't know what the "security" check in the source code is about,
> perhaps the pickle author was planning to make it safe for untrusted
> data at some point.

More likely the original version of pickle was intended to be safe
for untrusted data, and the __getinitargs__ scheme was added later
without considering the security implications.

> the fact that pickle shouldn't be fed untrusted data has been common
> knowledge in the python user and developer communities as long as i
> can remember.

It sure shocked the heck out of me!  I discovered it as a result of
making a sourceforge bug report (#467384) requesting that marshal be
documented so it could be used for RPC, and Tim suggested I use pickle
instead.  So even Tim (one of the main authorities on Python's
implementateion) wasn't aware of the problem at the time.  The authors
of the Cookie and Pyro modules weren't aware of it either.  So this
type of "common knowledge" needs, at the very least, to be clearly
documented!  It's not reasonable to expect programmers of a supposedly
easy to learn language to absorb all kinds of unwritten folklore
before they can safely write as common an application as a simple cgi
script.

> iirc the cookie module had this erroneous code before it was accepted
> into the library.
> 
> when it was put in the library, warnings were put in its documentation
> to warn against anyone ever using the functionality.

However, "Cookie" is still aliased to SmartCookie in the module,
supposedly for backward compatibility.  That is, to leave a security
hole in any existing servers using the module.  SmartCookie and
SerialCookie should be disabled altogether.  If the holes were known
before the module was accepted to the library, that's even worse.  It
should never have been accepted with holes like that.