Credit Card Processor settings visible in LSE History

[Jason Boyer]

Eg 2.4.0
OSRF 2.2

The Library Settings Editor will hide CC processor settings from users who lack the VIEW_CREDIT_CARD_PROCESSING permission, but the history function still shows past settings.

reproduction:

User with ADMIN_CREDIT_CARD_PROCESSING permission sets AuthorizeNet Login setting at CONS to XYZ
User without VIEW_CREDIT_CARD_PROCESSING permission opens Lib setting editor, sees no indication that AuthorizeNet Login is set
User clicks History, sees the date, context, previous and new settings.

I would expect that the setting history function consult the permission's view_perm, just like the editor does.

[Jason Stephenson (jstephenson)]

While the initial bug report indicates this a problem in the Library
Settings Editor's history functions only, the underlying cause turns
out to be yet another remote vulnerability. Any user who can
authenticate to Evergreen and make the proper open-ils.pcrud calls can
view the history of any setting, including those that are sensitive.

This happens because the permacrud action entries in the IDL for the
coustl object lists no required permission for retrieve. Thus, no
permission is required and once anonymous pcrud goes in, no login
would be required either.

An immediate fix for this would be to add a permission, just about any
permission that a patron would not have will do, to the retrieve
action in couustl's permacrud block.

A longer winded fix will appear in a forthcoming comment. There are a
number of things "wrong" about this feature.

[Jason Stephenson (jstephenson)]

 The collab/dyrcona/lp1206589-quick-fix branch in the security repo adds a retrieve permission of STAFF_LOGIN to the coustl IDL entry, and it fixes a bug with the primary key entry that I noticed while looking into this.

After adding the STAFF_LOGIN permission and testing a proof of concept with my wife's account, I got this error message which seemed to leak information to the user: open-ils.pcrud: no object found with primary key date_applied of 2015-02-19T09:02:12-0500. Looking into that showed that the primary field in the IDL needed to change to id. Doing that resolved the strange error.

I chose STAFF_LOGIN even though it does not resolve Jason's initial problem. STAFF_LOGIN at a minimum requires a user to be able to login as staff in order to exploit this. That leaves us pretty much where the initial bug reports assumes we were with settings exposed only to unauthorized staff. If someone has a better idea for a permission to use, feel free to change it.

This also needs a good write up for the security releases this week. With people urged to upgrade. This one may be harder to patch accurately as sites are known to customize their IDL.

My next comment will contain my thoughts on a longer term fix for the LSE history feature's issues.

[Jason Stephenson (jstephenson)]

The database table and IDL object are config.org_unit_setting_type_log
and coustl. These should probably be actor.org_unit_setting_log and
aousl, respectively. It may be too late to change this without
inconveniencing someone, but I suggest changing the table name and the
IDL.

Because the table and IDL seem to be based on coust, the IDL uses
permissions based on ADMIN_ORG_UNIT_SETTING_TYPE, and a new permission
for managing the log: ADMIN_ORG_UNIT_SETTING_TYPE_LOG. Given that
we're actually tracking changes to the settings themselves, and not to
the setting types, these permissions should be changed, or even
dropped. Log entries should be created and managed in the backend
without regard to the user's permissions. Log entries should only be
viewable to users who have the view permission for the referenced
setting at the referenced organizational unit. (More on how the
history is exposed appears later.)

The IDL lists open-ils.pcrud as a controller. This listing is
problematic since org. unit settings do not follow the pcrud model for
permissions. The open-ils.pcrud controller should be dropped. The
parent aous permissions should be used by log entry API calls. Given
that open-ils.cstore would be the remaining controller, the backend
can still manage the log without needing permissions.

The log records the date of the change, the org unit where the setting
applies, the original value, the new value, and the setting (field
name) that was changed. It does not track what user made the change,
nor at what workstation. I suggest adding the user and workstation as
new columns if possible.

The database trigger, limit_log_oust, limits the settings log to the
five most recent changes for any setting determined solely by the
field name. The limit of 5 is "hard coded" by using a combination of
dates and a limit of 4 on a subselect in a larger delete statement.
This limit should, itself, have a setting, either an internal or
global flag, or an org. unit setting itself. Furthermore, the limit
code should be expanded to look at both the org_unit and the
field_name, or if the table/IDL changes suggested above are adopted,
the parent aous. While this will expand the logs a bit, it will give
each site its own history. If an org. unit setting is the method of
controlling the limit, it also gives each location the chance to
control how much of the history they can see.

The staff client uses the openils.PermaCRUD JavaScript module to
retrieve information to display to the user. As mentioned above,
pcrud is the wrong permission model for org. unit settings.
Additionally, the IDL specifies an empy retrieve permission, which
means that none is required to view the history of any setting. This
not only exposes settings in the staff client, but anyone who can
currently login and make the appropriate open-ils.pcrud call remotely
could also gain access to the history of a given setting or settings.

Additionally, the pcrud search is done only on the setting field name
itself. Thus all settings are exposed in the history view and not
just those that concern the currently logged in user. It makes sense
to me that the retrieval should limit itself t...