open-ils.auth.login fails when password contains percent sign

Bug #1830642 reported by Jeff Davis on 2019-05-27
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
OpenSRF
Medium
Unassigned

Bug Description

If a user's password contains a percent sign (%), open-ils.auth.login will always fail. Old-style authentication with init/complete works just fine.

Jeff Davis (jdavis-sitka) wrote :

This bug is annoying to test manually, so I've added a live test for it in working branch user/jeffdavis/lp1830642-password-percent (using the existing test for open-ils.auth.login):

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jeffdavis/lp1830642-password-percent

The updated live test authenticates a user via both init/complete and open-ils.auth.login; currently the latter test fails when the password contains a percent sign, but init/complete succeeds. The system is restored to its initial state at the end, so you can safely run the test multiple times to verify the issue and to test any potential bugfix.

I haven't figured out what's causing the problem so I don't have a fix. If anyone feels like tackling this bug, please feel free to do so. :)

Bill Erickson (berick) wrote :

I suspect this is caused by the OpenSRF utils.c md5sum() function accepting variable length arguments. A quick glance suggests no existing OpenSRF or Evergreen code takes advantage of this, so I propose we just change the md5sum() function definition.

Confirming...

Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Bill Erickson (berick) wrote :

Issue and fix confirmed. OpenSRF branch pushed:

https://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/berick/lp1830642-md5sum-no-var-args

Thanks for the tests, Jeff! Very helpful.

Changed in evergreen:
status: New → Confirmed
Changed in opensrf:
milestone: none → 3.1.1
Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Changed in opensrf:
status: New → Confirmed
tags: added: pullrequest
Galen Charlton (gmc) wrote :

I believe the patch as it breaks ABI compatibility. For the sake of maintenance releases, is there any reason not to (for rel_3_1 and rel_3_0) keep the function notionally variadic but remove the invocation of VA_LIST_TO_STRING, similar to commit 6414c25?

Galen Charlton (gmc) on 2019-06-07
Changed in opensrf:
milestone: 3.1.1 → 3.1.2
Jeff Davis (jdavis-sitka) wrote :

The commit mentioned by Galen was a fix for bug 1702978. I don't know anything about ABI compatibility so I can't comment on Galen's suggestion.

I am marking this bug as Medium priority since there is a workaround (don't use % in passwords).

Changed in evergreen:
importance: Undecided → Medium
Changed in opensrf:
importance: Undecided → Medium
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers