When using services_basic_auth 7.x-1.x-dev, if page caching is on and an unauthenticated request (or a request with failed authentication) is made to a services endpoint, that result can become cached, such that _services_basic_auth_authenticate_call() is bypassed and even properly authenticated requests produce an empty response until caches are cleared.

CommentFileSizeAuthor
#6 services_basic_auth-unauthenticated_requests_caching-2287771-6.patch1.04 KBmarkpavlitski
#4 interdiff.txt505 bytesmarkpavlitski
#4 services_basic_auth-unauthenticated_requests_caching-2287771-4.patch1.14 KBmarkpavlitski
#3 services_basic_auth-unauthenticated_requests_caching-2287771-3.patch592 bytesbadjava
#1 services_basic_auth-unauthenticated_requests_caching-2287771-1.patch1.08 KBmsupko

Comments

msupko’s picture

msupko commented
StatusFileSize
new services_basic_auth-unauthenticated_requests_caching-2287771-1.patch1.08 KB

This patch seeks to resolve the issue by sidestepping cache for all requests to the endpoint, not just those for which authentication succeeds.

pere orga’s picture

pere orga
Primary language Catalan
commented
Version: 7.x-1.x-dev »
Status: Needs review » Active

Thanks. See SA-CONTRIB-2015-050 - Services Basic Authentication - Access bypass - Unsupported

badjava’s picture

badjava commented
StatusFileSize
new services_basic_auth-unauthenticated_requests_caching-2287771-3.patch592 bytes

Given the current state of this module, I re-rolled the patch so that it can be applied cleanly on the 1.1 release while this is being resolved.

markpavlitski’s picture

markpavlitski commented
Assigned: msupko » Unassigned
Priority: Major » Critical
Status: Active » Needs review
StatusFileSize
new services_basic_auth-unauthenticated_requests_caching-2287771-4.patch1.14 KB
new interdiff.txt505 bytes

The patch in #1 is a good approach, but the issue can still be triggered when using non-database cache backends, when $conf['page_cache_without_database'] is TRUE, since that bypass the $conf['cache'] check. See: _drupal_bootstrap_page_cache().

The attached patch (based on latest 7.x-dev) forces caching to be disable in that case too.

@Pere Orga I'm happy to take over maintainership of this module to get a secure release out, if required.

Bumping the priority up to critical priority due to SA-CONTRIB-2015-050.

scor’s picture

scor commented

Shouldn't the patch use drupal_page_is_cacheable()?

markpavlitski’s picture

markpavlitski commented
StatusFileSize
new services_basic_auth-unauthenticated_requests_caching-2287771-6.patch1.04 KB

@scor yes, looking at _drupal_bootstrap_page_cache(), that would work too and makes the code a bit cleaner.

The main thinking on the previous patch was just minimal code changes to fix the problem, since that was the approach being used already.

New patch attached.

skwashd’s picture

skwashd commented
Status: Needs review » Reviewed & tested by the community

I have tested this locally and the patch at #6 solves the security issue. The docs suggest that drupal_page_is_cacheable(FALSE) is the correct way to flag a page as being uncachable. RTBC.

  • markpavlitski committed fee9254 on 7.x-1.x 
    Issue #2287771 by markpavlitski, msupko, badjava, skwashd:...
skwashd’s picture

skwashd commented
Status: Reviewed & tested by the community » Fixed

This was committed 10 days ago, marking as fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.