[openssl.org #359] Calling SSL_read and SSL_write with non-empty error stack may cause an error

[Arne Ansper via RT]

Hi!

I have a small problem with SSL_get_error. This function starts like this:

int SSL_get_error(SSL *s,int i)
{
int reason;
unsigned long l;
BIO *bio;

if (i > 0) return(SSL_ERROR_NONE);

/* Make things return SSL_ERROR_SYSCALL when doing SSL_do_handshake
* etc, where we do encode the error */
if ((l=ERR_peek_error()) != 0)
{
if (ERR_GET_LIB(l) == ERR_LIB_SYS)
return(SSL_ERROR_SYSCALL);
else
return(SSL_ERROR_SSL);
}

Now, if I have something in the error stack from previous operations and I
call SSL_read or SSL_write on non-blocking socket and read or write
returns -1 and sets errno to EAGAIN, then SSL_get_error will return
SSL_ERROR_SSL which makes this error look like an error in SSL
statemachine but it is really not.

Since BIO_f_ssl does not set retry flag for SSL_ERROR_SSL connection
aborts.

One solution to this problem is to make sure that ERR_clear_errors gets
called every time before you do SSL_read or SSL_write (or
BIO_read/BIO_write). But I think that this is not a very good approach,
because it relies on the good habbits of the application programmer.
Calling ERR_clear_errors automatically at the start of SSL_read and
SSL_write might hurt the performance and is therefore probably not a very
good solution.

I think that the good long term solution is do not rely on ERR_peek_error
inside library, because application can call OpenSSL functions with an
error stack that is in an arbitrary state.

I grepped the (almost) current source tree for ERR_peek_error and
ERR_get_error and found that it's fortunately used in just a very few
places:

apps/req.c: if ((ERR_GET_REASON(ERR_peek_error()) ==
apps/rsa.c: while ((e = ERR_peek_error()) != 0 &&
apps/rsa.c: ERR_get_error(); /* remove e from error stack */
apps/rsa.c: if (r == -1 || ERR_peek_error() != 0) /* should happen only if r == -1 */
ssl/ssl_lib.c: if ((l=ERR_peek_error()) != 0)
ssl/ssl_rsa.c: if (ERR_peek_error() != 0)
ssl/ssl_rsa.c: err = ERR_peek_error();
ssl/ssl_rsa.c: (void) ERR_get_error();
test/bntest.c: while ((l=ERR_get_error()))
test/rsa_test.c: while ((l = ERR_get_error()) != 0)
crypto/asn1/a_d2i_fp.c: e=ERR_GET_REASON(ERR_peek_error());
crypto/asn1/a_d2i_fp.c: ERR_get_error(); /* clear error */
crypto/bn/bntest.c: while ((l=ERR_get_error()))
crypto/objects/obj_dat.c: ERR_get_error();
crypto/pem/pem_info.c: error=ERR_GET_REASON(ERR_peek_error());
crypto/pem/pem_lib.c: if(ERR_GET_REASON(ERR_peek_error()) ==
crypto/pkcs7/bio_ber.c: (ERR_GET_REASON(ERR_peek_error()) == ASN1_R_TOO_LONG))
crypto/pkcs7/bio_ber.c: ERR_get_error(); /* clear the error */
crypto/rand/md_rand.c: err = ERR_peek_error();
crypto/rand/md_rand.c: (void)ERR_get_error();
crypto/rsa/rsa_test.c: while ((l = ERR_get_error()) != 0)
crypto/x509/by_file.c: if ((ERR_GET_REASON(ERR_peek_error()) ==
crypto/x509/by_file.c: if ((ERR_GET_REASON(ERR_peek_error()) ==

I suggest that if the exact kind of the error is important we should add
an explicit parameter to the lower level functions to return this
information. Or additional status fields to strucutres like SSL.

Arne

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Lutz Jaenicke via RT]

Post by Arne Ansper via RT
Hi!
I have a small problem with SSL_get_error. This function starts like
int SSL_get_error(SSL *s,int i)
{
int reason;
unsigned long l;
BIO *bio;
if (i > 0) return(SSL_ERROR_NONE);
/* Make things return SSL_ERROR_SYSCALL when doing
SSL_do_handshake
* etc, where we do encode the error */
if ((l=ERR_peek_error()) != 0)
{
if (ERR_GET_LIB(l) == ERR_LIB_SYS)
return(SSL_ERROR_SYSCALL);
else
return(SSL_ERROR_SSL);
}
Now, if I have something in the error stack from previous operations
and I
call SSL_read or SSL_write on non-blocking socket and read or write
returns -1 and sets errno to EAGAIN, then SSL_get_error will return
SSL_ERROR_SSL which makes this error look like an error in SSL
statemachine but it is really not.
Since BIO_f_ssl does not set retry flag for SSL_ERROR_SSL connection
aborts.

Ooops.

Post by Arne Ansper via RT
One solution to this problem is to make sure that ERR_clear_errors
gets
called every time before you do SSL_read or SSL_write (or
BIO_read/BIO_write). But I think that this is not a very good
approach,
because it relies on the good habbits of the application programmer.
Calling ERR_clear_errors automatically at the start of SSL_read and
SSL_write might hurt the performance and is therefore probably not a
very
good solution.

Adding ERR_clear_errors() into SSL_read() etc seems to be the correct
approach. It is already handled this way in _accept(), _connect(), but
not that obvious, because it is found e.g. in ssl3_accept() which is
called depending on the method selected.

You will often find ERR_clear_errors() combined with clear_sys_error()
but obviously not in all occasions.

Unfortunately it is not obvious enough to simply add it without some
further investigation. I will thus put this issue into the 0.9.7 queue
and will not consider it for 0.9.6h anymore.

Post by Arne Ansper via RT
I suggest that if the exact kind of the error is important we should
add
an explicit parameter to the lower level functions to return this
information. Or additional status fields to strucutres like SSL.

Even though this seems to be a reasonable proposal, I am afraid that it
would require significant changes to the error handling concept...

Best regards,
Lutz




______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Arne Ansper]

Post by Lutz Jaenicke via RT
Adding ERR_clear_errors() into SSL_read() etc seems to be the correct
approach. It is already handled this way in _accept(), _connect(), but
not that obvious, because it is found e.g. in ssl3_accept() which is
called depending on the method selected.
You will often find ERR_clear_errors() combined with clear_sys_error()
but obviously not in all occasions.

I just checked. Seems that SSL_CTX_use_certificate_chain_file has a same
problem. Other uses of ERR_peek_error seem to be immune to the old entries
in error stack.

Post by Lutz Jaenicke via RT
Unfortunately it is not obvious enough to simply add it without some
further investigation. I will thus put this issue into the 0.9.7 queue
and will not consider it for 0.9.6h anymore.

0.9.7 is fine for me.

Arne

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Arne Ansper via RT]

Post by Lutz Jaenicke via RT
Adding ERR_clear_errors() into SSL_read() etc seems to be the correct
approach. It is already handled this way in _accept(), _connect(), but
not that obvious, because it is found e.g. in ssl3_accept() which is
called depending on the method selected.
You will often find ERR_clear_errors() combined with clear_sys_error()
but obviously not in all occasions.

I just checked. Seems that SSL_CTX_use_certificate_chain_file has a same
problem. Other uses of ERR_peek_error seem to be immune to the old entries
in error stack.

Post by Lutz Jaenicke via RT
Unfortunately it is not obvious enough to simply add it without some
further investigation. I will thus put this issue into the 0.9.7 queue
and will not consider it for 0.9.6h anymore.

0.9.7 is fine for me.

Arne

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Lutz Jaenicke via RT]

There was no time to solve this problem before the release of 0.9.7.
The ticket is therefore moved forward to 0.9.7a.

Best regards,
Lutz


______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Richard Levitte via RT]

Any more thoughts on this issue?

[Lutz Jaenicke via RT]

Post by Richard Levitte via RT
Any more thoughts on this issue?

The problem is not yet solved. Using the global error stack as error indicator
instead of correctly passing state back via return values is a design flaw.
It happend to make problems in the past.

I am currently busy as hell, so I will probably not be able to fix it over the
next days.

Best regards,
Lutz
--
Lutz Jaenicke ***@aet.TU-Cottbus.DE
http://www.aet.TU-Cottbus.DE/personen/jaenicke/
BTU Cottbus, Allgemeine Elektrotechnik
Universitaetsplatz 3-4, D-03044 Cottbus

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Richard Levitte via RT]

OK...

On Thu, Jan 30, 2003 at 10:09:22PM +0100, Richard Levitte via RT

Post by Richard Levitte via RT
Any more thoughts on this issue?

The problem is not yet solved. Using the global error stack as error
indicator
instead of correctly passing state back via return values is a design
flaw.
It happend to make problems in the past.
I am currently busy as hell, so I will probably not be able to fix it
over the
next days.
Best regards,
Lutz

[Bodo Moeller via RT]

Post by Arne Ansper
I just checked. Seems that SSL_CTX_use_certificate_chain_file has a same
problem. Other uses of ERR_peek_error seem to be immune to the old entries
in error stack.

One theory is that applications should not call arbitrary OpenSSL
functions while there is stuff in the error queue.

A second theory is that OpenSSL should always clear the error queue by
calling ERR_clear_error() if stuff left in the error queue might cause
confusion later.

The second theory is nicer, but until someone has patched OpenSSL
appropriately, unfortunately the first theory remains true.
--
Bodo Möller <***@cdc.informatik.tu-darmstadt.de>
PGP http://www.informatik.tu-darmstadt.de/TI/Mitarbeiter/moeller/0x36d2c658.html
* TU Darmstadt, Theoretische Informatik, Alexanderstr. 10, D-64283 Darmstadt
* Tel. [phone], Fax [phone]

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Richard Levitte - VMS Whacker]

In message <rt-359-***@openssl.org> on Fri, 31 Jan 2003 15:50:06 +0100 (MET), "Bodo Moeller via RT" <***@openssl.org> said:

rt> A second theory is that OpenSSL should always clear the error queue by
rt> calling ERR_clear_error() if stuff left in the error queue might cause
rt> confusion later.

The problem is finding out what it's appropriate. Let's not forget
that some OpenSSL functions are called from other OpenSSL functions,
so this might be tricky.

Besides, I'm not sure I agree with that theory in any case. Would
libc functions clear errno all the time?
--
Richard Levitte \ Spannvägen 38, II \ ***@stacken.kth.se
***@Stacken \ S-168 35 BROMMA \ T: [phone]
\ SWEDEN \ or [phone]
Procurator Odiosus Ex Infernis -- ***@bofh.se
Member of the OpenSSL development team: http://www.openssl.org/

Unsolicited commercial email is subject to an archival fee of $400.
See <http://www.stacken.kth.se/~levitte/mail/> for more info.
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Ben Laurie]

Post by Bodo Moeller via RT

Post by Arne Ansper
I just checked. Seems that SSL_CTX_use_certificate_chain_file has a same
problem. Other uses of ERR_peek_error seem to be immune to the old entries
in error stack.

One theory is that applications should not call arbitrary OpenSSL
functions while there is stuff in the error queue.
A second theory is that OpenSSL should always clear the error queue by
calling ERR_clear_error() if stuff left in the error queue might cause
confusion later.

How can it cause confusion (I think I missed something here)? You should
only look at the error stack if you got an error, surely?

Cheers,

Ben.
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/

"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Arne Ansper]

Post by Ben Laurie

Post by Bodo Moeller via RT

Post by Arne Ansper
I just checked. Seems that SSL_CTX_use_certificate_chain_file has a same
problem. Other uses of ERR_peek_error seem to be immune to the old entries
in error stack.

One theory is that applications should not call arbitrary OpenSSL
functions while there is stuff in the error queue.
A second theory is that OpenSSL should always clear the error queue by
calling ERR_clear_error() if stuff left in the error queue might cause
confusion later.

How can it cause confusion (I think I missed something here)? You should
only look at the error stack if you got an error, surely?

There are places (three if I remember correctly) in OpenSSL that look at
the error stack and when there is something decide that something went
wrong.

When the ticket was opened I proposed that one should not use the contents
of the error stack for anything else other than reporting the error. If
you must differentiate between two types of errors that may happen in some
function, add an additional parameter to the function (of type int* for
example) that can carry out the special error condition from the function.

There are actually very few places inside OpenSSL where ERR_peek_error and
ERR_get_error are used, so the required changes are not big.

Arne

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Ben Laurie]

Post by Arne Ansper

Post by Ben Laurie

Post by Bodo Moeller via RT

Post by Arne Ansper
I just checked. Seems that SSL_CTX_use_certificate_chain_file has a same
problem. Other uses of ERR_peek_error seem to be immune to the old entries
in error stack.

One theory is that applications should not call arbitrary OpenSSL
functions while there is stuff in the error queue.
A second theory is that OpenSSL should always clear the error queue by
calling ERR_clear_error() if stuff left in the error queue might cause
confusion later.

How can it cause confusion (I think I missed something here)? You should
only look at the error stack if you got an error, surely?

There are places (three if I remember correctly) in OpenSSL that look at
the error stack and when there is something decide that something went
wrong.

Like I say, they should only do this if there was an error reported, surely?

Post by Arne Ansper
When the ticket was opened I proposed that one should not use the contents
of the error stack for anything else other than reporting the error. If
you must differentiate between two types of errors that may happen in some
function, add an additional parameter to the function (of type int* for
example) that can carry out the special error condition from the function.

I guess that's an alternative, but I don't see why it should be needed.

Cheers,

Ben.
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/

"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Arne Ansper]

Post by Ben Laurie
Like I say, they should only do this if there was an error reported, surely?

No. Take a look at the SSL_CTX_use_certificate_chain_file:

ret=SSL_CTX_use_certificate(ctx,x);
if (ERR_peek_error() != 0)
ret = 0; /* Key/certificate mismatch doesn't imply ret==0 ... */

Post by Ben Laurie
I guess that's an alternative, but I don't see why it should be needed.

To make it explicit. Right now the function that is called does not know
that the error code it puts into error stack will be used to make some
decision by caller. There is implicit dependency and it is bad for code
maintenance.

Arne

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Bodo Moeller]

