Looks like an MD5 hash of a password field is being displayed in record holds grid

[Cesar V]

I've noticed that the Record Holds grid, has two password fields that are from the hold requestor and the user. See attached screenshot. The grid call that fetches the hold data seems to use cstore and thus the password fields are not suppressed. This is in latest EG and Opensurf (3.0+ for both).

Branch with potential patch to follow.

[Cesar V (cesardv)]

Figured I'd just upload the patch, instead of pushing out a public branch. Just erring on the side caution...

[Jason Stephenson (jstephenson)]

Cesar, thanks for the patch. While it looks like it will do the trick in this one case, I can see this turning into a game of whack-a-mole as new cases like this pop up.

Maybe cstore should be made aware of the mechanism used to redact passwords and other data in the log files?

It may be that we need to implement a change as suggested in one of the bugs about web staff client columns to move away from the IDL to get the list of available columns and use a fixed list per interface. (I forget exactly which one of the bugs had this suggestion and a 5-minute look didn't turn up the one I wanted.)

We should reduce the reliance on cstore is some parts of the code and make actual back end calls for some of the things that are commonly retrieved by cstore. This would be a good option in the event that we can't or won't change cstore to be able to obfuscate fields. I can see the possibility that field obfuscation in cstore could lead to breakage in some other areas.

I'm just throwing out ideas here. I haven't really looked into this in detail.

[Kathy Lussier (klussier)]

I have the bug number! bug 1683385

+1 to using a fixed list per interface.

In addition to this issue, I think it solves some other usability issues where several grids are pulling in more columns than our needed for some interfaces. I would be willing to work on this next week if we decide to go in that direction.

[Mike Rylander (mrylander)]

cstore can be told to ignore fields today -- look for suppress_controller= in fm_IDL.xml. First, though, we need to make sure that there's no code expecting to see a passwd value coming out of cstore. I hope that auth is the only one that ever did need that, in which case, with auth-internal, we may be able to just have cstore drop the field on read like pcrud does, but I'm not certain.

Separately, I agree that we should stop using cstore in some areas (there's a bug from ... many moons ago ... where Bill and I tried to stop using cstore by teaching pcrud about ANONYMOUS mode) but json_query is the big problem, IIRC.

[Bill Erickson (berick)]

+1 to using fixed columns per interface.

+1 to hiding the passwd field from cstore, with an eye toward complete removal.

-1 to making services (open-ils.circ, etc.) use pcrud instead of cstore. The passwd field just needs to not exist.

+1 to Cesar's patch as a stop-gap.

[Bill Erickson (berick)]

Regarding my "The passwd field just needs to not exist" -- bug #1730484.

[Kathy Lussier (klussier)]

Adding a note that the record holds grid no longer displays the user's password as a result of subsequent code that has gone into Evergreen. I don't know if other grids are displaying this information, but should we make one of the suggested long-term fixes a blocker for the 3.2 release?

[Kathy Lussier (klussier)]

I just marked bug 1783605 as a duplicate as this one. As reported by Jennifer Pringle, the password is also showing in the holds shelf interface. We should get this fixed before the 3.2 release.

[Bill Erickson (berick)]

I've pushed a branch for an alternate approach:

user/berick/lp1729889-virtualize-passwd-field @ security.

The passwrd field is still used by the patron update API as a way for the caller to deliver the new password to the server, so we can't drop the field, unless we modify the API and calling code. Instead, I have opted to mark the field as virtual in the IDL.

This will prevent cstore/pcrud from requesting the data from the database and it means the field will no longer appear in auto-generated lists of columns. However, the column can still be used by the API as a data storage location.

I deployed the branch locally and was able to log in, log out, edit patron, verify credentials, catalog login, and catalog password changes. I also confirmed the password fields no longer display in the hold shelf grid (bug #1783605).

[Mike Rylander (mrylander)]

Note, if the branch on bug 1712854 goes in, this specific instance of surfacing a passwd hash will be removed.

[Jason Stephenson (jstephenson)]

This would warrant a 3.0.14 release, I believe. Also, is there a specific reason that this bug is kept private?

[Galen Charlton (gmc)]

Noting that during testing Bill's patch breaks patron registration since actor.usr.passwd is currently not null with no default value.

Dropping the not null constraint made patron registration and edits work, including setting and change the password:

ALTER TABLE actor.usr ALTER COLUMN passwd DROP NOT NULL;
ALTER TABLE auditor.actor_usr_history ALTER COLUMN passwd DROP NOT NULL;

[Jason Stephenson (jstephenson)]

In comment #12 Mike says that this particular instance of surfacing a password hash will be fixed if the path for another bug goes in. That patch went in.

In comment #14 Galen indicates that Bill's patch needs two changes in the database to function properly.

So, I'm asking if we consider this bug fixed or if it should be tagged needsrepatch?

[Evergreen Bug Maintenance (bugmaster)]

This would warrant a 3.4.6 release if it goes in before the security updates deadline.

[Jason Stephenson (jstephenson)]

This bug was apparently fixed by the branch for bug 1712854, which was released with Evergreen 3.2 beta.

I am not able to select the Password for display on the holds grid in Evergreen 3.2.10 nor in Evergreen 3.6.1, so this bug appears to be fixed.