Fix SSL_key_update() problems by mattcaswell · Pull Request #16077 · openssl/openssl

mattcaswell · 2021-07-14T13:50:36Z

This contains 2 fixes affecting SSL_key_update():

Don't reset the packet pointer in ssl3_setup_read_buffer

Sometimes this function gets called when the buffers have already been
set up. If there is already a partial packet in the read buffer then the
packet pointer will be set to an incorrect value. The packet pointer already
gets reset to the correct value when we first read a packet anyway, so we
don't also need to do it in ssl3_setup_read_buffer.

Disallow SSL_key_update() if there are writes pending

If an application is halfway through writing application data it should
not be allowed to attempt an SSL_key_update() operation. Instead the
SSL_write() operation should be completed.

The former issue can result in a "bad record mac" problem if a partial record read occurs around the same time as a key update.

Fixes #13729
Fixes #12485

PR #16021 will provide tests for these scenarios.

This should also be applied to 1.1.1, but I will wait until this is approved to create a PR there since it requires some minor adjustment there.

Sometimes this function gets called when the buffers have already been set up. If there is already a partial packet in the read buffer then the packet pointer will be set to an incorrect value. The packet pointer already gets reset to the correct value when we first read a packet anyway, so we don't also need to do it in ssl3_setup_read_buffer. Fixes openssl#13729

If an application is halfway through writing application data it should not be allowed to attempt an SSL_key_update() operation. Instead the SSL_write() operation should be completed. Fixes openssl#12485

kaduk

Don't reset the packet pointer in ssl3_setup_read_buffer
Sometimes this function gets called when the buffers have already been
set up. If there is already a partial packet in the read buffer then the
packet pointer will be set to an incorrect value. The packet pointer already
gets reset to the correct value when we first read a packet anyway, so we
don't also need to do it in ssl3_setup_read_buffer.

The main initialization of s->rlayer.packet in ssl3_read_n() seems to only occur in the !extend case; in the extend case we seem to memmove from the current packet value before updating it. I guess it would be a programming error to call the "extend" case without a preceding "non-extend" case, so maybe that's good enough.

(Also, the comment at the top of the body of ssl3_read_n() refers to s->packet and s->packet_length, which seems out of date with respect to the RECORD_LAYER abstraction.)

kaduk · 2021-07-14T14:16:38Z

ssl/ssl_lib.c

        return 0;
    }