Post by Arne Ansper

Post by Ben Laurie
Like I say, they should only do this if there was an error reported, surely?

ret=SSL_CTX_use_certificate(ctx,x);
if (ERR_peek_error() != 0)
ret = 0; /* Key/certificate mismatch doesn't imply ret==0 ... */

Actually I think this is a bug in SSL_CTX_use_certificate() -- if it
intentionally ignores an error returned by X509_check_private_key(),
it should call ERR_clear_error().

The reason why I did not fix this when I looked at this some time ago
is some rather weird code in ssl_set_cert(), the function used by
SSL_CTX_use_certificate() from which X509_check_private_key() is
called. (If you look at ssl_set_cert(), you'll see that it switches
from SSL_PKEY_DH_RSA to SSKL_PKEY_DH_DSA and the other way around,
which does not appear to make much sense.) Investigating this has
been on my "to do" list for a while. Once this has been resolved,
the lines

if (ERR_peek_error() != 0)
ret = 0; /* Key/certificate mismatch doesn't imply ret==0 ... */

can be removed from SSL_CTX_use_certificate_chain_file().

[Richard Levitte - VMS Whacker via RT]

In message <rt-359-***@openssl.org> on Fri, 31 Jan 2003 15:50:06 +0100 (MET), "Bodo Moeller via RT" <***@openssl.org> said:

rt> A second theory is that OpenSSL should always clear the error queue by
rt> calling ERR_clear_error() if stuff left in the error queue might cause
rt> confusion later.

The problem is finding out what it's appropriate. Let's not forget
that some OpenSSL functions are called from other OpenSSL functions,
so this might be tricky.

Besides, I'm not sure I agree with that theory in any case. Would
libc functions clear errno all the time?
--
Richard Levitte \ Spannvägen 38, II \ ***@stacken.kth.se
***@Stacken \ S-168 35 BROMMA \ T: [phone]
\ SWEDEN \ or [phone]
Procurator Odiosus Ex Infernis -- ***@bofh.se
Member of the OpenSSL development team: http://www.openssl.org/

Unsolicited commercial email is subject to an archival fee of $400.
See <http://www.stacken.kth.se/~levitte/mail/> for more info.

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Richard Levitte via RT]

OK, what's the status on this ticket?

Post by Ben Laurie

Post by Arne Ansper

Post by Ben Laurie
Like I say, they should only do this if there was an error reported,

surely?

Post by Arne Ansper
ret=SSL_CTX_use_certificate(ctx,x);
if (ERR_peek_error() != 0)
ret = 0; /* Key/certificate mismatch doesn't imply ret==0

... */

Post by Ben Laurie
Actually I think this is a bug in SSL_CTX_use_certificate() -- if it
intentionally ignores an error returned by X509_check_private_key(),
it should call ERR_clear_error().
The reason why I did not fix this when I looked at this some time ago
is some rather weird code in ssl_set_cert(), the function used by
SSL_CTX_use_certificate() from which X509_check_private_key() is
called. (If you look at ssl_set_cert(), you'll see that it switches
from SSL_PKEY_DH_RSA to SSKL_PKEY_DH_DSA and the other way around,
which does not appear to make much sense.) Investigating this has
been on my "to do" list for a while. Once this has been resolved,
the lines
if (ERR_peek_error() != 0)
ret = 0; /* Key/certificate mismatch doesn't imply ret==0 ... */
can be removed from SSL_CTX_use_certificate_chain_file().

[Nils Larsch via RT]

This should be fixed in 0.9.8 . As we don't want to backport the
necessary changes to 0.9.7 I close this ticket.

Cheers,
Nils
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org