Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12273 closed (worksforme)

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

Reported by: robhudson 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: UI/UX:

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.

Attachments (0)

Change History (5)

comment:1 Changed 4 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by kenseehart

  • Owner changed from nobody to kenseehart
  • Status changed from new to assigned

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

comment:3 Changed 4 years ago by kenseehart

  • Needs documentation set
  • Needs tests set
  • Resolution set to worksforme
  • Status changed from assigned to closed

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 Changed 4 years ago by kenseehart

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

comment:5 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.