memcache keys containing % fail

[Mike Rylander]

In src/libopensrf/osrf_cache.c we make heavy use of VA_LIST_TO_STRING to (seemingly) duplicate a string from a user that's used as a key for memcache. The key is effectively used as the format to va_list. If that key contains a '%' then the obvious, and bad, thing happens. Note: we are doing this in the "get" methods, but not in the "put" methods, so the keys are being created in memcache, but can't be looked up.

This can happen if a username or barcode contains a '%', and a user attempts to log in (using the open-ils.auth service, and thus the C functions in question).

Is there any institutional memory regarding why we're using VA_LIST_TO_STRING at all? Should we not just use the key directly via _clean_key()?

[Bill Erickson (berick)]

IIRC, it was purely for convenience. Looks like it was never used, though. +1 to dropping the variable args entirely and just passing the key to _clean_key().

[Galen Charlton (gmc)]

Yeah, I did some trawling and it looks like commit 3fdecfb0 is when osrfCacheGet* became variadic, but that commit does not introduce any use of variadicity.

For the purpose of a security release, I suggest breaking the change into two parts:

- dropping use of VA_LIST_TO_STRING for 2.4.x and 2.5.x
- making those functions non-variadic for 2.6.0.

Doing it this way would allow applying the security update to require only a recompilation of OpenSRF, reserving ABI changes for major OpenSRF releases.

[Mike Rylander (mrylander)]

Here's a commit to make the Get methods pretend that they are not variadic. I'll create an Evergreen bug to remove the one use of variadic functionality in a Remove call.

http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/miker/lp-1702978-variadic_get_hates_percent_in_key

This does not seem security-ish to me, after looking at it harder. One can't inject commands or clobber other keys.

[Mike Rylander (mrylander)]

Well, instead of a new bug, I just attached Evergreen. Here's the fix for it:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp-1702978-variadic_get_hates_percent_in_key

NOTE: the Evergreen fix should go in, and be backported, before the OpenSRF fix is rolled into a release.

[Scott Thomas (scott-thomas-9)]

This is a big problem for one of our libraries. The library claims the characters M and - also have the same effect. I am awaiting documentation.

Scott

[Cesar V (cesardv)]

Tested Miker's patch using concerto users.
Changed user #236's barcode to 99999338317% and username to br1bbrown% and with the patches (both EG and Opensrf) applied I was able to successfully login to myOPAC and staff client.

Here's a signed off branch for the EVergreen portion:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/cesardv/lp1702978_percents_in_patron_cards

And the related OpenSRF part:
http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/cesardv/miker_lp-1702978-variadic_get_hates_percent_in_key_SIGNOFF

[Jason Stephenson (jstephenson)]

The Evergreen bug is targeted at 3.next. Does this mean we don't think it is a real bug/good candidate for backport?

I'm asking because I can easily test this on 3.0 and add targets if we think it should be backported.

I'm leaning toward backport if it is easy enough, but will go with the consensus on this.

Also, assigning myself for testing.

[Jason Stephenson (jstephenson)]

So, I've tested both branches on Evergreen 3.0 with OpenSRF 3.0, and the fix works for me. I used a copy of production data and change my username to have a % in it. Without the branches, I could not log in. With them applied, I could.

I've added my sign offs to the branches below, but not pushed because I think we should have a conversation about putting the version that breaks ABI into OpenSRF 3.1. I'd be OK with that and using this non-ABI breaking version in OpenSRF 3.02.

Branch named user/dyrcona/lp-1702978-variadic_get_hates_percent_in_key-SIGNOFF in both working repositories.

http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/dyrcona/lp-1702978-variadic_get_hates_percent_in_key-SIGNOFF

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp-1702978-variadic_get_hates_percent_in_key-SIGNOFF

[Jason Stephenson (jstephenson)]

I pushed the fixes to OpenSRF master and OpenSRF rel_3_0 as well as Evergreen master, rel_3_1, and rel_3_0.

Mike thinks we should add the ABI breaking version of the OpenSRF fix before OpenSRF 3.1 is released. That said, I'm still marking that milestone "Fix Committed."

Thanks, Mike, Galen, and Cesar!