Re: Unexpected EOF handling

[Kurt Roeckx]

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

[Dmitry Belyavsky]

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: Unexpected EOF handling

[Tomas Mraz]

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.]

Re: Unexpected EOF handling

[Kurt Roeckx]

On Fri, May 08, 2020 at 01:27:00PM +0300, Dmitry Belyavsky wrote:
> On Fri, May 8, 2020 at 1:10 PM Kurt Roeckx  wrote:
> >
> > 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.
> >
> 
> If I remember correctly, session resumption is a way to significantly
> reduce a server's workload.
> So I think that by default (and maybe the only option) we should prefer the
> old behaviour.

If it's a fatal error, the session should be marked as bad. So if
you want that by default we don't mark is as bad, the default
should be that it's a non-fatal error, and we don't want to return
SSL_ERROR_ZERO_RETURN by default.

SSL_ERROR_SYSCALL with errno 0 does not look like a good long term
API to indicate a non-fatal error. And a different error is
also not what people want.


Kurt

Re: Unexpected EOF handling

[Dmitry Belyavsky]

Dear Kurt

On Fri, May 8, 2020 at 1:10 PM 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.
>

If I remember correctly, session resumption is a way to significantly
reduce a server's workload.
So I think that by default (and maybe the only option) we should prefer the
old behaviour.

-- 
SY, Dmitry Belyavsky

Re: Unexpected EOF handling

[Kurt Roeckx]

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.


Kurt

Re: Unexpected EOF handling

[Matt Caswell]

On 07/05/2020 20:28, Dmitry Belyavsky wrote:
> From my point of view, if we don't revert the change for the sake of API
> clarity, we need to provide an option restoring old behaviour at least
> for test purposes.

Presumably nginx can already handle the situation where a close_notify
*is* received. So if we have an option to behave as if that had occurred
in the event of eof then nginx should be able to handle it without
requiring special codepaths?? If so then we don't necessarily have to
have an option to restore the old behaviour - just an option to treat
eof like a close_notify.

Matt


> 
> On Thu, May 7, 2020 at 2:52 PM Tomas Mraz  > wrote:
> 
> On Thu, 2020-05-07 at 13:22 +0200, Kurt Roeckx wrote:
> > Hi,
> >
> > We introduced a change in the 1.1.1e release that changed how we
> > handled an unexpected EOF. This broke various software which
> > resulted in the release of 1.1.1f that reverted that change.
> > In master we still have the 1.1.1e behaviour.
> >
> > The change itself is small, it just calls SSLfatal() with a new
> > error code. This has at least 2 effects:
> > - The error code was changed from SSL_ERROR_SYSCALL with errno 0
> >   to SSL_ERROR_SSL with reason code
> >   SSL_R_UNEXPECTED_EOF_WHILE_READING
> > - The session is marked as in error, and so can't be used to
> >   resume.
> >
> > There is code written that now checks for the SSL_ERROR_SYSCALL
> > with errno 0 case, while we never documented that behaviour. The
> > 1.1.1 manpage of SSL_get_error() currently reports this as a bug
> > and that it will change to SSL_ERROR_SSL /
> > SSL_R_UNEXPECTED_EOF_WHILE_READING.
> >
> > Code that checks the SSL_ERROR_SYSCALL with errno 0 seem to fall
> > in at least 2 categories:
> > - Ignore the error
> > - Check for the error, for instance in a test suite
> >
> > Not sending the close notify alert is a violation of the TLS
> > specifications, but there is software out there doesn't send it,
> > so there is a need to be able to deal with peers that don't send
> > it.
> >
> > When the close notify alert wasn't received, but we did get an
> > EOF, there are 2 cases:
> > - It's a fatal error, the protocol needs the close notify alert to
> >   prevent a truncation attack
> > - The protocol running on top of SSL can detect a truncation
> >   attack itself, and does so. It doesn't need the close notify
> >   alert. It's not a fatal error. They just want to check that that
> >   is what happened.
> >
> > The problem is that we can't make a distiction between the 2
> > cases, so the only thing we can do currently is to look at
> > it as a fatal error. So the documentation was changed to say
> > that if you're in the 2nd cases, and need to talk to a peer
> > that doesn't send the close notify alert, you should not wait
> > for the close notify alert, so that you don't get an error.
> >
> > The alternative is that we add an option that does tell us if we
> > should look at it as a fatal error or not.
> >
> > There is at least a request that we keep returning the old error code
> > (SSL_ERROR_SYSCALL with errno 0). I think that from an API point
> > of view this is very confusing. We'd then have SSL_ERROR_SYSCALL
> > as documented that it's fatal, except when errno is 0. If errno is
> > 0, it can either be fatal or not, and we're not telling you which
> > one it is. I think we also can't guarantee that SSL_ERROR_SYSCALL
> > with errno 0 is always the unexpected EOF, we returned that
> > combination because of a bug in OpenSSL.
> >
> > 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?
> 
> From application developer side of view for protocols that do not
> depend on SSL detecting the truncation I think inventing a different
> behavior for this condition than the existing one as of 1.1.1f would be
> clearly wrong. Switching on a SSL_OP if that newly provided OP is a
> trivial change to an application that needs to accommodate various
> versions of OpenSSL (and I am not talking about forks), however
> switching on a SSL_OP and also add another special error case is much
> more complicated change and has potential for bringing regressions in
> the applications that need to do it.
> 
> It is also an API break, however we would do it anyway unless we make
> the legacy behavior the default one, so that is not really relevant
> here.
> 
> Is there really another situation where SSL_ERROR_SYSCALL with errno 0
> could be returned apart from the unclean EOF condition?
> 
> I can't really think of any.
> 
> So I would be just for properly 

