Re: Unexpected EOF handling
On Mon, May 11, 2020 at 06:38:40PM +0300, Dmitry Belyavsky wrote: > Dear Tomas, > > I'm not sure this is the best possible solution because it makes the > application developers doing extra compile-time checks. This is a very minimal change they they need to do that doesn't have much risk. But if you have a different suggestion, please say so. > But anyway, is it a final decision and the patch can be amended or we are > waiting for objections some more time? I think there is just a concensus that that is the way forward. There has not been a vote, nor do I see a reason to hold a vote at this point. Kurt
Re: Unexpected EOF handling
Dear Tomas, On Mon, May 11, 2020 at 2:07 PM Tomas Mraz wrote: > On Fri, 2020-05-08 at 12:09 +0200, Kurt Roeckx wrote: > > On Thu, May 07, 2020 at 02:31:24PM +0200, Tomas Mraz wrote: > > > On Thu, 2020-05-07 at 12:47 +0100, Matt Caswell wrote: > > > > On 07/05/2020 12:22, Kurt Roeckx wrote: > > > > > So I think we need at least to agree on: > > > > > - Do we want an option that makes the unexpected EOF either a > > > > > fatal > > > > > error or a non-fatal error? > > > > > - Which error should we return? > > > > > > > > This is an excellent summary of the current situation. > > > > > > > > I am not keen on maintaining the SSL_ERROR_SYSCALL with 0 errno > > > > as a > > > > long term solution. It's a very confusing API for new > > > > applications to > > > > use. Effectively it means SSL_ERROR_SYSCALL is a fatal error - > > > > except > > > > when its not. SSL_ERROR_SYSCALL should mean fatal error. > > > > > > > > That said I also recognise that it is apparently commonplace to > > > > shut > > > > down a TLS connection without sending close_notify - despite what > > > > the > > > > standards may say about it (and TBH we can hardly claim the moral > > > > high > > > > ground here since s_server does exactly this - or at least it > > > > does in > > > > 1.1.1 and did until very recently in master). > > > > > > > > But we do have to consider usages beyond HTTPS. I have no idea if > > > > this > > > > occurs in other settings or not. > > > > > > > > Perhaps what we need is an option for "strict shutdown". With > > > > strict > > > > shutdown "off" we could treat EOF as if we had received a > > > > close_notify > > > > gracefully (and don't invalidate the session). Presumably > > > > existing > > > > code > > > > would be able to cope with that??? > > > > > > Yes, existing code would be able to cope with that with one > > > important > > > exception that I am going to talk about below. > > > > > > > With strict shutdown "on" we treat it as SSL_ERROR_SSL (as now in > > > > master). > > > > > > > > I'm not sure though what the default should be. > > > > > > In case we go with this solution, which would be acceptable I > > > think, we > > > MUST NOT EVER make it the default because existing applications > > > that > > > depend on the existing way how the unclean EOF condition is > > > returned > > > might actually depend on it to detect the truncation attack. > > > > I agree that we should not return SSL_ERROR_ZERO_RETURN by default > > on an unexpected EOF. > > > > If the default behaviour should be to make it a non-fatal error, > > like the old behaviour is, I would really prefer a different > > error, one that's not SSL_ERROR_SYSCALL or SSL_ERROR_SSL. > > > > So I think the suggestion is to have this: > > - By default, SSL_ERROR_SSL is returned with > > SSL_R_UNEXPECTED_EOF_WHILE_READING, the session will be > > marked invalid. > > - With an option, SSL_ERROR_ZERO_RETURN is returned, the session > > will stay valid. > > +1 > > And my suggestion for the SSL_OP name is SSL_OP_IGNORE_UNEXPECTED_EOF. > > Dmitry, I think this solution should be working well for nginx and > similar http related applications. They just need to use the > SSL_OP_IGNORE_UNEXPECTED_EOF and the peers that do not properly > terminate the TLS session will just appear as if they properly > terminated it. > I'm not sure this is the best possible solution because it makes the application developers doing extra compile-time checks. But anyway, is it a final decision and the patch can be amended or we are waiting for objections some more time? -- SY, Dmitry Belyavsky
Re: Some more extra tests
Dear Nicola, Please see https://github.com/openssl/openssl/pull/11792 It currently does not enable TCL and Perl tests, but the C tests also helped me to find regression in the master branch. On Thu, May 7, 2020 at 10:55 PM Dmitry Belyavsky wrote: > Dear Nicola, > > I feel a significant lack of knowledge of preparing such a PR. > If I was able to submit it, I would. > > On Thu, May 7, 2020 at 10:38 PM Nicola Tuveri wrote: > >> I would be interested in seeing a PR to see what enabling these tests >> would require! >> >> I believe we do indeed need to test more thoroughly to ensure we are not >> breaking the engine API! >> >> >> Nicola >> >> On Thu, May 7, 2020, 21:08 Dmitry Belyavsky wrote: >> >>> Dear colleagues, >>> >>> Let me draw your attention to a potentially reasonable set of extended >>> tests for the openssl engines. >>> >>> The gost-engine project (https://github.com/gost-engine/engine) has >>> some test scenarios robust enough for testing engine-provided algorithms >>> and some basic RSA regression tests. It contains a rather eclectic set of >>> C, Perl, and TCL(!) tests that are used by me on a regular basis. >>> >>> If these tests are included in the project extended test suite, they >>> could reduce some regression that sometimes occurs (see >>> https://github.com/gost-engine/engine/issues/232 as a current list of >>> known problems). >>> >>> I will be happy to assist in enabling these tests as a part of openssl >>> test suites. >>> Many thanks! >>> >>> -- >>> SY, Dmitry Belyavsky >>> >> > > -- > SY, Dmitry Belyavsky > -- SY, Dmitry Belyavsky
Re: Unexpected EOF handling
On Fri, 2020-05-08 at 12:09 +0200, Kurt Roeckx wrote: > On Thu, May 07, 2020 at 02:31:24PM +0200, Tomas Mraz wrote: > > On Thu, 2020-05-07 at 12:47 +0100, Matt Caswell wrote: > > > On 07/05/2020 12:22, Kurt Roeckx wrote: > > > > So I think we need at least to agree on: > > > > - Do we want an option that makes the unexpected EOF either a > > > > fatal > > > > error or a non-fatal error? > > > > - Which error should we return? > > > > > > This is an excellent summary of the current situation. > > > > > > I am not keen on maintaining the SSL_ERROR_SYSCALL with 0 errno > > > as a > > > long term solution. It's a very confusing API for new > > > applications to > > > use. Effectively it means SSL_ERROR_SYSCALL is a fatal error - > > > except > > > when its not. SSL_ERROR_SYSCALL should mean fatal error. > > > > > > That said I also recognise that it is apparently commonplace to > > > shut > > > down a TLS connection without sending close_notify - despite what > > > the > > > standards may say about it (and TBH we can hardly claim the moral > > > high > > > ground here since s_server does exactly this - or at least it > > > does in > > > 1.1.1 and did until very recently in master). > > > > > > But we do have to consider usages beyond HTTPS. I have no idea if > > > this > > > occurs in other settings or not. > > > > > > Perhaps what we need is an option for "strict shutdown". With > > > strict > > > shutdown "off" we could treat EOF as if we had received a > > > close_notify > > > gracefully (and don't invalidate the session). Presumably > > > existing > > > code > > > would be able to cope with that??? > > > > Yes, existing code would be able to cope with that with one > > important > > exception that I am going to talk about below. > > > > > With strict shutdown "on" we treat it as SSL_ERROR_SSL (as now in > > > master). > > > > > > I'm not sure though what the default should be. > > > > In case we go with this solution, which would be acceptable I > > think, we > > MUST NOT EVER make it the default because existing applications > > that > > depend on the existing way how the unclean EOF condition is > > returned > > might actually depend on it to detect the truncation attack. > > I agree that we should not return SSL_ERROR_ZERO_RETURN by default > on an unexpected EOF. > > If the default behaviour should be to make it a non-fatal error, > like the old behaviour is, I would really prefer a different > error, one that's not SSL_ERROR_SYSCALL or SSL_ERROR_SSL. > > So I think the suggestion is to have this: > - By default, SSL_ERROR_SSL is returned with > SSL_R_UNEXPECTED_EOF_WHILE_READING, the session will be > marked invalid. > - With an option, SSL_ERROR_ZERO_RETURN is returned, the session > will stay valid. +1 And my suggestion for the SSL_OP name is SSL_OP_IGNORE_UNEXPECTED_EOF. Dmitry, I think this solution should be working well for nginx and similar http related applications. They just need to use the SSL_OP_IGNORE_UNEXPECTED_EOF and the peers that do not properly terminate the TLS session will just appear as if they properly terminated it. -- Tomáš Mráz No matter how far down the wrong road you've gone, turn back. Turkish proverb [You'll know whether the road is wrong if you carefully listen to your conscience.]
