Roundup Tracker - Issues

Issue 2551033

classification
Title: REST etag security
Type: security Severity: normal
Components: Web interface, Infrastructure Versions: devel
process
Status: fixed Resolution: fixed
Dependencies: Superseder:
Assigned To: rouilj Nosy List: joseph_myers, rouilj
Priority: normal Keywords: rest

Created on 2019-03-19 00:22 by joseph_myers, last changed 2019-05-31 23:48 by rouilj.

Messages
msg6402 Author: [hidden] (joseph_myers) Date: 2019-03-19 00:22
The REST code generates an etag as an md5 hash of a representation of
item properties.

That includes properties to which the user does not have access. 
Depending on the schema, that means it could be used as an oracle to
test guesses for values of such properties by generating hashes with
guessed values for those properties inserted and seeing if those match
the provided etag.

This could be addressed by using HMAC with a per-instance random secret
key, instead of a simple hash function, or by storing a random etag in
the database for each item (generated automatically like the 'activity'
property) so it's not related to item properties at all, just changes to
a new random value whenever any other change is made.

See mailing list discussion:
https://sourceforge.net/p/roundup/mailman/message/36615272/
msg6478 Author: [hidden] (rouilj) Date: 2019-05-23 22:59
Hi Joseph:

I just pushed a fix for this. Can you review: 5726:e199d0ae4a25
and see if it looks sufficient.

If so please close this issue.

Thanks.

-- rouilj
msg6479 Author: [hidden] (joseph_myers) Date: 2019-05-23 23:06
I don't see anything in that commit to actually use the key in the hmac 
calculation (it's not passed to hmac.new), nor to update those tests that 
assert a particular value of the etag.
msg6480 Author: [hidden] (rouilj) Date: 2019-05-23 23:23
Hi Joseph:

In message <alpine.DEB.2.21.1905232304440.30861@digraph.polyomino.org.uk>,
Joseph Myers writes:
>I don't see anything in that commit to actually use the key in the hmac 
>calculation (it's not passed to hmac.new), nor to update those tests that 
>assert a particular value of the etag.

Gah.... Something felt wrong/too easy when I was testing. That's why I
pinged you. Added use of key and fixing tests now.

Thanks.
msg6481 Author: [hidden] (rouilj) Date: 2019-05-24 01:02
Hi Joseph:

Ok, lets try rev5727:8b5171f353eb

Also I finally got the etag test finished.

Thoughts...

-- rouilj
msg6482 Author: [hidden] (joseph_myers) Date: 2019-05-24 20:25
I think that looks OK, but the test should have everything that uses the 
patched date.Date inside a

date.Date = dummyDate()
try:
    ... rest of test contents ...
finally:
    date.Date = originalDate

to ensure that the patching is undone if an exception occurs during the 
test code.  (Once we can assume Python 3, unittest.mock.patch can be used 
for this.)
msg6483 Author: [hidden] (rouilj) Date: 2019-05-24 21:24
Hi Joseph:

In message <alpine.DEB.2.21.1905242023010.371@digraph.polyomino.org.uk>,
Joseph Myers writes:
>I think that looks OK, but the test should have everything that uses the 
>patched date.Date inside a
>
>date.Date = dummyDate()
>try:
>    ... rest of test contents ...
>finally:
>    date.Date = originalDate
>
>to ensure that the patching is undone if an exception occurs during the 
>test code.  (Once we can assume Python 3, unittest.mock.patch can be used 
>for this.)

Just made the change. That was a try:/finally: with no except right? I
assume any exception that is raised will trigger the finally code and
then be bubbled up the stack to be handled by an except handler at a
higher level.

Just ran that specific test under py2/py3 to make sure
I got the syntax right. See checkin: 5728:bfd28644fe43

If that change works for you, feel free to just close the ticket.
msg6487 Author: [hidden] (rouilj) Date: 2019-05-31 00:05
Joseph should I close this issue?

-- rouilj
msg6488 Author: [hidden] (joseph_myers) Date: 2019-05-31 01:30
Yes.
History
Date User Action Args
2019-05-31 23:48:57rouiljsetkeywords: + rest
status: open -> fixed
resolution: fixed
2019-05-31 01:30:45joseph_myerssetmessages: + msg6488
2019-05-31 00:05:13rouiljsetmessages: + msg6487
2019-05-24 21:24:49rouiljsetmessages: + msg6483
2019-05-24 20:25:26joseph_myerssetmessages: + msg6482
2019-05-24 01:02:56rouiljsetmessages: + msg6481
2019-05-23 23:23:26rouiljsetmessages: + msg6480
2019-05-23 23:06:02joseph_myerssetmessages: + msg6479
2019-05-23 22:59:21rouiljsetnosy: + rouilj
messages: + msg6478
2019-05-23 22:41:35rouiljsetstatus: new -> open
assignee: rouilj
components: + Web interface
2019-03-19 00:22:45joseph_myerscreate