+    if (RECORD_LAYER_write_pending(&s->rlayer)) {


Hmm, should the check in SSL_new_session_ticket() use RECORD_LAYER_write_pending() instead of peeking into s->rlayer.wbuf[0].left?

Yes. It probably should (different PR though)

hou2gou · 2021-07-15T01:21:50Z

(Also, the comment at the top of the body of ssl3_read_n() refers to s->packet and s->packet_length, which seems out of date with respect to the RECORD_LAYER abstraction.) The following comments have the same problem.

openssl/ssl/record/ssl3_record.c

Lines 430 to 446 in 11f18ef

    
                   if (more > 0) { 
        
                       /* now s->packet_length == SSL3_RT_HEADER_LENGTH */ 
        
                       rret = ssl3_read_n(s, more, more, 1, 0, &n); 
        
                       if (rret <= 0) 
        
                           return rret;     /* error or non-blocking io */ 
        
                   } 
        
                   /* set state for later operations */ 
        
                   RECORD_LAYER_set_rstate(&s->rlayer, SSL_ST_READ_HEADER); 
        
                   /* 
        
                    * At this point, s->packet_length == SSL3_RT_HEADER_LENGTH 
        
                    * + thisrr->length, or s->packet_length == SSL2_RT_HEADER_LENGTH 
        
                    * + thisrr->length and we have that many bytes in s->packet 
        
                    */ 
        
                   if (thisrr->rec_version == SSL2_VERSION) {

mattcaswell · 2021-07-15T12:59:46Z

Hmmmm. Strangely I just saw the message "First-time contributors need a maintainer to approve running workflows" for this PR, and I had to approve the run despite all commits in it being authored by me. I could have sworn I'd made a few commits to this repo before now...

Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in openssl#16077.

mattcaswell · 2021-07-15T13:34:52Z

I've created #16086 to address the problems identified in the various comments (plus a few other instances I found). I also fixed SSL_new_session_ticket to use RECORD_LAYER_write_pending().

openssl-machine · 2021-07-15T15:00:12Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

kaduk · 2021-07-15T22:39:45Z

I think this should go to 1.1.1 as well (the cherry-pick is clean).

kaduk · 2021-07-15T22:41:45Z

the cherry-pick is clean

... but ERR_raise() doesn't exist on 1.1.1

mattcaswell · 2021-07-16T09:48:10Z

... but ERR_raise() doesn't exist on 1.1.1

Yeah, as I said in the introduction to this PR:

This should also be applied to 1.1.1, but I will wait until this is approved to create a PR there since it requires some minor adjustment there.

Sometimes this function gets called when the buffers have already been set up. If there is already a partial packet in the read buffer then the packet pointer will be set to an incorrect value. The packet pointer already gets reset to the correct value when we first read a packet anyway, so we don't also need to do it in ssl3_setup_read_buffer. Fixes #13729 Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16077)

If an application is halfway through writing application data it should not be allowed to attempt an SSL_key_update() operation. Instead the SSL_write() operation should be completed. Fixes #12485 Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16077)

t8m · 2021-07-16T10:21:35Z

Merged to master. Thanks.

t8m · 2021-07-16T10:22:24Z

I've now noticed that I've accidentally forgot to add @kaduk to reviewed-by. I am sorry for that.

Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in #16077. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #16086)

Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in openssl#16077. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from openssl#16086) (cherry picked from commit ca00152)

Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in #16077. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (cherry picked from commit ca00152) Reviewed-by: Paul Dale <[email protected]> (Merged from #16105)

mattcaswell added 2 commits July 14, 2021 14:42

Disallow SSL_key_update() if there are writes pending

8ad410f

If an application is halfway through writing application data it should not be allowed to attempt an SSL_key_update() operation. Instead the SSL_write() operation should be completed. Fixes openssl#12485

mattcaswell added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jul 14, 2021

mattcaswell mentioned this pull request Jul 14, 2021

fix key update fail in non-blocking #16021

Closed

t8m approved these changes Jul 14, 2021

View reviewed changes

t8m added approval: done This pull request has the required number of approvals triaged: bug The issue/pr is/fixes a bug and removed approval: review pending This pull request needs review by a committer labels Jul 14, 2021

kaduk approved these changes Jul 14, 2021

View reviewed changes

hou2gou mentioned this pull request Jul 15, 2021

Add testcases for SSL_key_update problems #16085

Closed

2 tasks

mattcaswell mentioned this pull request Jul 15, 2021

Fix some minor record layer issues #16086

Closed

t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jul 16, 2021

t8m closed this Jul 16, 2021

mattcaswell mentioned this pull request Jul 16, 2021

Fix SSL_key_update() problems (1.1.1) #16098

Closed

sthagen mentioned this pull request Jul 17, 2021

Fix some minor record layer issues sthagen/openssl-openssl#152

Merged

2 tasks

dstebila added a commit to open-quantum-safe/openssl that referenced this pull request Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Fix SSL_key_update() problems#16077

Fix SSL_key_update() problems#16077
mattcaswell wants to merge 2 commits intoopenssl:masterfrom
mattcaswell:key-update-read

mattcaswell commented Jul 14, 2021

Uh oh!

kaduk left a comment

Uh oh!

kaduk Jul 14, 2021

Uh oh!

mattcaswell Jul 15, 2021

Uh oh!

hou2gou commented Jul 15, 2021 •

edited

Loading

Uh oh!

mattcaswell commented Jul 15, 2021

Uh oh!

mattcaswell commented Jul 15, 2021

Uh oh!

openssl-machine commented Jul 15, 2021

Uh oh!

kaduk commented Jul 15, 2021

Uh oh!

kaduk commented Jul 15, 2021

Uh oh!

mattcaswell commented Jul 16, 2021

Uh oh!

t8m commented Jul 16, 2021

Uh oh!

t8m commented Jul 16, 2021

Uh oh!

Uh oh!

Comments

Conversation

mattcaswell commented Jul 14, 2021

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

kaduk Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

mattcaswell Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

hou2gou commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Jul 15, 2021

Uh oh!

mattcaswell commented Jul 15, 2021

Uh oh!

openssl-machine commented Jul 15, 2021

Uh oh!

kaduk commented Jul 15, 2021

Uh oh!

kaduk commented Jul 15, 2021

Uh oh!

mattcaswell commented Jul 16, 2021

Uh oh!

t8m commented Jul 16, 2021

Uh oh!

t8m commented Jul 16, 2021

Uh oh!

hou2gou commented Jul 15, 2021 •

edited

Loading