Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#12273 closed (worksforme)

django.contrib.formtools.utils.security_hash resulting in different pickled string on same data

Reported by: Rob Hudson Owned by: kenseehart
Component: contrib.formtools Version: 1.1
Severity: Keywords: security hash
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have a fairly complex form wizard I'm using to walk a site administrator through creating a bi-weekly newsletter.

On the "Done" step of the form wizard I was getting hash errors and started to dig a little deeper. In the security_hash method I output both the data and the pickled string and found that the data was the same but the pickled string differed by a small number of characters -- what looks to me like some quoting delimiter characters.

From Step 2 to Step 3, my print statements in security_hash shows me this (wrapped to not make reading this difficult):

[('fishing_message', u'<p>F</p>'), ('hunting_message', u'<p>H</p>'), ('special_fishing_header_1', u'F1'), ('special_fishing_message_1', u'<p>1</p>'), 
('special_fishing_books_1', u'1-57223-365-6'), ('special_fishing_header_2', u'F2'), ('special_fishing_message_2', u'<p>2</p>'), 
('special_fishing_books_2', u'1-57223-365-6'), ('special_hunting_header_1', u'H1'), ('special_hunting_message_1', u'<p>1</p>'), ('special_hunting_books_1', u'1-57223-365-6'),
('special_hunting_header_2', u'H2'), ('special_hunting_message_2', u'<p>2</p>'), ('special_hunting_books_2', u'1-57223-365-6'), '3&#xsvm^bt(*tg-18-(b+3bfr+*21=c(gg8mnoyumgtd3!rok8']

?]q(Ufishing_messageq<p>F</p>q?qUhunting_messageq<p>H</p>q?qUspecial_fishing_header_1XF1q	?q
Uspecial_fishing_message_1q
                           <p>1</p>q
1-57223-365-6q*?q+U23&#xsvm^bt(*tg-18-(b+3bfr+*21=c(gg8mnoyumgtd3!rok8q,e.age_2q&<p>2</p>q'?q(Uspecial_hunting_books_2q)X

From Step 3 to Done, this is the output of the same data and pickled string:

[('fishing_message', u'<p>F</p>'), ('hunting_message', u'<p>H</p>'), ('special_fishing_header_1', u'F1'), ('special_fishing_message_1', u'<p>1</p>'), 
('special_fishing_books_1', u'1-57223-365-6'), ('special_fishing_header_2', u'F2'), ('special_fishing_message_2', u'<p>2</p>'), 
('special_fishing_books_2', u'1-57223-365-6'), ('special_hunting_header_1', u'H1'), ('special_hunting_message_1', u'<p>1</p>'), ('special_hunting_books_1', u'1-57223-365-6'),
('special_hunting_header_2', u'H2'), ('special_hunting_message_2', u'<p>2</p>'), ('special_hunting_books_2', u'1-57223-365-6'), '3&#xsvm^bt(*tg-18-(b+3bfr+*21=c(gg8mnoyumgtd3!rok8']

?]q(Ufishing_messageq<p>F</p>q?qUhunting_messageq<p>H</p>q?qUspecial_fishing_header_1XF1q	?q
Uspecial_fishing_message_1q
                           <p>1</p>q
1-57223-365-6q'?q(U23&#xsvm^bt(*tg-18-(b+3bfr+*21=c(gg8mnoyumgtd3!rok8q)e._2q#<p>2</p>q$?q%Uspecial_hunting_books_2q&X

Trying to highlight the differences, they appear to be only on the last line of the pickled strings above, so here they are together with another line pointing at the differences:

1-57223-365-6q*?q+U23&#xsvm^bt(*tg-18-(b+3bfr+*21=c(gg8mnoyumgtd3!rok8q,e.age_2q&<p>2</p>q'?q(Uspecial_hunting_books_2q)X
1-57223-365-6q'?q(U23&#xsvm^bt(*tg-18-(b+3bfr+*21=c(gg8mnoyumgtd3!rok8q)e.   _2q#<p>2</p>q$?q%Uspecial_hunting_books_2q&X
              ^  ^                                                     ^  ___   ^         ^  ^                         ^

In order to move past this I overrode the security_hash method of my FormWizard to simply use repr(data) rather than call the formtools version. I don't know if this is any more or less secure but since this is all happening in the admin I'm comfortable with less security rather than false positives.

Since it seems that this data is never un-pickled and the pickling is only there to generate a string that can be hashed, would it make sense to use repr() instead of pickle? There are a few other bugs in the tracker related to the security_hash and pickled data this might solve.

Change History (5)

comment:1 by Russell Keith-Magee, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:2 by kenseehart, 14 years ago

Owner: changed from nobody to kenseehart
Status: newassigned

Implementing suggested change: "use repr() instead of pickle". If anyone knows a reason not to do this, please say so ASAP.

comment:3 by kenseehart, 14 years ago

Needs documentation: set
Needs tests: set
Resolution: worksforme
Status: assignedclosed

There is not enough information to reproduce the error. Ideally, we need enough to create a test case that fails.

Comments like "... From Step 2 to Step 3" are meaningless since there is no description of a sequence of steps.

In the case described, it appears that only basic python types are used (lists, tuples, strings), but if that were the case, then I can't see how it would be possible for pickle to give two different results when repr gives the same result. So it seems that some of the objects have repr that looks like a string repr.

I am not sure that replacing pickle with repr is wise. Using repr on such objects is dangerous because some objects could contain content that would cause problems for repr, whereas they would not cause problems for pickle. I don't see this as a security issue; just a possible cause of exceptions.

Some kinds of objects, such as dictionaries, can differ in both repr and pickle for equal values, so repr is not necessarily an improvement over pickle in the general case.

Therefore a description of how to cause this bug starting from a new Django installation is necessary.

This bug can be reopened if a sequence of steps can be provided that allows us to reproduce the problem on a fresh Django install.

Note: the current test suite does not cover utils.security_hash at all.

comment:4 by kenseehart, 14 years ago

... so I'm not implementing suggested change after all...

comment:5 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top