Re: Unexpected EOF handling

[Dmitry Belyavsky]

Hello,

I want to draw everybody's attention to the position (and argumentation) of
Nginx developers:
===

As already mentioned by Dmitry, we here at nginx don't think the change was
necessary. As Matt already said above in the comments to SSL_CONF_cmd.pod
change, the error was always reported. The only issue is that
SSL_ERROR_SYSCALL with a 0 errno is not properly documented. On the other
hand, the behaviour was present since ancient OpenSSL versions, and
actually tested in various software using OpenSSL library, including nginx.
A better solution, in our opinion, would be to document the error instead.

Right now the situation in OpenSSL 3.0 is that the error reporting
behaviour was changed, and, if we are going to support OpenSSL 3.0, we have
to introduce specific error testing for OpenSSL 3.0. And at the same time
we have to support previous error reporting, since we support OpenSSL
versions starting from OpenSSL 0.9.8, as well as other libraries such as
BoringSSL and LibreSSL, which still report connection close with
SSL_ERROR_SYSCALL with a 0 errno.

For obvious reasons we don't want to support multiple code paths to test
for the same error. Especially keeping in mind that due to BoringSSL and
LibreSSL we probably have to support these multiple code paths forever.

It would be really helpful if the change in question was reverted and the
existing behaviour (that is, SSL_ERROR_SYSCALL with a 0 errno) was
documented instead.
===

>From my point of view, if we don't revert the change for the sake of API
clarity, we need to provide an option restoring old behaviour at least for
test purposes.

On Thu, May 7, 2020 at 2:52 PM Tomas Mraz  wrote:

