Skip to content

Comments

Fix an out-of-bounds read for TLS <- 1.2 cipher strings that end in a "-"#19166

Closed
millert wants to merge 1 commit intoopenssl:masterfrom
millert:master
Closed

Fix an out-of-bounds read for TLS <- 1.2 cipher strings that end in a "-"#19166
millert wants to merge 1 commit intoopenssl:masterfrom
millert:master

Conversation

@millert
Copy link
Contributor

@millert millert commented Sep 7, 2022 •
edited
Loading

In ssl_cipher_process_rulestr(), if rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. The trivial fix is to only increment "l" if the last byte read is not a NUL.

CLA: trivial

@millert millert changed the title Fix an out-of-bounds read for TLS <- 1.2 cipher strings that end in a "-"ssl_cipher_process_rulestr: don't read outside rule_str buffer Fix an out-of-bounds read for TLS <- 1.2 cipher strings that end in a "-" Sep 7, 2022
@paulidale paulidale added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug cla: trivial One of the commits is marked as 'CLA: trivial' branch: 3.0 Applies to openssl-3.0 branch labels Sep 7, 2022
@t8m t8m added the approval: done This pull request has the required number of approvals label Sep 8, 2022
@paulidale
Copy link
Contributor

paulidale commented Sep 8, 2022

Confirm trivial? I think it is but I added the label 😃

@t8m
Copy link
Member

t8m commented Sep 8, 2022

I am OK with CLA: trivial for this.

@paulidale should this go into 1.1.1 too? IMO yes, if it applies there.

@paulidale paulidale added the branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) label Sep 8, 2022
@davidben
Copy link
Contributor

davidben commented Sep 8, 2022

Should this have a regression test? That test combined with ASan should provide regression coverage.

davidben
davidben reviewed Sep 8, 2022
View reviewed changes
ssl/ssl_ciph.c Outdated
retval = found = 0;
l++;
if (ch != '\0')
l++;
Copy link
Contributor

@davidben davidben Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, is this the right fix? This is an error condition, ERR_raise and all, and yet we're breaking out of just the inner loop. It seems we actually just want to break out of the outer loop.

Looking back, it seems we hit this with a fuzzer a while back, but our fix was to make the function immediately fail.
https://boringssl-review.googlesource.com/c/boringssl/+/11421/

Copy link
Contributor Author

@millert millert Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning early seems safer.

Copy link
Contributor

@davidben davidben Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It also seems the function will get confused otherwise. If I'm reading this right, I believe break exits to line 1220 ("Ok, we have the rule, now apply it"). That could try to execute CIPHER_SPECIAL or skip over something and then continue running. We may execute more rules, pile on more errors, before finally returning failure anyway because retval is cleared.

@millert
Copy link
Contributor Author

millert commented Sep 8, 2022

For some reason this can be hard to trigger with address sanitizer. The following reliably reproduces the problem for me with gcc -O2 -fsanitize=address -fno-omit-frame-pointer (but does not if -fno-omit-frame-pointer is not specified). With a dynamically allocated ciphers list triggering the bug is more difficult.

#include <stdio.h>
#include <string.h>
#include <openssl/ssl.h>

int
main(int argc, char *argv[])
{
    const char ciphers[] = "ALL:-";
    SSL_CTX *ctx = SSL_CTX_new(TLS_method());
    SSL_CTX_set_cipher_list(ctx, ciphers);
    SSL_CTX_free(ctx);
}

@openssl-machine
Copy link
Collaborator

openssl-machine commented Sep 9, 2022

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.

@t8m t8m added branch: 3.1 Applies to openssl-3.1 (EOL) and removed approval: done This pull request has the required number of approvals labels Oct 24, 2022
@t8m
Copy link
Member

t8m commented Oct 24, 2022

@millert could you please change this according to the @davidben's suggestion?

@millert
ssl_cipher_process_rulestr: don't read outside rule_str buffer
93dcd42 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial
@millert
Copy link
Contributor Author

millert commented Oct 24, 2022

@t8m I updated the commit to just return early as discussed.

@t8m t8m added the approval: review pending This pull request needs review by a committer label Oct 24, 2022
@t8m
Copy link
Member

t8m commented Oct 24, 2022

@paulidale still OK?

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 25, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Oct 26, 2022
@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Oct 26, 2022
@openssl-machine
Copy link
Collaborator

openssl-machine commented Oct 26, 2022

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Oct 26, 2022
@millert @t8m
ssl_cipher_process_rulestr: don't read outside rule_str buffer
428511c 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19166)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2022
@millert @t8m
ssl_cipher_process_rulestr: don't read outside rule_str buffer
91caaa3 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19166)

(cherry picked from commit 428511c)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2022
@millert @t8m
ssl_cipher_process_rulestr: don't read outside rule_str buffer
3efb41c 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19166)

(cherry picked from commit 428511c)
@t8m
Copy link
Member

t8m commented Oct 26, 2022

Merged to all branches (after fixing trivial conflict on 1.1.1 branch). Thank you for your contribution.

@t8m t8m closed this Oct 26, 2022
openssl-machine pushed a commit that referenced this pull request Oct 26, 2022
@millert @t8m
ssl_cipher_process_rulestr: don't read outside rule_str buffer
9b3219b 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19166)

(cherry picked from commit 428511c)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Nov 4, 2022
@millert @bernd-edlinger
ssl_cipher_process_rulestr: don't read outside rule_str buffer
8379a8d 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19166)

(cherry picked from commit 428511c)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Nov 4, 2022
@millert @bernd-edlinger
ssl_cipher_process_rulestr: don't read outside rule_str buffer
a3d26e2 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19166)

(cherry picked from commit 428511c)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Nov 4, 2022
@millert @bernd-edlinger
ssl_cipher_process_rulestr: don't read outside rule_str buffer
6f4de1d 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19166)

(cherry picked from commit 428511c)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@millert @beldmit
ssl_cipher_process_rulestr: don't read outside rule_str buffer
47702d2 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19166)
a-kromm-rogii pushed a commit to a-kromm-rogii/openssl that referenced this pull request Mar 14, 2025
@millert @a-kromm-rogii
ssl_cipher_process_rulestr: don't read outside rule_str buffer
92612c8 
If rule_str ended in a "-", "l" was incremented one byte past the
end of the buffer.  This resulted in an out-of-bounds read when "l"
is dereferenced at the end of the loop.  It is safest to just return
early in this case since the condition occurs inside a nested loop.

CLA: trivial

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19166)

(cherry picked from commit 428511c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

+1 more reviewer

@davidben davidben davidben left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) cla: trivial One of the commits is marked as 'CLA: trivial' triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@millert @paulidale @t8m @davidben @openssl-machine