> >> 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
