Avoid information disclosure by timing attack in EntityResource::patch() [#2934520]

Problem/Motivation

Note that while this issue is a security flaw, it only exists in 8.5.x so it is not exploitable in any released version of Drupal.

#2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior recently updated the access checking for PATCH requests to entities. Interdiff #203 in that issue introduced the possibility of a timing attack.

We allow providing fields in a PATCH request, even if the user is not allowed to edit those fields, if the values match the originals. However, if the user does not even have access to view that field, we always deny the PATCH request, as otherwise the user could just try out different values until they find one which doesn't deny access at which point they know the field value, so they have in essence "viewed" the field. The following code takes care of this:

    if ($original_field->equals($received_field) && $original_field->access('view')) {
      return FALSE;
    }

The problem is the order of the checks. If the field values do not match we never reach the access check, so there will be a difference in response time depending on whether the field value is the current one or not.

Proposed resolution

Simply flipping those conditions will make sure that both conditions are always evaluated.

Comment	File	Size	Author
#2	2934520-2.patch	834 bytes	tstoeckler
#2	8.5.x: PHP 7 & MySQL 5.5 23,008 pass
#2	2934520-access-handler-hack--do-not-test.patch	905 bytes	tstoeckler
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

4 January 2018 at 22:58

tstoeckler created an issue. See original summary.

Comment #2

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler as a volunteer commented 5 January 2018 at 00:45

Status:

Active

» Needs review

File	Size
2934520-access-handler-hack--do-not-test.patch	905 bytes

2934520-2.patch	834 bytes
8.5.x: PHP 7 & MySQL 5.5 23,008 pass

I'm not sure if there is any way to test this automatically, but I did check that this is actually an issue.

To play along at home on a clean Drupal (without any patches) enable Node, HAL and Basic Auth modules and create a node with a given title. Now apply the "2934520-access-handler-hack--do-not-test.patch" patch and clear caches. Now send two patch requests to the node: one with the original title and one with a different title. The first request will take something like 20 seconds (because both edit and view access of the field are checked) and the second one will take something like 10 seconds (because only edit access is checked).

Here's my terminal for easy copy-paste and verification:

$ time curl --request PATCH --user admin:password --header 'Content-Type: application/hal+json;' --data '{"_links":{"type":{"href":"http:\/\/d8.loc\/rest\/type\/node\/article"}},"title":[{"value":"Original title","lang":"en"}]}' http://d8.loc/node/1?_format=hal_json
{"message":"Access denied on updating field \u0027title\u0027."}
real    0m20.174s
user    0m0.007s
sys     0m0.006s
$ time curl --request PATCH --user admin:password --header 'Content-Type: application/hal+json;' --data '{"_links":{"type":{"href":"http:\/\/d8.loc\/rest\/type\/node\/article"}},"title":[{"value":"Updated title","lang":"en"}]}' http://d8.loc/node/1?_format=hal_json
{"message":"Access denied on updating field \u0027title\u0027."}
real    0m10.172s
user    0m0.007s
sys     0m0.005s

Now apply the "2934520-2.patch" patch and repeat the two requests. This time they will both take around 20 seconds.

Comment #3

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 4 January 2018 at 23:36

Priority:

Normal

» Critical

I think this qualifies as critical because we can't release 8.5 without it.

Comment #4

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 4 January 2018 at 23:38

Makes sense, but having a reliable difference between the two cases seems pretty hard to achieve, we are talking about PHP after all and I did a few tries without the sleep and the at least on my laptop, Didn't do an actual empirical test, but I've seen values that vary as much as 124ms and 146ms with the same title string. So you wouldn't have to just guess the right value, you'd have to do it enough times to have a statistically relevant output. Maybe if the access logic would be very complex but that's not usually the case on a field.

Did that because I'm wondering about the security impact of this. If it would be a security issue, even just in 8.5.x, it would be a release-blocking critical, but you created it as a normal issue. That or a reason to revert that other issue, as we did before, although this is by far less complex to change than the previous problem.

Ha, actually crossposted with @larowlan :)

Comment #5

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler as a volunteer commented 5 January 2018 at 00:44

Yes - although I'm not a security expert, so take this with a fair amount of salt - I agree that it will be near impossible to exploit this under any normal circumstances. It may become exploitable with crazier access checks, though. A sleep() is obviously stupid, but if someone is asking some remote service over HTTP or is accidentally causing a router rebuild or something bizarre like that it may become noticeable.

The far-fetched-ness of this together with the fact that I would reeeeally like to avoid a revert of the mentioned issue is the reason why I didn't mark this critical from the start.

In any case, even if it is far-fetched, it really doesn't hurt in any way, i.e. it doesn't increase code complexity or anything, so, I do think we should get this in.

Comment #6

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 5 January 2018 at 00:48

Status:

Needs review

» Reviewed & tested by the community

Agreed, was just waiting for the tests to come back green :)

PS: why u no sleep?!

Comment #7

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 5 January 2018 at 07:11

Status:

Reviewed & tested by the community

» Fixed

Committed 2971ea9 and pushed to 8.5.x

thanks

Comment #8

5 January 2018 at 07:11

larowlan committed 2971ea9 on 8.5.x

Issue #2934520 by tstoeckler: Avoid information disclosure by timing...

Comment #9

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler as a volunteer commented 5 January 2018 at 09:22

Status:

Reviewed & tested by the community

» Fixed

Thanks for the quick commit! Not sure what happened, but somehow the status didn't stick.

@Berdir: Aren't we in the same timezone... !? ;-)

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 5 January 2018 at 10:18

Component:	entity system	» rest.module
Issue tags:		+API-First Initiative, +Security

I didn't even get a chance to review this!

If the field values do not match we never reach the access check, so there will be a difference in response time depending on whether the field value is the current one or not.

Wow. I suspect there are many of these timing attack surfaces in Drupal then. But the difference is that usually, lots and lots of PHP needs to execute, which is not the case for a REST PATCH request, where you can even send a single field to be modified.

IOW: timing attacks weren't really a problem for Drupal in the past, but they are for API requests made to Drupal. Great catch, @tstoeckler! 😮 👏

Huge mitigating factor: the Symfony routing and access checking is so slow (~30 ms) that it prevents accurate time measurements that'd allow the line of code in question to even be accurately measured… This is why @tstoeckler had to artificially inflate certain computations in #2 to be able to actually attack. It may become exploitable with crazier access checks, though. is true though — with crazy complex access checks (perhaps some that talk to some external service), it would be possible to deduce the current value!

The far-fetched-ness of this together with the fact that I would reeeeally like to avoid a revert of the mentioned issue is the reason why I didn't mark this critical from the start.

👏

In any case, even if it is far-fetched, it really doesn't hurt in any way, i.e. it doesn't increase code complexity or anything, so, I do think we should get this in.

👌

Comment #11

cburschka

they

CreditAttribution: cburschka at PwC's Experience Center commented 6 January 2018 at 01:40

Nice one! Timing-based leakage certainly seems to be on people's minds this week. :D

And yeah, since the entity access check can be hooked into, it does seem possible for a contrib module to make this attack feasible just by being slow. Maybe not critical, but certainly good to harden against.