STARTTLS patch for imap and ftp

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

STARTTLS patch for imap and ftp

Kees Cook-9
Hello!

3 years ago, I wrote a patch[1] (and did the TSU[2]) for adding these
features to s_client.  Can this please be applied to CVS?  I've seen
other people on the mailing list asking for it[3], including fixes for
HELO[4].

This is a pretty trivial patch, and would help a lot of people.  I have
updated it (see attached) for current CVS.  Is there anything else I
need to help with to see it get committed?

Thanks,

-Kees

[1] http://marc.theaimsgroup.com/?l=openssl-dev&m=109794442901659&w=2
[2] http://marc.theaimsgroup.com/?l=openssl-dev&m=109803041012966&w=2
[3] http://marc.theaimsgroup.com/?l=openssl-dev&w=2&r=1&s=starttls&q=b
[4] http://www.mail-archive.com/openssl-dev@.../msg20600.html

--
Kees Cook                                            @outflux.net

starttls-cvs.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Richard Levitte - VMS Whacker
In message <[hidden email]> on Thu, 15 Feb 2007 10:34:23 -0800, Kees Cook <[hidden email]> said:

kees> 3 years ago, I wrote a patch[1] (and did the TSU[2]) for adding
kees> these features to s_client.  Can this please be applied to CVS?

Yes.  Done.  Thank you, and sorry you had to wait 3 years for this to
happen.

Cheers,
Richard

-----
Please consider sponsoring my work on free software.
See http://www.free.lp.se/sponsoring.html for details.

--
Richard Levitte                         [hidden email]
                                        http://richard.levitte.org/

"When I became a man I put away childish things, including
 the fear of childishness and the desire to be very grown up."
                                                -- C.S. Lewis
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Kees Cook-9
On Fri, Feb 16, 2007 at 07:12:50PM +0100, Richard Levitte - VMS Whacker wrote:
> Yes.  Done.  Thank you, and sorry you had to wait 3 years for this to
> happen.

Great!  Thank you.  :)

--
Kees Cook                                            @outflux.net
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Goetz Babin-Ebell
In reply to this post by Richard Levitte - VMS Whacker
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Richard,

Richard Levitte - VMS Whacker wrote:
> In message <[hidden email]> on Thu, 15 Feb 2007 10:34:23 -0800,
> Kees Cook <[hidden email]> said:
>
> kees> 3 years ago, I wrote a patch[1] (and did the TSU[2]) for adding
> kees> these features to s_client.  Can this please be applied to CVS?
>
> Yes.  Done.  Thank you, and sorry you had to wait 3 years for this to
> happen.

The problem (not only I have) with the patch is
that at least in SMTP and IMAP it is illegal
to start TLS before an initial protocol handshake is done:

* in SMTP doing a STARTTLS without previous EHLO
  will return a
  503 STARTTLS command used when not advertised
* in IMAP doing a STARTLS requires a
  . CAPABILITY
  first.

In both cases the server response should be parsed for
the string "STARTTLS"...

Bye

Goetz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFF1xsY2iGqZUF3qPYRAreLAJ9MF6ht6pP2nnzx5pL5x7kTwuOsuACeLyZb
QAA8Z0W0Wd6biFEb0K4D0SA=
=72Vc
-----END PGP SIGNATURE-----
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Lutz Jänicke
Goetz Babin-Ebell wrote:

> Hello Richard,
>
> Richard Levitte - VMS Whacker wrote:
> > In message <[hidden email]> on Thu, 15 Feb 2007
> 10:34:23 -0800,
> > Kees Cook <[hidden email]> said:
>
> > kees> 3 years ago, I wrote a patch[1] (and did the TSU[2]) for adding
> > kees> these features to s_client.  Can this please be applied to CVS?
>
> > Yes.  Done.  Thank you, and sorry you had to wait 3 years for this to
> > happen.
>
> The problem (not only I have) with the patch is
> that at least in SMTP and IMAP it is illegal
> to start TLS before an initial protocol handshake is done:
>
> * in SMTP doing a STARTTLS without previous EHLO
>   will return a
>   503 STARTTLS command used when not advertised
> * in IMAP doing a STARTLS requires a
>   . CAPABILITY
>   first.
>
> In both cases the server response should be parsed for
> the string "STARTTLS"...
>
This statement is technically correct. As the s_client tool is however
intended for testing purposes only (you remember that a capital
"R" at the beginning of the line will start a renegotiation instead
of being transferred to the server :-) adding the EHLO and .CAPABILITY
should be sufficient and the more complex parsing of the response
might be omitted...

Best regards,
    Lutz
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Goetz Babin-Ebell
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Lutz Jaenicke wrote:
> Goetz Babin-Ebell wrote:
[...]

