Library Settings Editor History Feature Exposes Sensitive Data to Unauthorized Staff

Bug #1450519 reported by Jason Stephenson
306
This bug affects 11 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
High
Unassigned
3.6
Fix Released
High
Unassigned

Bug Description

This bug is a followup to https://bugs.launchpad.net/evergreen/+bug/1206589. The reason for opening this bug is that the fix to the previous bug is incomplete. That fix prevented unauthenticated, third party access to the library settings history, but it did not address the underlying issue that made that access possible. The history feature still has the bugs of not following the permission model used by the org. unit settings and of exposing data for locations outside of the current user's organizational unit ancestor hierarchy. At this point, however, the data is only exposed to accounts with the STAFF_LOGIN permission.

To address these problems, the following changes need to be made:

* The open-ils.pcrud controller, along with the associated pcrud permissions, should be removed from the coustl (config.org_unit_setting_type_log) object definition in the IDL.

* OpenSRF calls need to be added to open-ils.actor to retrieve settings history. These calls will need to be given the user's authtoken as well as the setting name and possibly the desired organizational unit to look up. The latter can be derived from the user's authtoken and current working location, and so may be deemed optional or unnecessary. These calls should only expose the settings to the user that are in that user's org. unit hierarchy as defined by the appropriate database or other functions.

* The library settings editor interface in the staff client then requires modification to use the new OpenSRF calls and to stop using the openils.PermaCRUD JavaScript module.

Changed in evergreen:
milestone: none → 2.next
Kathy Lussier (klussier)
Changed in evergreen:
status: New → Confirmed
no longer affects: evergreen/2.6
Changed in evergreen:
milestone: 2.next → none
no longer affects: evergreen/2.7
no longer affects: evergreen/2.8
Changed in evergreen:
status: Confirmed → Won't Fix
Revision history for this message
Andrea Neiman (aneiman) wrote :

I am removing the won't fix because I think this is still an issue -- as of 3.2 I am still able to see Settings History for OUs outside of my assigned OU hierarchy.

Changed in evergreen:
status: Won't Fix → Confirmed
tags: added: admin-pages orgunitsettings webstaffclient
removed: admin staffclient
Revision history for this message
Rogan Hamby (rogan-hamby) wrote :

This is and can expose things like PayPal settings at other org units, which we definitely don't want.

Revision history for this message
Kyle Huckins (khuckins) wrote :

Noting that I'm looking at a solution to this within bug #1839341, specifically in regards to the new API method.

Revision history for this message
Ruth Frasur Davis (redavis) wrote :

Has this ticket been resolved in the above-mentioned ticket?

Revision history for this message
Jane Sandberg (sandbergja) wrote :

Noting that #1839341 does resolve the second bullet point in Jason's original bug report. The first one still remains to be fixed (removing the pcrud controller from the coustl class in the IDL).

If we'd like to backport the fix -- and I think we should -- we would need to modify the dojo interface to use Kyle's new API. Additionally, when the new Angular UI is merged, we should either include the updated dojo that uses the new UI, or remove the dojo history interface completely. It wouldn't do much good to keep the old (insecure) UI around.

tags: removed: webstaffclient
Revision history for this message
Galen Charlton (gmc) wrote (last edit ):

A patch is available at

working/user/gmcharlt/lp1450519_restrict_access_to_ou_history / https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/gmcharlt/lp1450519_restrict_access_to_ou_history

Note that the API it introduces is a partial counter-proposal to the one Kyle proposes in bug 1839341; because of the way that settings can inherit values from OU ancestors, being able to see the history at more than just the current context OU can be helpful.

Changed in evergreen:
milestone: none → 3.7.2
tags: added: pullrequest
Revision history for this message
Shula Link (slink-g) wrote :

Tested this on PattyPan, and currently it appears you can still see historical setting from outside the current OU hierarchy, unless I have misunderstood the bug completely. Screenshot attached.

Revision history for this message
Jason Boyer (jboyer) wrote :

Hi Shula, the patch only affects those settings that have a view permission attached, which aren't many; primarily credit card related settings. You should be able to set one of these permissions at BR1 and then a BR2 circ staff user should not be able to see them:
Enable AuthorizeNet payments
AuthorizeNet login
AuthorizeNet password
AuthorizeNet server
AuthorizeNet test mode
Name default credit processor
Enable PayflowPro payments
PayflowPro login/merchant ID
PayflowPro partner
PayflowPro password
PayflowPro test mode
PayflowPro vendor
Enable PayPal payments
PayPal login
PayPal password
PayPal signature
PayPal test mode
Enable Stripe payments
Stripe publishable key
Stripe secret key

Though I'd ask that no one change the default credit card processor or Stripe settings since that's also being tested there. :) Just changing something like the authorize.net password and seeing if it's visible from a different location is probably enough to verify if things are working.

Revision history for this message
Shula Link (slink-g) wrote :

Got it! Verified that it's working on the authorize.net password.

Revision history for this message
Shula Link (slink-g) wrote (last edit ):

I have tested this code and consent to signing off on it with my name,
Shula Link and my email address, <email address hidden>

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=e0f7568e70e8593c081790eb40094ee3047ba792

tags: added: signedoff
Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Works for me, too!

Pushed to master, 3.7, and 3.6.

Thanks, Galen, Shula, and everyone else!

Changed in evergreen:
status: Confirmed → Fix Committed
assignee: Jason Stephenson (jstephenson) → nobody
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.