openssl: Fix return value from tls_net_read. #4100

krispraws · 2021-09-17T22:47:38Z

Error codes returned from SSL_get_error() are positive values: https://github.com/openssl/openssl/blob/master/include/openssl/ssl.h.in#L1181
Without this fix, if the read fails and SSL_get_error() returned SSL_ERROR_SSL or any other similar error, tls_net_read returns 1 to the caller which thinks it read 1 byte.

Addresses

Addresses #4098

Testing
Before we can approve your change; please submit the following in a comment:

Example configuration file for the change
Debug log output from testing the change

Attached Valgrind output that shows no leaks or memory corruption was found

Documentation

Documentation required for this feature

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

edsiper · 2021-09-17T23:00:03Z

Some fixes are needed. Please sign-off your commits otherwise DCO won't pass (git commit -s -m "..")

In your other branch, you also have ERR_clear_error(); and you are right, that is needed before every SSL_Read|Write call.

Let's take some inspiration from Curl OpenSSL backend usage:

https://github.com/curl/curl/blob/master/lib/vtls/openssl.c#L4210-L4270 (fixed link)

krispraws · 2021-09-17T23:32:02Z

The curl implementation is a good reference but it is a little different from the current one.
I won’t be able to make major changes and test in time for today's release.
If you want it today,

I can add the ERR_clear_error() to tls_net_read, tls_net_write and tls_net_handshake.
In https://github.com/fluent/fluent-bit/blob/master/src/tls/openssl.c#L362, change if( ret < 0) to if (ret != SSL_ERROR_NONE && ret != SSL_ERROR_ZERO_RETURN)
SSL_ERROR_NONE is not really needed because we only call SSL_get_err() if SSL_read() < = 0 unlike the curl version. And the docs say SSL_ERROR_NONE The TLS/SSL I/O operation completed. This result code is returned if and only if ret > 0.
I read conflicting opinions on handling SSL_ERROR_ZERO_RETURN so I am not confident about that.

My other branch based off 1.7.5 has changes to clear the error queue before and after the read/write/handshake call, and print the errors. it also checks errno for SSL_ERROR_SYSCALL. I can compare it with curl's implementation next week.

krispraws · 2021-09-17T23:49:06Z

Looking at

fluent-bit/src/tls/flb_tls.c

Line 210 in 37aa680