>> * in SMTP doing a STARTTLS without previous EHLO
>>   will return a
>>   503 STARTTLS command used when not advertised
>> * in IMAP doing a STARTLS requires a
>>   . CAPABILITY
>>   first.
>>
>> In both cases the server response should be parsed for
>> the string "STARTTLS"...
>>
> This statement is technically correct. As the s_client tool is however
> intended for testing purposes only (you remember that a capital
> "R" at the beginning of the line will start a renegotiation instead
> of being transferred to the server :-) adding the EHLO and .CAPABILITY
> should be sufficient and the more complex parsing of the response
> might be omitted...
Do you want something like the attached patch ?
(untested, I'm off to bed...)

Bye

Goetz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFF2lTq2iGqZUF3qPYRAirhAJ9+e7H1qRzUH7RZAuHKBGpqUDrVfwCfb2A2
B7Z713+mhzGcIx5/VZHtBNA=
=ABXa
-----END PGP SIGNATURE-----

Index: apps/s_client.c
===================================================================
RCS file: /home/gbe/data/cvs/openssl/openssl/apps/s_client.c,v
retrieving revision 1.100
diff -u -r1.100 s_client.c
--- apps/s_client.c 18 Feb 2007 18:21:57 -0000 1.100
+++ apps/s_client.c 20 Feb 2007 01:47:50 -0000
@@ -914,12 +914,27 @@
  /* This is an ugly hack that does a lot of assumptions */
  if (starttls_proto == PROTO_SMTP)
  {
+ int foundit=0;
  /* wait for multi-line response to end from SMTP */
  do
  {
  mbuf_len = BIO_read(sbio,mbuf,BUFSIZZ);
  }
  while (mbuf_len>3 && mbuf[3]=='-');
+ /* STARTTLS command requires EHLO... */
+ BIO_printf(sbio,"EHLO openssl.client.net\r\n");
+ /* wait for multi-line response to end EHLO SMTP response */
+ do
+ {
+ mbuf_len = BIO_read(sbio,mbuf,BUFSIZZ);
+ if (strstr(mbuf,"STARTTLS"))
+ foundit=1;
+ }
+ while (mbuf_len>3 && mbuf[3]=='-');
+ if (!foundit)
+ BIO_printf(bio_err,
+   "didn't found starttls in server response,"
+   " try anyway...\n");
  BIO_printf(sbio,"STARTTLS\r\n");
  BIO_read(sbio,sbuf,BUFSIZZ);
  }
@@ -931,8 +946,23 @@
  }
  else if (starttls_proto == PROTO_IMAP)
  {
+ int foundit=0;
  BIO_read(sbio,mbuf,BUFSIZZ);
- BIO_printf(sbio,"0 STARTTLS\r\n");
+ /* STARTTLS command requires CAPABILITY... */
+ BIO_printf(sbio,". CAPABILITY\r\n");
+ /* wait for multi-line CAPABILITY response */
+ do
+ {
+ mbuf_len = BIO_read(sbio,mbuf,BUFSIZZ);
+ if (strstr(mbuf,"STARTTLS"))
+ foundit=1;
+ }
+ while (mbuf_len>3);
+ if (!foundit)
+ BIO_printf(bio_err,
+   "didn't found STARTTLS in server response,"
+   " try anyway...\n");
+ BIO_printf(sbio,". STARTTLS\r\n");
  BIO_read(sbio,sbuf,BUFSIZZ);
  }
  else if (starttls_proto == PROTO_FTP)
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Lutz Jänicke
Goetz Babin-Ebell wrote:

