Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125870
Precedence: bulk
MIME-Version: 1.0
References: <CA+NMNO16_p1=vmjpKSfWgSVCVqgxC0v-VGgfX4H5RhfGNNpSvg@mail.gmail.com>
 <6e533102b9a2e9c8f6a2183440b2601a@bastelstu.be> <CA+NMNO3rNd3vDth_0n6MOEdDorfHZXGymQXnk_ehwM=htvLeyQ@mail.gmail.com>
 <cd7343897b0a92d43c82062a49520fa5@bastelstu.be>
In-Reply-To: <cd7343897b0a92d43c82062a49520fa5@bastelstu.be>
Reply-To: erictnorris@gmail.com
Date: Mon, 28 Oct 2024 11:31:56 -0400
Message-ID: <CA+NMNO0HVQAXkzcyYPjuF0KQBs-=1DE0-Bki2w0KXPUmvfjovA@mail.gmail.com>
Subject: Re: [PHP-DEV] [VOTE] Add persistent curl share handles
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
From: eric.t.norris@gmail.com (Eric Norris)

> >> Accidentally sharing a cookie jar for unrelated requests due to a
> >> badly
> >> chosen `$persistent_id` sounds like a vulnerability to is bound to
> >> happen to someone.
> >
> > I'll admit that I don't have a good response to this, since while I
> > agree this is possible, I don't think it outweighs the benefit that
> > persistence gives to the applications that need it (again, we won't be
> > using the cookie jar). I see you've noted concerns about
> > documentation, would this be something you'd at least feel more
> > comfortable with if curl persistence documentation included a strong
> > warning?
>
> Yes, I would be *more* comfortable if the documentation would ensure to
> appropriately the possible issues with persisting the share handle
> across multiple requests, possibly giving suggestions as to how a good
> persistent ID could be constructed (e.g. by using a hash of the current
> security context or something like that).
>
> I would also be more comfortable, if persisting a share handle with
> cookie jar sharing enabled would not be possible / would be ignored,
> because that only leaves the TLS session sharing (see below) as a
> possible safety issue.

I think it's interesting to note that within a request, users are
still vulnerable to accidentally over-sharing cookies. It's unclear to
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.

> >> As for sharing a TLS or HTTP connection,
> >> this might or might not lead to vulnerabilities similarly to cookie
> >> sharing, when TLS client certificates are used for authentication.
> >
> > I believe curl itself handles this - it won't re-use a connection if
> > the settings aren't the same.
>
> Okay, the curl documentation does not say anything about this in
> https://curl.se/libcurl/c/CURLSHOPT_SHARE.html#CURLLOCKDATASSLSESSION.

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.

> >> Also badly chosen `$persistent_id`s might result in a
> >> large number of handles accumulating, without any kind of garbage
> >> collection. For the existing persistent handles (e.g. for database
> >> connections), the ID is chosen by PHP itself, ensuring a somewhat
> >> predictable behavior.
> >
> > I agree that this may be a risk, but it's not clear to me that this is
> > a risk that people would realistically run into, considering you'd
> > want to have a small number of persistent IDs in order to benefit from
> > re-using them. Dynamically picking your persistent IDs would lead to
> > less re-use.
>
> I believe in misuse-resistant API design. Folks should be steered
> towards doing the right thing by default. As an example: What if a
> library internally choses to use persistent share handles for whatever
> reason? Or an add-on you install in your CMS software? PHP for better or
> worse is not just used for in-house software with an experienced
> development team.

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.

In a sense, I could think of this as something akin to the "subtle"
cryptography library pattern you'll find elsewhere. For example, take
https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto.
Cryptography library authors use the subtle API to implement
misuse-resistant abstractions on top of these primitives; if you find
yourself using the primitive, you are at risk of misusing it.

The risk here is *much* lower than dealing with subtle cryptography
APIs, as there are two clear misuse patterns: allocating too many
persistent handles without closing them, and reusing a persistent
handle *with a cookie jar* for multiple user-associated requests.

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.

> > Regarding the RFC text, I apologize for not filling it out more, but
> > it seemed straightforward to me: curl supports the share interface as
> > a primary feature, and there are no glaring warnings in the
> > documentation (https://curl.se/libcurl/c/libcurl-share.html)
> > indicating that you might hold the share interface incorrectly. As I
>
> The difference to me is that when using libcurl directly, you would
> generally have a more explicit data flow and don't pull a curl share
> handle in an unknown state out of thin air. See the library example
> above.

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.

> > If I were to add notes about these both to the documentation, and
> > curl_share_close closed persistent share handles, would you change
> > your vote for this or a future RFC?
>
> I'm not sure about whether I consider documentation alone to be
> sufficient, but when share handles with cookie sharing enabled would not
> be persisted (and removed from persistence when enabling the option
> after the fact), then I would be fine with trusting libcurl to do the
> right thing. Ideally this could be confirmed with some reference to the
> libcurl source code or getting upstream confirmation that this is safe
> to do.

Understood. I appreciate that even if I don't convince you of the
acceptability of the proposed API as-is, you have provided me with
enough information to try again if this RFC fails to pass. Thank you!

Eric