Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » patch for imap bug 77153 »

patch for imap bug 77153

php.internals

Stas Malyshev

7 years ago
Hi! I've checked in the patch for https://bugs.php.net/bug.php?id=77153, which disables by default rsh/ssh login functionality in IMAP. I assume most people neither know such functionality existed nor need it, but still it's a BC break. The reason why I did it is because IMAP library does not validate mailbox parameters it sends to the underlying shell commands, which creates all kinds of unpleasant security scenarios (see bug for details). Strictly speaking, such bug is a problem in the library, not PHP wrapper, since all parsing and mailbox string handling is done inside the library and it completely opaque to PHP. However, c-client library has been essentially unsupported for many years (why we're using an ancient unsupported library is a separate issue which we'd probably want to address but let's not get distracted) so no fix is probably coming from that direction. And since imap extension is used by a bunch of tools and most are not aware underlying library has this vulnerability, I think disabling this function is a right thing to do. More details in the bug and in the UPGRADING note. I've merged patch now since the issue is public (essentially has been for a while, and was first submitted as https://bugs.php.net/bug.php?id=76428 but at the time I haven't realized c-client is not going to be fixed, which is my fault - should have checked the status of this library). Despite it not being a PHP issue per se, I think we may still want a CVE for it. For RMs, please incorporate it into the next release. Maybe not that urgent for PHP 7.3.0RC6 since it's not a production release anyway. Please comment if you see any troubles or have any questions about the fix.
-- Stas Malyshev smalyshev@gmail.com

Christoph Becker

7 years ago
On 20.11.2018 at 20:45, Stanislav Malyshev wrote:
> Strictly speaking, such bug is a problem in the library, not PHP > wrapper, since all parsing and mailbox string handling is done inside > the library and it completely opaque to PHP. However, c-client library > has been essentially unsupported for many years (why we're using an > ancient unsupported library is a separate issue which we'd probably want > to address but let's not get distracted) so no fix is probably coming > from that direction. And since imap extension is used by a bunch of > tools and most are not aware underlying library has this vulnerability, > I think disabling this function is a right thing to do. More details in > the bug and in the UPGRADING note.
I fully agree with the fix (thanks!), and also that it is a security issue. However, I don't think it's really a problem in c-client; actually the PHP wrapper should not have allowed to pass the mailbox name verbatim, which would only be reasonable in my opinion, if we were supporting arbitrary drivers (which we don't). And of course, userland clients should not pass unvalidated input as mailbox name, but as you said, quite likely at least some developers are not aware that potentially arbitrary shell commands could be executed this way, and our docs don't explicitly mention this issue.
> For RMs, please incorporate it into the next release. Maybe not that > urgent for PHP 7.3.0RC6 since it's not a production release anyway.
PHP-7.3.0 has already been branched, and PHP-7.2.13RC1 and PHP-7.3.0RC6 have already been tagged without the patch. We probably should re-tag.
-- Christoph M. Becker

Stas Malyshev

7 years ago
Hi!
> actually the PHP wrapper should not have allowed to pass the mailbox > name verbatim, which would only be reasonable in my opinion, if we were > supporting arbitrary drivers (which we don't). And of course, userland
PHP does not do anything with mailbox names. In fact, PHP doesn't even know what mailbox name is (it just passes opaque string to c-client and hopes c-client can make sense of it, whatever that string does). Thus, we support everything c-client supports, AFAIK. We could extract the whole parsing logic from c-client and re-implement it, but I don't think it's worth the time spent.
-- Stas Malyshev smalyshev@gmail.com

Pierre Joye

7 years ago
On Wed, Nov 21, 2018, 6:02 AM Stanislav Malyshev <smalyshev@gmail.com wrote:
> Hi! > > > actually the PHP wrapper should not have allowed to pass the mailbox > > name verbatim, which would only be reasonable in my opinion, if we were > > supporting arbitrary drivers (which we don't). And of course, userland > > PHP does not do anything with mailbox names. In fact, PHP doesn't even > know what mailbox name is (it just passes opaque string to c-client and > hopes c-client can make sense of it, whatever that string does). Thus, > we support everything c-client supports, AFAIK. We could extract the > whole parsing logic from c-client and re-implement it, but I don't think > it's worth the time spent.
Btw, is imap on the list to deprecate in 7.x + kill in 8.x? It is really not maintained well, both c-client and the ext. Would it be possible to consider it? best, Pierre

Kalle Sommer Nielsen

7 years ago
Den ons. 21. nov. 2018 kl. 06.13 skrev Pierre Joye <pierre.php@gmail.com>:
> Btw, is imap on the list to deprecate in 7.x + kill in 8.x? It is really > not maintained well, both c-client and the ext. Would it be possible to > consider it?
I remember we have spoken about this in the past and it seems that everytime it comes up, is the argument that we don't provide a real mail alternative. I mean sure there is lots of userland based implementations and all, but I still think that PHP should provide something out of the box, but I do agree that at one point, the security concerns will start to vastly outweight the unmaintained library, but its really something that should be discussed for a future release.
-- regards, Kalle Sommer Nielsen kalle@php.net

Alice Wonder

7 years ago
On 11/20/2018 11:38 PM, Kalle Sommer Nielsen wrote:
> Den ons. 21. nov. 2018 kl. 06.13 skrev Pierre Joye <pierre.php@gmail.com>: >> Btw, is imap on the list to deprecate in 7.x + kill in 8.x? It is really >> not maintained well, both c-client and the ext. Would it be possible to >> consider it? > > I remember we have spoken about this in the past and it seems that > everytime it comes up, is the argument that we don't provide a real > mail alternative. I mean sure there is lots of userland based > implementations and all, but I still think that PHP should provide > something out of the box, but I do agree that at one point, the > security concerns will start to vastly outweight the unmaintained > library, but its really something that should be discussed for a > future release. > > >
To me it is not logical to continue maintaining a wrapper to an abandoned library just so the functions continue to exist for the sake of existing. That seems like a bad policy. Import the library into core and maintain it or write an alternate from scratch - IF the functions it provides truly has market demand. Otherwise remove support. Those seem to me like the only logical options. Nostalgia is not logical. I just checked my roundcube install, it does not use this wrapper, so clearly IMAP connectivity can be obtained through other means. That makes me feel inclined to believe this wrapper is not needed. I don't vote, but PHP shouldn't be like OpenSSL was with things like heartbeat - an extension almost no one used and seemingly no one maintained yet it was everywhere and vulnerable. Maybe stick the IMAP wrapper in PECL if someone needs it badly enough to be willing to maintain the wrapper there. Less is more, more or less.

Stas Malyshev

7 years ago
Hi!
> Import the library into core and maintain it or write an alternate from > scratch - IF the functions it provides truly has market demand.
Nice suggestion if we had a line of volunteers at our doors asking us to give them something to maintain. We don't. So this is not an option, it's a happy dream. Realistic option is imap support that we have now or no imap support at all. Market demand for functionality is always way exceeds market supply of maintainers, especially long-term maintainers. If we have somebody coming forward and volunteering to write new imap extension (or take over c-client maintainership) - excellent, problem solved. But I don't see it happening now.
> Otherwise remove support. Those seem to me like the only logical > options. Nostalgia is not logical.
Functionality is logical. We don't have imap module because people need to feel nice about keeping imap module. We have imap module because imap functionality is used. There's a lot of PHP packages using imap on github, if you look. Can they switch? Probably. But let's not pretend that the situation is we're having this module in core out of pure nostalgia. We have it because it's functionality that is needed by people.
-- Stas Malyshev smalyshev@gmail.com

Christoph Becker

7 years ago
On 21.11.2018 at 08:38, Kalle Sommer Nielsen wrote:
> Den ons. 21. nov. 2018 kl. 06.13 skrev Pierre Joye <pierre.php@gmail.com>: > >> Btw, is imap on the list to deprecate in 7.x + kill in 8.x? It is really >> not maintained well, both c-client and the ext. Would it be possible to >> consider it? > > I remember we have spoken about this in the past and it seems that > everytime it comes up, is the argument that we don't provide a real > mail alternative. I mean sure there is lots of userland based > implementations and all, but I still think that PHP should provide > something out of the box, but I do agree that at one point, the > security concerns will start to vastly outweight the unmaintained > library, but its really something that should be discussed for a > future release.
One particular issue wrt. removing ext/imap is that the mail testsuite[1] relies on this extension on Windows, so the tests would have to be rewritten first. [1] <https://github.com/php/php-src/tree/php-7.3.0RC5/ext/standard/tests/mail>
-- Christoph M. Becker

Michael Kliewe

7 years ago
Am 21.11.2018 um 06:13 schrieb Pierre Joye:
> On Wed, Nov 21, 2018, 6:02 AM Stanislav Malyshev <smalyshev@gmail.com wrote: > >>> actually the PHP wrapper should not have allowed to pass the mailbox >>> name verbatim, which would only be reasonable in my opinion, if we were >>> supporting arbitrary drivers (which we don't). And of course, userland >> PHP does not do anything with mailbox names. In fact, PHP doesn't even >> know what mailbox name is (it just passes opaque string to c-client and >> hopes c-client can make sense of it, whatever that string does). Thus, >> we support everything c-client supports, AFAIK. We could extract the >> whole parsing logic from c-client and re-implement it, but I don't think >> it's worth the time spent. > > Btw, is imap on the list to deprecate in 7.x + kill in 8.x? It is really > not maintained well, both c-client and the ext. Would it be possible to > consider it?
There was a vote in 2015 if ext/imap should be removed from core: https://wiki.php.net/rfc/removal_of_dead_sapis_and_exts#extimap Michael

Christoph Becker

7 years ago
On 21.11.2018 at 00:01, Stanislav Malyshev wrote:
>> actually the PHP wrapper should not have allowed to pass the mailbox >> name verbatim, which would only be reasonable in my opinion, if we were >> supporting arbitrary drivers (which we don't). And of course, userland > > PHP does not do anything with mailbox names. In fact, PHP doesn't even > know what mailbox name is (it just passes opaque string to c-client and > hopes c-client can make sense of it, whatever that string does). Thus, > we support everything c-client supports, AFAIK. We could extract the > whole parsing logic from c-client and re-implement it, but I don't think > it's worth the time spent.
libc-client supports *arbitrary* drivers[1], but the PHP wrapper does not[2] (presumably for good reasons[3]). Thus, the PHP wrapper could have expected an array (or some such) instead of a string, assembling the mailbox name from white-listed elements. This would have been a much cleaner and more robust API. Anyhow, this is water under the bridge now, and I think we should issue a call for maintainership[4] for ext/imap as soon as possible, since this is not the only issue[5] of this unmaintained[6] extension. [1] see docs/drivers.txt in the imap-2007f distribution [2] <https://github.com/php/php-src/blob/php-7.3.0RC5/ext/imap/php_imap.c#L845-L859> [3] see the mail_link() docs in docs/internal.txt [4] <https://wiki.php.net/rfc/umaintained_extensions#proposal> [5] <https://bugs.php.net/search.php?cmd=display&package_name[]=IMAP+related&direction=DESC&limit=30&status=Open> [6] <https://github.com/php/php-src/blob/5a5f3617558b7a278a5c73652087b3d89927ccba/EXTENSIONS#L265>
-- Christoph M. Becker

Stas Malyshev

7 years ago
Hi!
> Anyhow, this is water under the bridge now, and I think we should issue > a call for maintainership[4] for ext/imap as soon as possible, since > this is not the only issue[5] of this unmaintained[6] extension.
Pierre is listed as maintainer. But given that he is for deprecating it, I guess we try to find somebody who is willing to revive it (not likely) and failing that (likely) go the deprecation route probably... Given how many references to it I see on github though, it's not the best outcome.
-- Stas Malyshev smalyshev@gmail.com

Bishop Bettini

7 years ago
On Wed, Nov 21, 2018 at 7:27 PM Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > > > Anyhow, this is water under the bridge now, and I think we should issue > > a call for maintainership[4] for ext/imap as soon as possible, since > > this is not the only issue[5] of this unmaintained[6] extension. > > Pierre is listed as maintainer. But given that he is for deprecating it, > I guess we try to find somebody who is willing to revive it (not likely) > and failing that (likely) go the deprecation route probably... Given how > many references to it I see on github though, it's not the best outcome. >
I'd like to take this one, too. RoundCube may roll its own, but I know the popular Horde IMP uses this extensively. I suspect that the extension's going to have the performance one would desire when dealing with giant mailboxes, but c-client is hard to deal with. I've had experience with c-client for a while (stretching back to Pine days), and yet still would like to roadmap our way to a modern implementation. bishop

Stas Malyshev

7 years ago
Hi!
> Hi! > > > Anyhow, this is water under the bridge now, and I think we should > issue > > a call for maintainership[4] for ext/imap as soon as possible, since > > this is not the only issue[5] of this unmaintained[6] extension. > > Pierre is listed as maintainer. But given that he is for deprecating it, > I guess we try to find somebody who is willing to revive it (not likely) > and failing that (likely) go the deprecation route probably... Given how > many references to it I see on github though, it's not the best outcome. > > > I'd like to take this one, too.
That's be great. I see there are 57 open bugs for imap, triaging them would be a good start.
> RoundCube may roll its own, but I know the popular Horde IMP uses this > extensively. I suspect that the extension's going to have the > performance one would desire when dealing with giant mailboxes, but > c-client is hard to deal with. I've had experience with c-client for a > while (stretching back to Pine days), and yet still would like to > roadmap our way to a modern implementation.
Migrating to something more modern (and not dead :) would definitely be great. May be not that easy though given that one has to keep both APIs and mailbox formats (at least to some measure) for it to be useful. Alternative imap implementation may be a good route too...
-- Stas Malyshev smalyshev@gmail.com

Jan Schneider

7 years ago
Zitat von Bishop Bettini <bishop@php.net>:
> On Wed, Nov 21, 2018 at 7:27 PM Stanislav Malyshev <smalyshev@gmail.com> > wrote: > >> Hi! >> >> > Anyhow, this is water under the bridge now, and I think we should issue >> > a call for maintainership[4] for ext/imap as soon as possible, since >> > this is not the only issue[5] of this unmaintained[6] extension. >> >> Pierre is listed as maintainer. But given that he is for deprecating it, >> I guess we try to find somebody who is willing to revive it (not likely) >> and failing that (likely) go the deprecation route probably... Given how >> many references to it I see on github though, it's not the best outcome. >> > > I'd like to take this one, too. > > RoundCube may roll its own, but I know the popular Horde IMP uses this > extensively.
Horde isn't using ext/imap since ages, but has developed a PHP-only IMAP library long ago, that's being used by many other OSS projects too.
> I suspect that the extension's going to have the performance > one would desire when dealing with giant mailboxes, but c-client is hard to > deal with. I've had experience with c-client for a while (stretching back > to Pine days), and yet still would like to roadmap our way to a modern > implementation. > > bishop
-- Jan Schneider The Horde Project https://www.horde.org/

Bishop Bettini

7 years ago
On Fri, Feb 1, 2019 at 11:45 AM Jan Schneider <jan@horde.org> wrote:
> > Zitat von Bishop Bettini <bishop@php.net>: > > > On Wed, Nov 21, 2018 at 7:27 PM Stanislav Malyshev <smalyshev@gmail.com> > > wrote: > > > >> Hi! > >> > >> > Anyhow, this is water under the bridge now, and I think we should > issue > >> > a call for maintainership[4] for ext/imap as soon as possible, since > >> > this is not the only issue[5] of this unmaintained[6] extension. > >> > >> Pierre is listed as maintainer. But given that he is for deprecating it, > >> I guess we try to find somebody who is willing to revive it (not likely) > >> and failing that (likely) go the deprecation route probably... Given how > >> many references to it I see on github though, it's not the best outcome. > >> > > > > I'd like to take this one, too. > > > > RoundCube may roll its own, but I know the popular Horde IMP uses this > > extensively. > > Horde isn't using ext/imap since ages, but has developed a PHP-only > IMAP library long ago, that's being used by many other OSS projects too. >
I appreciate the clarification, Jan, thanks. I must have been reading the h3 docs, instead of current. o.O The library is indeed very nice, https://github.com/horde/Imap_Client/ Looks like it's faster than native ext/imap, too https://dev.horde.org/imap_client/ One recent SO answer comparing ext/imap and Horde Imap_Client: https://stackoverflow.com/q/6659409/2908724 Given the high quality userland option(s) out there, it seems a waste of effort to revitalize the extension with a fresh imap implementation. We should be fixing the bugs though, and I'll focus on that. For now let's just call it actively maintained, but feature frozen? bishop