else if (ret == 0) {

, a 0 return is also treated as an error so returning distinct values for anything other than WANT_READ/WANT_WRITE is not going to make a difference to behaviour.
Current version:

ret = SSL_read(session->ssl, buf, len);
    if (ret <= 0) {
        ret = SSL_get_error(session->ssl, ret);
        if (ret == SSL_ERROR_WANT_READ) {
            ret = FLB_TLS_WANT_READ;
        }
        else if (ret < 0) {
            ret = -1;
        }
    }

PR change:

ret = SSL_read(session->ssl, buf, len);
    if (ret <= 0) {
        ret = SSL_get_error(session->ssl, ret);
        if (ret == SSL_ERROR_WANT_READ) {
            ret = FLB_TLS_WANT_READ;
        }
        else {
            ret = -1;
        }
    }

Adding a special check for other errors won't make a difference, IMO.

edsiper · 2021-09-17T23:58:08Z

note that current version and PR change commented above are the same code

krispraws · 2021-09-18T00:06:08Z

I had a typo - fixed it. So in the current version, i don't distinguish between errors other than WANT_READ/WANT_WRITE. And I don't see what I can include without more testing. Adding special cases for SSL_ERROR_ZERO_RETURN and SSL_ERROR_NONE won't make a difference as I explained.

Can you be more clear about what you are expecting for today's release?

krispraws · 2021-09-18T00:06:50Z

Updated the commit to clear error queue.

edsiper · 2021-09-18T00:19:06Z

src/tls/openssl.c

    ret = SSL_read(session->ssl, buf, len);
    if (ret <= 0) {
        ret = SSL_get_error(session->ssl, ret);
        if (ret == SSL_ERROR_WANT_READ) {
            ret = FLB_TLS_WANT_READ;
        }
-        else if (ret < 0) {


here is a thing (and I still need to clarify more): the error might describe that "wants to read or write" in the case of "SSL_ERROR_WANT_WRITE", by definition:

#define SSL_ERROR_WANT_WRITE 3

that is not an error and removing the < 0 will make that case to fail. Not sure about the impact of that change.

I understand that. However it is already a bug. tls_net_read is returning 3 today for SSL_ERROR_WANT_WRITE which is worse because it makes the http client adds 3 garbage bytes to its response buffer. It doesn't log an error but it will corrupt memory later on.

Added the change to handle SSL_ERROR_WANT_WRITE.

edsiper · 2021-09-18T00:20:35Z

I think until we don't have full clarity of the impact of the change, let's put this PR on stand-by, I will ask other folks to provide their opinion on this (cc: @nokute78 @leonardo-albertovich)

krispraws · 2021-09-18T00:26:11Z

Thanks for clarifying. I think we do need to handle SSL_ERROR_WANT_WRITE for SSL_read and I asked about it in #4098.
In the current code, if SSL_ERROR_WANT_WRITE occurs, it will return 3 and the caller will think it read 3 bytes, which is wrong.
With the version in this PR, we will return -1. This is not ideal but it will at least kill the connection rather than adding garbage data to the response. I have an unpublished commit that actually handles SSL_ERROR_WANT_WRITE correctly but it is based off 1.7.5 and I wanted to get other's opinions.

krispraws · 2021-09-18T00:43:35Z

Holding off on this PR sounds fine. The fix for handling SSL_ERROR_WANT_WRITE is in my other branch: krispraws@d999787
I will create a formal PR next week. Thanks

nokute78 · 2021-09-19T11:59:58Z

Looks good to me.

By the way, we may need to take care a case which mbedtls_ssl_read returns MBEDTLS_ERR_SSL_WANT_WRITE.
https://tls.mbed.org/api/ssl_8h.html#aa2c29eeb1deaf5ad9f01a7515006ede5

hossain-rayhan · 2021-10-18T02:28:05Z

Hi @edsiper what's our plan on this PR now?

github-actions · 2021-11-18T01:47:45Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

leonardo-albertovich

This looks good to me, I double checked the documentation to be sure that the event loop event type switch is correct and indeed it is so let's make one last round of checkins to ensure that we all agree about it : @edsiper @krispraws

krispraws · 2021-11-19T07:24:29Z

This looks good to me, I double checked the documentation to be sure that the event loop event type switch is correct and indeed it is so let's make one last round of checkins to ensure that we all agree about it : @edsiper @krispraws

I will wait a day for additional comments and rebase this PR.

nokute78 · 2021-12-16T12:22:33Z

@krispraws Could you fix conflict ?

Note: #4369 and aws/aws-for-fluent-bit#278 (comment) may be fixed by this patch.

Signed-off-by: Ramya <[email protected]>

krispraws · 2021-12-16T15:29:37Z

@nokute78 , I have rebased against the latest master branch and fixed conflicts. Please review.

nokute78 · 2021-12-18T12:05:58Z

@krispraws Thank you for fixing.

nokute78

@edsiper

I think until we don't have full clarity of the impact of the change, let's put this PR on stand-by, I will ask other folks to provide their opinion on this (cc: @nokute78 @leonardo-albertovich)

I and @leonardo-albertovich approved this patch.
Let's merge this PR.

nokute78 · 2021-12-22T23:04:54Z

@edsiper I can't merge this since At least 1 approving review is required to merge this pull request..
Could you review this ?

krispraws · 2022-01-04T22:05:25Z

@edsiper , Is there anything else needed for this PR?

edsiper · 2022-01-06T19:18:00Z

thanks everybody for your patience, I will re-force the CI since it's stuck and merge it as soon as is ready

edsiper · 2022-01-06T19:20:38Z

looks like everything is good, just a problem with the CI message update

edsiper · 2022-01-06T19:22:57Z

@krispraws @nokute78 @leonardo-albertovich @PettitWesley

are you ok with moving this change also to 1.8 branch for the next 1.8.12 release ?

krispraws · 2022-01-06T21:55:26Z

@edsiper , yes, we should move this to 1.8. Do you want me to create a PR for that?

edsiper · 2022-01-06T22:12:38Z

@krispraws thanks, yes please! so we can make it part of 1.8.12

krispraws · 2022-01-06T23:56:55Z

@edsiper, Backport PR for 1.8 is up : #4584

…4100) Signed-off-by: Ramya <[email protected]>

github-actions bot added the docs-required label Sep 17, 2021

krispraws force-pushed the tls_net_read_fix branch from edac0e0 to 2622e87 Compare September 18, 2021 00:00

edsiper reviewed Sep 18, 2021

View reviewed changes

krispraws force-pushed the tls_net_read_fix branch from 2622e87 to 8d1cfeb Compare September 18, 2021 23:49

github-actions bot added the Stale label Nov 18, 2021

leonardo-albertovich previously approved these changes Nov 18, 2021

View reviewed changes

github-actions bot removed the Stale label Nov 20, 2021

matthewfala mentioned this pull request Dec 3, 2021

Bump version to 2.21.3 aws/aws-for-fluent-bit#275

Merged

tls: Issue fluent#4098 Fix error handling for OpenSSL apis

0ff059f

Signed-off-by: Ramya <[email protected]>

dylanlingelbach mentioned this pull request Dec 16, 2021

OOMs/crashes when using fluent-bit 1.8.11 aws/aws-for-fluent-bit#278

Closed

krispraws dismissed leonardo-albertovich’s stale review via 0ff059f December 16, 2021 15:26

krispraws force-pushed the tls_net_read_fix branch from 8d1cfeb to 0ff059f Compare December 16, 2021 15:26

krispraws requested review from fujimotos and koleini as code owners December 16, 2021 15:26

nokute78 approved these changes Dec 18, 2021

View reviewed changes

PettitWesley requested review from edsiper and leonardo-albertovich December 20, 2021 20:06

leonardo-albertovich approved these changes Dec 21, 2021

View reviewed changes

edsiper mentioned this pull request Jan 6, 2022

1.9: Track release and pendings #4581

Closed

15 tasks

edsiper merged commit a8eaa0a into fluent:master Jan 6, 2022

krispraws mentioned this pull request Jan 6, 2022

tls: Issue #4098 Fix error handling for OpenSSL apis #4584

Merged

This was referenced Jan 8, 2022

FluentBit's tls wrapper function for SSL_read returns SSL errors as positive numbers indicating bytes read. #4098

Closed

Segfault during S3 upload #4369

Closed

lecaros added this to the Fluent Bit v1.9 milestone Jan 14, 2022

0Delta pushed a commit to 0Delta/fluent-bit that referenced this pull request Jan 20, 2022

tls: openssl: fix error handling for OpenSSL API fluent#4098 (fluent#…

02855f0

…4100) Signed-off-by: Ramya <[email protected]>

zhonghui12 mentioned this pull request Jan 29, 2022

Bump version to 2.22.0 aws/aws-for-fluent-bit#291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openssl: Fix return value from tls_net_read. #4100

openssl: Fix return value from tls_net_read. #4100

krispraws commented Sep 17, 2021 •

edited

Loading

edsiper commented Sep 17, 2021 •

edited

Loading

krispraws commented Sep 17, 2021

krispraws commented Sep 17, 2021 •

edited

Loading

edsiper commented Sep 17, 2021

krispraws commented Sep 18, 2021

krispraws commented Sep 18, 2021

edsiper Sep 18, 2021

krispraws Sep 18, 2021

krispraws Sep 19, 2021

edsiper commented Sep 18, 2021

krispraws commented Sep 18, 2021

krispraws commented Sep 18, 2021

nokute78 commented Sep 19, 2021

hossain-rayhan commented Oct 18, 2021

github-actions bot commented Nov 18, 2021

leonardo-albertovich left a comment

krispraws commented Nov 19, 2021

nokute78 commented Dec 16, 2021

krispraws commented Dec 16, 2021

nokute78 commented Dec 18, 2021

nokute78 left a comment

nokute78 commented Dec 22, 2021

krispraws commented Jan 4, 2022

edsiper commented Jan 6, 2022

edsiper commented Jan 6, 2022

edsiper commented Jan 6, 2022

krispraws commented Jan 6, 2022

edsiper commented Jan 6, 2022

krispraws commented Jan 6, 2022

openssl: Fix return value from tls_net_read. #4100

openssl: Fix return value from tls_net_read. #4100

Conversation

krispraws commented Sep 17, 2021 • edited Loading

Addresses

edsiper commented Sep 17, 2021 • edited Loading

krispraws commented Sep 17, 2021

krispraws commented Sep 17, 2021 • edited Loading

edsiper commented Sep 17, 2021

krispraws commented Sep 18, 2021

krispraws commented Sep 18, 2021

edsiper Sep 18, 2021

Choose a reason for hiding this comment

krispraws Sep 18, 2021

Choose a reason for hiding this comment

krispraws Sep 19, 2021

Choose a reason for hiding this comment

edsiper commented Sep 18, 2021

krispraws commented Sep 18, 2021

krispraws commented Sep 18, 2021

nokute78 commented Sep 19, 2021

hossain-rayhan commented Oct 18, 2021

github-actions bot commented Nov 18, 2021

leonardo-albertovich left a comment

Choose a reason for hiding this comment

krispraws commented Nov 19, 2021

nokute78 commented Dec 16, 2021

krispraws commented Dec 16, 2021

nokute78 commented Dec 18, 2021

nokute78 left a comment

Choose a reason for hiding this comment

nokute78 commented Dec 22, 2021

krispraws commented Jan 4, 2022

edsiper commented Jan 6, 2022

edsiper commented Jan 6, 2022

edsiper commented Jan 6, 2022

krispraws commented Jan 6, 2022

edsiper commented Jan 6, 2022

krispraws commented Jan 6, 2022

krispraws commented Sep 17, 2021 •

edited

Loading

edsiper commented Sep 17, 2021 •

edited

Loading

krispraws commented Sep 17, 2021 •

edited

Loading