> On Thu, 2020-05-07 at 13:22 +0200, Kurt Roeckx wrote:
> > Hi,
> >
> > We introduced a change in the 1.1.1e release that changed how we
> > handled an unexpected EOF. This broke various software which
> > resulted in the release of 1.1.1f that reverted that change.
> > In master we still have the 1.1.1e behaviour.
> >
> > The change itself is small, it just calls SSLfatal() with a new
> > error code. This has at least 2 effects:
> > - The error code was changed from SSL_ERROR_SYSCALL with errno 0
> >   to SSL_ERROR_SSL with reason code
> >   SSL_R_UNEXPECTED_EOF_WHILE_READING
> > - The session is marked as in error, and so can't be used to
> >   resume.
> >
> > There is code written that now checks for the SSL_ERROR_SYSCALL
> > with errno 0 case, while we never documented that behaviour. The
> > 1.1.1 manpage of SSL_get_error() currently reports this as a bug
> > and that it will change to SSL_ERROR_SSL /
> > SSL_R_UNEXPECTED_EOF_WHILE_READING.
> >
> > Code that checks the SSL_ERROR_SYSCALL with errno 0 seem to fall
> > in at least 2 categories:
> > - Ignore the error
> > - Check for the error, for instance in a test suite
> >
> > Not sending the close notify alert is a violation of the TLS
> > specifications, but there is software out there doesn't send it,
> > so there is a need to be able to deal with peers that don't send
> > it.
> >
> > When the close notify alert wasn't received, but we did get an
> > EOF, there are 2 cases:
> > - It's a fatal error, the protocol needs the close notify alert to
> >   prevent a truncation attack
> > - The protocol running on top of SSL can detect a truncation
> >   attack itself, and does so. It doesn't need the close notify
> >   alert. It's not a fatal error. They just want to check that that
> >   is what happened.
> >
> > The problem is that we can't make a distiction between the 2
> > cases, so the only thing we can do currently is to look at
> > it as a fatal error. So the documentation was changed to say
> > that if you're in the 2nd cases, and need to talk to a peer
> > that doesn't send the close notify alert, you should not wait
> > for the close notify alert, so that you don't get an error.
> >
> > The alternative is that we add an option that does tell us if we
> > should look at it as a fatal error or not.
> >
> > There is at least a request that we keep returning the old error code
> > (SSL_ERROR_SYSCALL with errno 0). I think that from an API point
> > of view this is very confusing. We'd then have SSL_ERROR_SYSCALL
> > as documented that it's fatal, except when errno is 0. If errno is
> > 0, it can either be fatal or not, and we're not telling you which
> > one it is. I think we also can't guarantee that SSL_ERROR_SYSCALL
> > with errno 0 is always the unexpected EOF, we returned that
> > combination because of a bug in OpenSSL.
> >
> > 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?
>
> From application developer side of view for protocols that do not
> depend on SSL detecting the truncation I think inventing a different
> behavior for this condition than the existing one as of 1.1.1f would be
> clearly wrong. Switching on a SSL_OP if that newly provided OP is 

Re: Unexpected EOF handling

[Tomas Mraz]

On Thu, 2020-05-07 at 15:45 +0200, Kurt Roeckx wrote:
> On Thu, May 07, 2020 at 03:15:22PM +0200, Tomas Mraz wrote:
> > Actually the coincidence is that the errno is set to 0 on EOF. So
> > yes,
> > we should explicitly clear the errno on EOF so any leftover value
> > from
> > previous calls does not affect this.
> 
> On EOF, errno is normally not modified. It's value is not defined
> if no error is returned. It is not guaranteed to be 0 on success
> or EOF. It can be modified, because the implementation might have
> done other system calls that did return an error. But a simple test
> shows that it's not modified on my system.
> 

Yeah, that's what I actually meant, sorry for not being clear. I did
not mean that the errno is explicitly set to 0 on EOF by the read call
but that the errno is 0 because it is not modified and was 0 before
coincidentally. 

-- 
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.]

Re: Unexpected EOF handling

[Kurt Roeckx]

On Thu, May 07, 2020 at 03:15:22PM +0200, Tomas Mraz wrote:
> 
> Actually the coincidence is that the errno is set to 0 on EOF. So yes,
> we should explicitly clear the errno on EOF so any leftover value from
> previous calls does not affect this.

On EOF, errno is normally not modified. It's value is not defined
if no error is returned. It is not guaranteed to be 0 on success
or EOF. It can be modified, because the implementation might have
done other system calls that did return an error. But a simple test
shows that it's not modified on my system.


Kurt

Re: Unexpected EOF handling

[Tomas Mraz]

On Thu, 2020-05-07 at 14:53 +0200, Kurt Roeckx wrote:
> On Thu, May 07, 2020 at 01:46:05PM +0200, Tomas Mraz wrote:
> > From application developer side of view for protocols that do not
> > depend on SSL detecting the truncation I think inventing a
> > different
> > behavior for this condition than the existing one as of 1.1.1f
> > would be
> > clearly wrong. Switching on a SSL_OP if that newly provided OP is a
> > trivial change to an application that needs to accommodate various
> > versions of OpenSSL (and I am not talking about forks), however
> > switching on a SSL_OP and also add another special error case is
> > much
> > more complicated change and has potential for bringing regressions
> > in
> > the applications that need to do it.
> 
> Of course, just adding a call to get the old behaviour back is a
> very easy change. But I currently don't see how a different error
> is that much more complicated.
> 
> > Is there really another situation where SSL_ERROR_SYSCALL with
> > errno 0
> > could be returned apart from the unclean EOF condition?
> > 
> > I can't really think of any.
> 
> It's not because we can't think of any other case that there aren't
> any.
> 
> I also have a problem with checking errno being 0. We don't set
> errno. There is no reason to assume that errno is set to any
> value. errno can be modified on a succesful call. That errno is 0
> is just a coincidence. If we're going to document that, we should
> actually make sure that that is the case.

Actually the coincidence is that the errno is set to 0 on EOF. So yes,
we should explicitly clear the errno on EOF so any leftover value from
previous calls does not affect this.

> I think the other property of the old behaviour is that we don't
> add anything to the error stack.
> 
> > So I would be just for properly documenting the condition and
> > keeping
> > it as is if the SSL_OP to ignore unclean EOF is in effect.
> 
> And also don't add an option for applications that do want to get
> a fatal error?

Why another option? That would be the default behavior.

But anyway I like the Matt's proposal even more.

-- 
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.]

Re: Unexpected EOF handling

[Kurt Roeckx]

On Thu, May 07, 2020 at 01:46:05PM +0200, Tomas Mraz wrote:
> From application developer side of view for protocols that do not
> depend on SSL detecting the truncation I think inventing a different
> behavior for this condition than the existing one as of 1.1.1f would be
> clearly wrong. Switching on a SSL_OP if that newly provided OP is a
> trivial change to an application that needs to accommodate various
> versions of OpenSSL (and I am not talking about forks), however
> switching on a SSL_OP and also add another special error case is much
> more complicated change and has potential for bringing regressions in
> the applications that need to do it.

Of course, just adding a call to get the old behaviour back is a
very easy change. But I currently don't see how a different error
is that much more complicated.

> Is there really another situation where SSL_ERROR_SYSCALL with errno 0
> could be returned apart from the unclean EOF condition?
> 
> I can't really think of any.

It's not because we can't think of any other case that there aren't
any.

I also have a problem with checking errno being 0. We don't set
errno. There is no reason to assume that errno is set to any
value. errno can be modified on a succesful call. That errno is 0
is just a coincidence. If we're going to document that, we should
actually make sure that that is the case.

I think the other property of the old behaviour is that we don't
add anything to the error stack.

> So I would be just for properly documenting the condition and keeping
> it as is if the SSL_OP to ignore unclean EOF is in effect.

And also don't add an option for applications that do want to get
a fatal error?


Kurt

Re: Unexpected EOF handling

[Tomas Mraz]

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.

The existing legacy way does not really prevent applications from
detecting the truncation attack because the condition is reported to
the application albeit in this legacy undocumented way. So changing the
default to mean - never report the unclean EOF condition at all, simply
return as if the shutdown was clean, would actually mean a security
issue for these existing applications that care about the unclean EOF.

