muccg / rdrf

The Rare Disease Registry Framework (RDRF) is an open source tool for the creation of web-based patient registries.
GNU Affero General Public License v3.0
15 stars 8 forks source link

PROMS password breach mitigation : hash of proms responses json field #854

Closed id2359 closed 5 years ago

id2359 commented 5 years ago

Add a new model field on the SurveyAssignment object ( don't expose in the admin form view)

This can be set the md5 digest of the json. When the proms are "pulled down" , this field should also be sent to the site system and compared to a local md5 of the responses before they are processed

mouneyrac commented 5 years ago

A solution is to keep the current administration which allows editing/deleting. In production, the admin user has to be disabled from the backend (django-admin). So no user can log in the system but in case of trouble or if we want to update the proms later, we still can temporarily enable the admin user, do the required change, and disable it again.

This is the safest solution and the best one if the admin user usage is expected to be rare on the Proms system.

grahame commented 5 years ago

What are we trying to mitigate against with this md5 signature? If it's tampering on the PROMs system, it can't actually help at a security level as whoever tampers can just update the MD5 checksum.

(We shouldn't use MD5 in 2019 anyway, use sha256 instead, although that's not the issue here.)

On Mon, 1 Jul 2019 at 11:46, Jérôme Mouneyrac notifications@github.com wrote:

A solution is to keep the current administration which allows editing/deleting. In production, the admin user has to be disabled from the backend (django-admin). So no user can log in the system but in case of trouble or if we want to update the proms later, we still can temporarily enable the admin user, do the required change, and disable it again.

This is the safest solution and the best one if the admin user usage is expected to be rare on the Proms system.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/muccg/rdrf/issues/854?email_source=notifications&email_token=AACQYNK72GGZK2NDXAPXXA3P5F4XTA5CNFSM4H3OFWAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY45BSI#issuecomment-507105481, or mute the thread https://github.com/notifications/unsubscribe-auth/AACQYNN42VS5P3JYUOC3B5DP5F4XTANCNFSM4H3OFWAA .

-- Grahame Bowland Centre for Comparative Genomics Murdoch University 0433 317 142

mouneyrac commented 5 years ago

The issue is about this potential attack:

To avoid the problem, we deactivate the admin user as we don't need it.

grahame commented 5 years ago

Yes, good call – why even have the admin site running on the PROMs installation? We could just drop it from INSTALLED_APPS.

On Mon, 1 Jul 2019 at 11:56, Jérôme Mouneyrac notifications@github.com wrote:

The issue is about this potential attack:

  • a user gains access to the admin password
  • the user edits the survey data with wrong data

To avoid the problem, we deactivate the admin user as we don't need it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/muccg/rdrf/issues/854?email_source=notifications&email_token=AACQYNJKGTMV7IDIY33HD4LP5F55FA5CNFSM4H3OFWAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY45MYQ#issuecomment-507106914, or mute the thread https://github.com/notifications/unsubscribe-auth/AACQYNPBHO5W7HYXF4AHHIDP5F55FANCNFSM4H3OFWAA .

-- Grahame Bowland Centre for Comparative Genomics Murdoch University 0433 317 142

mouneyrac commented 5 years ago

Closing - the disable admin user feature is mentioned in the Google drive CIC folder under maintenance

PS: to avoid to have to redeploy when we (rarely) require the admin interface access we will keep it installed. But good thinking - I didn't think about removing the admin UI as a solution :)