> Lutz Jaenicke wrote:
> > Goetz Babin-Ebell wrote:
> [...]
> >> * in SMTP doing a STARTTLS without previous EHLO
> >>   will return a
> >>   503 STARTTLS command used when not advertised
> >> * in IMAP doing a STARTLS requires a
> >>   . CAPABILITY
> >>   first.
> >>
> >> In both cases the server response should be parsed for
> >> the string "STARTTLS"...
> >>
> > This statement is technically correct. As the s_client tool is however
> > intended for testing purposes only (you remember that a capital
> > "R" at the beginning of the line will start a renegotiation instead
> > of being transferred to the server :-) adding the EHLO and .CAPABILITY
> > should be sufficient and the more complex parsing of the response
> > might be omitted...
>
> Do you want something like the attached patch ?
> (untested, I'm off to bed...)
>
Yes, something like this. I have applied your patch to 0.9.8 and -dev... and
was just going to write "thank you" when I discovered that it does not work.
As I just noted BIO_read() does not work "line by line" but on the message
coming in. This message is the complete multi-line response and it has
to be parsed in a different way as attached as a crude hack.

No: BIO_gets() does not work on here (not supported on "connect BIO".

Yes: all other appearances of multi-line handling are broken as well.
The multi-line handling in the SMTP greeting would fail on the first
host with a multi-line greeting and the other protocol handlers are
as buggy. I have thus left your patch in and we have to decide how to
tackle the other occurances...

Best regards,
    Lutz

Index: s_client.c
===================================================================
RCS file: /e/openssl/cvs/openssl/apps/s_client.c,v
retrieving revision 1.76.2.7
diff -u -r1.76.2.7 s_client.c
--- s_client.c 21 Feb 2007 18:20:33 -0000 1.76.2.7
+++ s_client.c 21 Feb 2007 18:53:00 -0000
@@ -735,7 +735,7 @@
  /* This is an ugly hack that does a lot of assumptions */
  if (starttls_proto == PROTO_SMTP)
  {
- int foundit=0;
+ int foundit=0, response_done = 0;
  /* wait for multi-line response to end from SMTP */
  do
  {
@@ -747,11 +747,15 @@
  /* wait for multi-line response to end EHLO SMTP response */
  do
  {
+ int ll;
  mbuf_len = BIO_read(sbio,mbuf,BUFSIZZ);
  if (strstr(mbuf,"STARTTLS"))
  foundit=1;
+ for (ll = 0; !response_done && ll < mbuf_len - 4; ll++)
+ if (mbuf[ll] == '\n' && mbuf[ll + 3] != '-')
+ response_done = 1;
  }
- while (mbuf_len>3 && mbuf[3]=='-');
+ while (mbuf_len>3 && mbuf[3]=='-' && !response_done);
  if (!foundit)
  BIO_printf(bio_err,
    "didn't found starttls in server response,"
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Dr. Stephen Henson
On Wed, Feb 21, 2007, Lutz Jaenicke wrote:

> Goetz Babin-Ebell wrote:
> > Lutz Jaenicke wrote:
> > > Goetz Babin-Ebell wrote:
> > [...]
> > >> * in SMTP doing a STARTTLS without previous EHLO
> > >>   will return a
> > >>   503 STARTTLS command used when not advertised
> > >> * in IMAP doing a STARTLS requires a
> > >>   . CAPABILITY
> > >>   first.
> > >>
> > >> In both cases the server response should be parsed for
> > >> the string "STARTTLS"...
> > >>
> > > This statement is technically correct. As the s_client tool is however
> > > intended for testing purposes only (you remember that a capital
> > > "R" at the beginning of the line will start a renegotiation instead
> > > of being transferred to the server :-) adding the EHLO and .CAPABILITY
> > > should be sufficient and the more complex parsing of the response
> > > might be omitted...
> >
> > Do you want something like the attached patch ?
> > (untested, I'm off to bed...)
> >
> Yes, something like this. I have applied your patch to 0.9.8 and -dev... and
> was just going to write "thank you" when I discovered that it does not work.
> As I just noted BIO_read() does not work "line by line" but on the message
> coming in. This message is the complete multi-line response and it has
> to be parsed in a different way as attached as a crude hack.
>
> No: BIO_gets() does not work on here (not supported on "connect BIO".
>

Note that adding a buffering BIO to the chain is a simple way to fix this.

Steve.
--
Dr Stephen N. Henson. Email, S/MIME and PGP keys: see homepage
OpenSSL project core developer and freelance consultant.
Funding needed! Details on homepage.
Homepage: http://www.drh-consultancy.demon.co.uk
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Lutz Jänicke
Dr. Stephen Henson wrote:

> On Wed, Feb 21, 2007, Lutz Jaenicke wrote:
>
>  
>> Goetz Babin-Ebell wrote:
>>    
>>> Lutz Jaenicke wrote:
>>>      
>>>> Goetz Babin-Ebell wrote:
>>>>        
>>> [...]
>>>      
>>>>> * in SMTP doing a STARTTLS without previous EHLO
>>>>>   will return a
>>>>>   503 STARTTLS command used when not advertised
>>>>> * in IMAP doing a STARTLS requires a
>>>>>   . CAPABILITY
>>>>>   first.
>>>>>
>>>>> In both cases the server response should be parsed for
>>>>> the string "STARTTLS"...
>>>>>
>>>>>          
>>>> This statement is technically correct. As the s_client tool is however
>>>> intended for testing purposes only (you remember that a capital
>>>> "R" at the beginning of the line will start a renegotiation instead
>>>> of being transferred to the server :-) adding the EHLO and .CAPABILITY
>>>> should be sufficient and the more complex parsing of the response
>>>> might be omitted...
>>>>        
>>> Do you want something like the attached patch ?
>>> (untested, I'm off to bed...)
>>>
>>>      
>> Yes, something like this. I have applied your patch to 0.9.8 and -dev... and
>> was just going to write "thank you" when I discovered that it does not work.
>> As I just noted BIO_read() does not work "line by line" but on the message
>> coming in. This message is the complete multi-line response and it has
>> to be parsed in a different way as attached as a crude hack.
>>
>> No: BIO_gets() does not work on here (not supported on "connect BIO".
>>
>>    
>
> Note that adding a buffering BIO to the chain is a simple way to fix this.
>  
Yes. I get your point :-)

Best regards,
    Lutz

Index: apps/s_client.c
===================================================================
RCS file: /e/openssl/cvs/openssl/apps/s_client.c,v
retrieving revision 1.76.2.7
diff -u -r1.76.2.7 s_client.c
--- apps/s_client.c 21 Feb 2007 18:20:33 -0000 1.76.2.7
+++ apps/s_client.c 21 Feb 2007 19:55:21 -0000
@@ -736,22 +736,28 @@
  if (starttls_proto == PROTO_SMTP)
  {
  int foundit=0;
+ BIO *fbio = BIO_new(BIO_f_buffer());
+ BIO_push(fbio, sbio);
  /* wait for multi-line response to end from SMTP */
  do
  {
- mbuf_len = BIO_read(sbio,mbuf,BUFSIZZ);
+ mbuf_len = BIO_gets(fbio,mbuf,BUFSIZZ);
  }
  while (mbuf_len>3 && mbuf[3]=='-');
  /* STARTTLS command requires EHLO... */
- BIO_printf(sbio,"EHLO openssl.client.net\r\n");
+ BIO_printf(fbio,"EHLO openssl.client.net\r\n");
+ BIO_flush(fbio);
  /* wait for multi-line response to end EHLO SMTP response */
  do
  {
- mbuf_len = BIO_read(sbio,mbuf,BUFSIZZ);
+ mbuf_len = BIO_gets(fbio,mbuf,BUFSIZZ);
  if (strstr(mbuf,"STARTTLS"))
  foundit=1;
  }
  while (mbuf_len>3 && mbuf[3]=='-');
+ BIO_flush(fbio);
+ BIO_pop(fbio);
+ BIO_free(fbio);
  if (!foundit)
  BIO_printf(bio_err,
    "didn't found starttls in server response,"
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Lutz Jänicke
In reply to this post by Goetz Babin-Ebell
Goetz Babin-Ebell wrote:

> Lutz Jaenicke wrote:
> > Goetz Babin-Ebell wrote:
> [...]
> >> * in SMTP doing a STARTTLS without previous EHLO
> >>   will return a
> >>   503 STARTTLS command used when not advertised
> >> * in IMAP doing a STARTLS requires a
> >>   . CAPABILITY
> >>   first.
> >>
> >> In both cases the server response should be parsed for
> >> the string "STARTTLS"...
> >>
> > This statement is technically correct. As the s_client tool is however
> > intended for testing purposes only (you remember that a capital
> > "R" at the beginning of the line will start a renegotiation instead
> > of being transferred to the server :-) adding the EHLO and .CAPABILITY
> > should be sufficient and the more complex parsing of the response
> > might be omitted...
>
> Do you want something like the attached patch ?
> (untested, I'm off to bed...)
Ok, I have reworked this section as discussed by using a buffering BIO and
have committed everything to CVS. I would be most pleased if somebody would
also cross-test it (the part with the multi-line IMAP response may require
some more digging as the termination should be the "." at the beginning
of the response line, not the number of chars being less than 3!?)

Best regards,
    Lutz
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: STARTTLS patch for imap and ftp

Goetz Babin-Ebell
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Lutz,
Lutz Jaenicke wrote:

> Goetz Babin-Ebell wrote:
>> Lutz Jaenicke wrote:
>> [...]
>> Do you want something like the attached patch ?
>> (untested, I'm off to bed...)
> Ok, I have reworked this section as discussed by using a buffering BIO and
> have committed everything to CVS. I would be most pleased if somebody would
> also cross-test it (the part with the multi-line IMAP response may require
> some more digging as the termination should be the "." at the beginning
> of the response line, not the number of chars being less than 3!?)

Testet against cyrus imapd 2.1.18 and exim 4.50: OK.

You may drop the test for mbuf_len>3 in the while() for the IMAP
". CAPABILITY" response.
Has anyone seen a SMTP server return more than one line in the
SMTP opening message ?

Bye

Goetz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFF3twx2iGqZUF3qPYRAscBAJ9/JrYHEqPOcLfgDShP8onKeRLYFgCffg7+
YqGeshjiakpwo4f9gDtPJa0=
=HzVN
-----END PGP SIGNATURE-----
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]