So yes, perhaps it would be better for the SSL_OP to actually fully
ignore the unclean EOF instead of returning this undocumented error
condition, but it must not be a default (not even in SSL_OP_ALL).

-- 
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.]

Re: Unexpected EOF handling

[Tomas Mraz]

On Thu, 2020-05-07 at 13:22 +0200, Kurt Roeckx wrote:
> Hi,
> 
> We introduced a change in the 1.1.1e release that changed how we
> handled an unexpected EOF. This broke various software which
> resulted in the release of 1.1.1f that reverted that change.
> In master we still have the 1.1.1e behaviour.
> 
> The change itself is small, it just calls SSLfatal() with a new
> error code. This has at least 2 effects:
> - The error code was changed from SSL_ERROR_SYSCALL with errno 0
>   to SSL_ERROR_SSL with reason code
>   SSL_R_UNEXPECTED_EOF_WHILE_READING
> - The session is marked as in error, and so can't be used to
>   resume.
> 
> There is code written that now checks for the SSL_ERROR_SYSCALL
> with errno 0 case, while we never documented that behaviour. The
> 1.1.1 manpage of SSL_get_error() currently reports this as a bug
> and that it will change to SSL_ERROR_SSL /
> SSL_R_UNEXPECTED_EOF_WHILE_READING.
> 
> Code that checks the SSL_ERROR_SYSCALL with errno 0 seem to fall
> in at least 2 categories:
> - Ignore the error
> - Check for the error, for instance in a test suite
> 
> Not sending the close notify alert is a violation of the TLS
> specifications, but there is software out there doesn't send it,
> so there is a need to be able to deal with peers that don't send
> it.
> 
> When the close notify alert wasn't received, but we did get an
> EOF, there are 2 cases:
> - It's a fatal error, the protocol needs the close notify alert to
>   prevent a truncation attack
> - The protocol running on top of SSL can detect a truncation
>   attack itself, and does so. It doesn't need the close notify
>   alert. It's not a fatal error. They just want to check that that
>   is what happened.
> 
> The problem is that we can't make a distiction between the 2
> cases, so the only thing we can do currently is to look at
> it as a fatal error. So the documentation was changed to say
> that if you're in the 2nd cases, and need to talk to a peer
> that doesn't send the close notify alert, you should not wait
> for the close notify alert, so that you don't get an error.
> 
> The alternative is that we add an option that does tell us if we
> should look at it as a fatal error or not.
> 
> There is at least a request that we keep returning the old error code
> (SSL_ERROR_SYSCALL with errno 0). I think that from an API point
> of view this is very confusing. We'd then have SSL_ERROR_SYSCALL
> as documented that it's fatal, except when errno is 0. If errno is
> 0, it can either be fatal or not, and we're not telling you which
> one it is. I think we also can't guarantee that SSL_ERROR_SYSCALL
> with errno 0 is always the unexpected EOF, we returned that
> combination because of a bug in OpenSSL.
> 
> 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?

>From application developer side of view for protocols that do not
depend on SSL detecting the truncation I think inventing a different
behavior for this condition than the existing one as of 1.1.1f would be
clearly wrong. Switching on a SSL_OP if that newly provided OP is a
trivial change to an application that needs to accommodate various
versions of OpenSSL (and I am not talking about forks), however
switching on a SSL_OP and also add another special error case is much
more complicated change and has potential for bringing regressions in
the applications that need to do it.

It is also an API break, however we would do it anyway unless we make
the legacy behavior the default one, so that is not really relevant
here.

Is there really another situation where SSL_ERROR_SYSCALL with errno 0
could be returned apart from the unclean EOF condition?

I can't really think of any.

So I would be just for properly documenting the condition and keeping
it as is if the SSL_OP to ignore unclean EOF is in effect.

-- 
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.]

Re: Unexpected EOF handling

[Matt Caswell]

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???

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.

Matt