Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125896
Precedence: bulk
MIME-Version: 1.0
Date: Mon, 04 Nov 2024 12:38:08 +0100
To: erictnorris@gmail.com
Cc: PHP internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [VOTE] Add persistent curl share handles
In-Reply-To: <CA+NMNO0HVQAXkzcyYPjuF0KQBs-=1DE0-Bki2w0KXPUmvfjovA@mail.gmail.com>
References: <CA+NMNO16_p1=vmjpKSfWgSVCVqgxC0v-VGgfX4H5RhfGNNpSvg@mail.gmail.com>
 <6e533102b9a2e9c8f6a2183440b2601a@bastelstu.be>
 <CA+NMNO3rNd3vDth_0n6MOEdDorfHZXGymQXnk_ehwM=htvLeyQ@mail.gmail.com>
 <cd7343897b0a92d43c82062a49520fa5@bastelstu.be>
 <CA+NMNO0HVQAXkzcyYPjuF0KQBs-=1DE0-Bki2w0KXPUmvfjovA@mail.gmail.com>
Message-ID: <9dd47a928fd4dc673a137c6433cd3130@bastelstu.be>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

Am 2024-10-28 16:31, schrieb Eric Norris:
> I think it's interesting to note that within a request, users are
> still vulnerable to accidentally over-sharing cookies. It's unclear to

Yes.

> me why we would draw the line at persistence, considering it would be
> opt-in. That is, even if you're not using persistence, you must not
> accidentally share cookies within a request (say, if you were issuing
> batch requests for multiple users at once); if you're using
> persistence, you must also not accidentally share cookies between
> persistent handles.
> 
> My concern is that this feels a little arbitrary, and it may in fact
> be useful for users to share cookies between requests, so removing the
> functionality when persistence is enabled might be a mistake. That
> said, if this RFC vote fails and the only way to get a "yes" vote was
> to remove shared cookie functionality, I would do that.

The differences between using a CurlShareHandle in a single request and 
using it across multiple requests are:

1. For a majority of applications, the security boundary (e.g. the 
currently logged-in user) does not change within a single request, 
making it less likely to actually be an issue.
2. Unless using globals or static variables - which are generally 
considered a bad practice in newly written PHP code -, you generally 
would have a fairly explicit data flow using local variables, parameters 
and return values for a single CurlShareHandle, making it hard to 
*accidentally* reuse a CurlShareHandle where not intended. The 
persistence across requests is effectively pulling a handle in an 
unknown from a global array, in other words: Spooky action from a 
distance.

> Here's a pull request indicating that the curl team considers TLS
> reuse safe: https://github.com/curl/curl/pull/1917. I believe they
> consider it a vulnerability if you are able to make curl incorrectly
> reuse a TLS session with differing TLS settings.

Thank you. That would be useful to include in the “References” section 
of the RFC. Changing that one even during the vote seems legal to me, 
because it does not change the actual proposal.

> I am also a big fan of misuse-resistant design - I am on the Security
> team at Etsy - but I'm struggling with the hypothetical here.
> Persistence is not enabled by default, so developers would have to
> opt-in. If a library internally chooses to use persistent share
> handles, the author of the library should be responsible for designing
> a misuse-resistant library. The same goes for an add-on in custom CMS
> software.

Designing a misuse-resistant API around globally shared mutable state is 
very hard. I believe needs to happen on the lowest level, because a 
single high-level library can't have a full picture of the situation. 
libcurl making it impossible to hold incorrectly - with the Exception of 
Cookies - is a good thing here.

> That said, as I mentioned above I would be fine with removing cookie
> jar persistence if that was necessary to secure a passing vote, since
> it's not our primary focus.

Given the information regarding the TLS re-use, the cookie sharing is my 
only remaining concern. In fact with cookie sharing disabled it might 
not even be necessary for the user to choose an ID: Given that libcurl 
does the heavy lifting, as a user I should only need a single share 
handle and let libcurl figure out the details, no? A boolean “share 
connections across requests” when initializing a CurlHandle should 
probably sufficient.

> In the same vein as the "subtle" talk above, I'd point out that a
> library developer could set CURLOPT_SSL_VERIFYPEER to false "because
> it's faster". They shouldn't, because a library should have the right
> security settings for a user. I would expect that the library would
> not use the cookie jar functionality unless they exposed a way to
> select the correct security context.

See above: Globally shared mutable state is much different than changing 
a setting in a localized fashion. The former is very hard to statically 
analyze for safety, the latter can easily be audited with a `grep` 
(leaving aside users intentionally obfuscating the constant's value).

Best regards
Tim Düsterhus