Skip to content

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

Closed
millert wants to merge 1 commit into
openssl: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 into
openssl:masterfrom
millert:master

Conversation

@millert

@millert millert commented Sep 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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'. Review for non-trivial change in such commit. branch: 3.0 Applies to openssl-3.0 branch (EOL) labels Sep 7, 2022
@t8m t8m added the approval: done This pull request has the required number of approvals label Sep 8, 2022
@paulidale

paulidale commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

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

@t8m

t8m commented Sep 8, 2022

Copy link
Copy Markdown
Member

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

davidben commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

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

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

@davidben davidben Sep 8, 2022

Copy link
Copy Markdown
Contributor

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/

@millert millert Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Returning early seems safer.

@davidben davidben Sep 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

millert commented Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

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

openssl-machine commented Sep 9, 2022

Copy link
Copy Markdown
Collaborator

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

t8m commented Oct 24, 2022

Copy link
Copy Markdown
Member

@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

millert commented Oct 24, 2022

Copy link
Copy Markdown
Contributor Author

@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

t8m commented Oct 24, 2022

Copy link
Copy Markdown
Member

@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

openssl-machine commented Oct 26, 2022

Copy link
Copy Markdown
Collaborator

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

t8m commented Oct 26, 2022

Copy link
Copy Markdown
Member

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 (EOL) branch: 3.1 Applies to openssl-3.1 (EOL) cla: trivial One of the commits is marked as 'CLA: trivial'. Review for non-trivial change in such commit. 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