New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear error queue before SSL I/O operations #439

Merged
merged 2 commits into from Mar 17, 2016

Conversation

Projects
None yet
3 participants
@horgh
Member

horgh commented Mar 13, 2016

We need to clear the SSL error queue before performing SSL I/O operations. Otherwise we can see errors that are not related to the operation we error check. SSL_get_error() inspects the thread's error queue.

I read about this here: https://www.openssl.org/docs/manmaster/ssl/SSL_get_error.html where it says "The current thread's error queue must be empty before the TLS/SSL I/O operation is attempted, or SSL_get_error() will not work reliably."

How I came across this: I have a module (irssi-tcl) that itself initiates SSL connections through scripts (HTTP in the case I found). For some URLs I found that Irssi was complaining about read errors after SSL_read() and then dropping all the active SSL IRC connections. I would see "Irssi: warning SSL read error: No such file or directory". I found the condition being hit in irssi_ssl_read() was the 'SSL_ERROR_SYSCALL' one.

Unfortunately it does not always happen so it is difficult to reliably reproduce. However after applying this change I have not been able to cause it.

Clear error queue before SSL I/O operations
Otherwise we can see errors that are not related to the operation
we check for. SSL_get_error() inspects the thread's error queue.
See https://www.openssl.org/docs/manmaster/ssl/SSL_get_error.html for
more information.
@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 13, 2016

Very nice catch, it seems you missed one spot at irssi_ssl_get_iochannel though.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 13, 2016

+1 I think that also affects glowing bear which disconnects servers even when the browser fetched the https cert of the web socket endp

@horgh

This comment has been minimized.

Member

horgh commented Mar 13, 2016

Thank you for looking at this and for the help!

Regarding having this in irssi_ssl_get_iochannel(): Where in that function do you think it would be best to do this? Perhaps before the first if statement? (if (!ssl_inited ...). I don't see one of the function calls mentioned in the documentation for SSL_get_error() in there so I am not sure where it should be.

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 13, 2016

It's ERR_get_error which should behave in the same way if I understood the documentation correctly.

@horgh

This comment has been minimized.

Member

horgh commented Mar 13, 2016

Ah... True. That function takes the first error from the same queue. The earliest apparently!

So before relying on it we should make sure that queue is cleared out.

I found there are several function calls inside irssi_ssl_get_iochannel that can raise errors to the queue. SSL_CTX_new(), SSL_set_fd(), and SSL_CTX_use_certificate_file() being some of them.

I'll add a clear error call in two spots: Before the first function that can add to this queue (SSL_CTX_new()) and before the two calls where we actually use ERR_get_error()). Do you think that is sufficient?

Possibly we could have it before each function that can add to the queue, but that seems like it will clutter the code. Likely we could rely on having cleared it earlier in the function, and deal with errors as they arise.

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 14, 2016

We actually care about the code paths where we show the errors, beside the ones that could make the IO actions fail and you already covered those. The remaining spots are the two ERR_get_error callsites which the latest commit covers too, so it looks good to me as-is.

LemonBoy added a commit that referenced this pull request Mar 17, 2016

Merge pull request #439 from horgh/ssl-errors
Clear error queue before SSL I/O operations

@LemonBoy LemonBoy merged commit 795b7de into irssi:master Mar 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Mar 22, 2016

Merge pull request irssi#439 from horgh/ssl-errors
Clear error queue before SSL I/O operations

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Mar 22, 2016

Merge pull request irssi#439 from horgh/ssl-errors
Clear error queue before SSL I/O operations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment